Incorrect variable always exists

[lookyman]

Summary of a problem or a feature request

PHPStan incorrectly reports that a variable always exists in some cases.

Code snippet that reproduces the problem

https://phpstan.org/r/b0e37609338d0c49645295f039a0e73d

Expected output

PHPStan complains that Variable $handle in isset() always exists and is not nullable.. However, https://3v4l.org/nUfFW shows the actual output is ko, which means the isset gets evaluated to false.

[Majkl578]

Similar to #505.

[ondrejmirtes]

Hi, do you have some other example besides finally block?

On Sat, 20 Jan 2018 at 21:43, Michael Moravec ***@***.***> wrote: Similar to #505 <#505>. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#778 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAGZuObLopSt_ePn7ZoXFkbv5qZ1aU3oks5tMk_5gaJpZM4Rlk8R> .

-- Ondřej Mirtes

[lookyman]

I don't. However, the issue is not limited for the case when isset is inside finally block. See https://phpstan.org/r/e387cd1084ac70eb3ec94099c9e7b060.

EDIT Well now that I think about it, that isset is actually unreachable, so maybe not the same issue.

[ondrejmirtes]

I'm gonna leave this open because of the second example, it will require some advanced exception tracking which is not yet possible.

[lookyman]

@ondrejmirtes I have another example, this time without exceptions entirely:

https://phpstan.org/r/f28dcad21970df5f00f71201c1123320

Interesting fact, if you remove the @return annotation from the bar function, it doesn't report the error.

[ondrejmirtes]

@lookyman Please submit this as a new issue next time, this makes issue management harder 😊 I will look into it.

[ondrejmirtes]

Fixed: 0da95fc

[ondrejmirtes]

I solved the original issue: 7efd23c - basically all the variables in try block might be undefined in the finally block which might produce some unwanted results, let's hope I won't have to revert this one...

[rainbow-alex]

Hey @ondrejmirtes, when I run the code snippet again, it still reports the false negative.

Is this a regression?

[ondrejmirtes]

Yes.

[kunicmarko20]

I just run into this one, here is my use case https://phpstan.org/r/dbe44f1b-cb9d-4867-b266-61beb928fbaa

[rainbow-alex]

@ondrejmirtes I'm trying to tackle this one, but I've gotten stuck.

https://phpstan.org/r/dbe44f1b-cb9d-4867-b266-61beb928fbaa

I was hoping I could check the type of test() (in NodeScopeResolver::processAssignVar) and then pass $certainty = maybe to MutatingScope::assignVariable if it possibly throws. Unfortunately there's no indication that test() may at all throw. I was expecting the type of test() to be something like string|noreturn, but it's just string. I've tried adding test to earlyTerminatingFunctionCalls, but it has no effect on the type either. How can I test if an expression throws?

[ondrejmirtes]

@rainbow-alex That amount of precision is not that easy. We'd have to carry information in ExpressionResult and StatementResult if the expression/statement can throw an exception or not. That way we'd be able to tell if you're just accessing a variable (cannot throw an exception) or a calling a method (can throw an exception, that way we'd be even able to tell which one if you provide @throws annotation). It'd be a bigger initiative.

But this issue is marked as an easy fix - it might be fixable somewhere around the lines (https://github.com/phpstan/phpstan-src/blob/6bcb400f4540adaeaedf67a1929a6e601f69caa4/src/Analyser/NodeScopeResolver.php#L962-L1024), without the regard to what's on the right side of the variable assignment. Basically any variable assignment in try should be considered only as a "maybe". But if there's the same variable assigned in try and all catch statements, it's certain to exist below the try-catch I think

[rainbow-alex]

Basically any variable assignment in try should be considered only as a "maybe".

There are definitely some variable assignments that are safe, though they are not very useful:

try {
     $definitelySet = 3;
...

In review I would always argue for taking that assignment out of the try block, so I am not concerned about considering this a 'maybe' for analysis per se.

But if there's the same variable assigned in try and all catch statements, it's certain to exist below the try-catch I think

Here it gets tricky:

try {
    $var = foo();
} catch (A $e) {
    $var = fallback();
} catch (\Throwable $e) {
    $var = fallback2();
} finally {
    // $var is not necessarily set here, e.g. if either fallback call throws
}
// $var is definitely set here

So yes, we could assign all variables as 'maybe' until the final }. When leaving the try/catch/finally they can be considered certain if (and only if) they're set in each clause. But then we would need to keep track of which variables were assigned in each clause. And I can already foresee further issues, e.g.:

try {
    if (random()) {
        $var = 'x';
    }
} finally {
}
// $var is genuinely 'maybe' here and should not be promoted to certain!

Thinking about it in depth, I think we can accurately model this by just splitting/merging scopes according to the semantics of try/catch/finally:

• we start with the parent scope P
• the try scope T splits from P, any variables it assigns are certain
• for each catch scope we start again from P, but we merge in T; we don't know how much of of T was executed so any variable that was certain only in T becomes maybe
• the finally scope F starts again from P and merges T and all catch scopes; again we don't know how much of each was executed... making all their variables maybe (unless they were set in P)
• the final parent scope when leaving the whole statement is similar to F, but starts merging from T instead of P because we know at least one of the try/catch clauses was executed completely. This way all variables that are certain in T and all catch scopes remain certain.

I think MutatingScope can already do all this splitting/merging correctly and it's just a matter of tweaking the use of scopes in NodeScopeResolver. Does this make sense at all? I can give implementation a try tonight.

[ondrejmirtes]

You can try to do it. You'll see which test cases will fail and we'll probably have to decide which behaviour is better/less annoying :)

[ondrejmirtes]

You can do it in spirit of NodeScopeResolverTest::testFileAsserts (the easiest way to test this) but currently there's only assertType function, no assertVariableCertainty, you'd have to do that first.

[ondrejmirtes]

BTW I implemented assertVariableCertainty: phpstan/phpstan-src@1567d0f

[phpstan-bot]

@kunicmarko20 After the latest commit in dev-master, PHPStan now reports different result with your code snippet:

@@ @@
-15: Variable $file in isset() always exists and is not nullable.
+No errors

[ondrejmirtes]

Fixed by: phpstan/phpstan-src#481

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.