Skip to content

Error inconsistency with exhaustively validating parameter types #2875

@jbafford

Description

@jbafford

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).

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions