-
-
Notifications
You must be signed in to change notification settings - Fork 946
Description
Bug report
In phpstan 0.12.5, function one in the snippet below reports no errors. 0.12.6 instead reports:
Method HelloWorld::one() should return int but return statement is missing.
The function provided takes a parameter which could be multiple types, which are specified by an annotation, and then validated in code. While the error is technically true, as invalid input could be passed to the function, in the presence of static analysis, it should be provably impossible for this to happen, and thus, it should not be necessary to include an explicit test.
Adding an explicit test (as in function two) results in
Else branch is unreachable because previous condition is always true.
If that is the case, then phpstan should not be complaining about function one.
In function three, where the fall-through exception is "unconditionally" thrown (rather than guarded by an else statement), no error is emitted, even though two and three are functionally identical.
Code snippet that reproduces the problem
https://phpstan.org/r/edd3884d-3b74-4cad-bcc7-11e86575021a
Expected output
I'm not really sure what the best solution here is. In any case, phpstan is being inconsistent with its error reporting. three is a workaround to silence the warning in this case, but it wouldn't work if the code had a different structure (see two_alt).
If phpstan is going to allow three to not report an error (since by type analysis, throwing the exception is provably dead code), then it should probably not complain at all about two and two_alt. Even if redundant (according to docblocks), it will always be safe. If an invalid type is passed into one, a exception is going to be thrown anyway by PHP when the function fails to return the expected type, a behavior that is matched in two (but made explicit).
Perhaps the rule should be: if a provably-redundant else branch results in an exception, then silence the unreachability warning since the code is intended as a defensive measure (and would likely result in an exception one way or another in actual code).