[EA] New value type (bool) is not matching the resolved parameter type

[jtheuerkauf]

Subject	Details
Plugin	Php Inspections (EA Extended)
Language level	8.3

function foo(DateInterval|string $expires) {
    if (is_string($expires)) {
        $exp = date_create($expires);
        $now = date_create();
        $expires = $now->diff($exp) ?: new DateInterval('P30D');
    }
}

Given this code, inspection will detect diff() as potentially returning "bool", not strictly false, so it may cause a type issue.

But since the only Boolean that can be expected is false and it's handled by ?: to a properly typed value, there should be no warning - no possible type collision.

Bear in mind, this is an example of the issue - it's not exclusive to this set of functions.

Expected Result

A couple things:

1. date_create() with an arbitrary string argument may return false but no warning is raised when it's passed to diff().
2. When the potential false return from diff() is handled by a substitute (?: to a default or throw expression, for instance), the warning should be canceled.

function foo(DateInterval|string $expires) {
    $now = date_create();

    // The rest assumes $expires is a string...    

    $exp = date_create($expires);        
    $diff = $now->diff($exp); // Current: No warning
                              // Desired: Potential invalid argument type "false" for $exp

    $exp = date_create($expires) ?: new DateTime($expires);
    // or
    $exp = date_create($expires) ?: throw new DateTimeException();
    $diff = $now->diff($exp); // Current: No warning
                              // > correct: false will proceed to new (which will throw on failure) or throw
                              // > (If anything, the function should warn about unhandled DateTimeException)

    $exp = date_create($expires) ?: new DateTime($expires);
    $expires = $now->diff($exp) ?: new DateInterval('P30D'); // Current: Warning about assigning bool to $expires
                                                             // Desired: No warning - type is preserved by `?:`

    $expires = $now->diff($exp) ?: throw new DateTimeException(); // Current: Warning about assigning bool to $expires
                                                                  // Desired: No warning - DT Exception is thrown
}

Environment details

PhpStorm 2024.1 RC
Build #PS-241.14494.159, built on March 21, 2024
Runtime version: 17.0.10+8-b1207.12 amd64
VM: OpenJDK 64-Bit Server VM by JetBrains s.r.o.
Windows 11.0
GC: G1 Young Generation, G1 Old Generation
Memory: 4096M
Cores: 12
Registry:
  debugger.new.tool.window.layout=true
  run.processes.with.pty=TRUE
  ide.experimental.ui=true
Non-Bundled Plugins:
  com.github.leomillon.uuidgenerator (4.5.1)
  dev.patrickpichler.darculaPitchBlackTheme (1.0.0)
  dev.turingcomplete.intellijdevelopertoolsplugins (4.1.1)
  org.jcorbett.more-black (1.0.0-10)
  org.toml.lang (241.14494.150)
  de.netnexus.camelcaseplugin (3.0.12)
  one.util.ideaplugin.screenshoter (1.8.1)
  com.intellij.ideolog (222.3.2.0)
  com.intellij.properties (241.14494.150)
  String Manipulation (9.12.0)
  krasa.CpuUsageIndicator (1.18.0-IJ2023)
  com.intellij.aqua (241.14494.241)
  com.intellij.grazie.pro (0.3.294)
  de.shyim.ideaphpstantoolbox (0.0.7)
  com.kalessil.phpStorm.phpInspectionsEA (5.0.0.0)
  ru.adelf.idea.dotenv (2024.1)
  de.espend.idea.php.annotation (10.0.0)
  com.elicul.azure-devops-commit-message-plugin (1.0.2)
  com.microsoft.vso.idea (1.162.2)

Screenshot

[Simbiat]

Based on what I see in #1935, the issue is that $now->diff($exp) ?: new DateInterval('P30D') can result in boolean, which is not expected type as per definition of $expires: DateInterval|string $expires. To me this seems like expected behavior, since you can get issues in the code after this re-assingment, if boolean is assigned, and you do not handle that in any way.

[jtheuerkauf]

@Simbiat The line you noted in your comment can't produce false. If diff() results in false from a bad argument, the ternary will go to new DateInterval(), which will either produce the object or throw an exception. Therefore the inspection is incorrectly detecting a bool assignment.

In the other issue you referenced, the variable is being assigned to an item from $_POST which has no inferrable type, but the inspection assumes it's an array. As you point out there, the type for $_POST['avatar'] should be assumed as mixed since there's no hint otherwise.

[Simbiat]

It's not about whether it's proper boolean or just false, it's the fact that you can have a situation, when diff will return false, which is not supposed to be assigned to $expires. Or rather interpreter thinks that way, since technically ?: should not allow that. That's why I think this may be an expected behavior, if interpreter is not supposed to be evaluating the whole expression on the right. But it probably, should.

[jtheuerkauf]

i'm not sure how the interpreter could see false and then bail on the rest of the line. i wouldn't ever expect a statement to be partially interpreted (i.e., stopping short of ;). This isn't JavaScript. 😁

[Simbiat]

Most likely the logic is "go through every variable/value in the expression, and fail on first one that may not match the type". Depending on complexity of the expression, there may not be a easy way to get all possible combinations. But again, we probably should be doing something to interpret all of them. That's why I say that this may be expected behavior, not that it is.