Prevent ShouldNotHappenException in filter_* extensions#2951
Prevent ShouldNotHappenException in filter_* extensions#2951staabm wants to merge 14 commits intophpstan:1.10.xfrom
Conversation
There was a problem hiding this comment.
I thought about returning null, or catching this exception (or a new exception) at the call-sites, but this seemed too much for this case. instead I decided to return a negative constant value, which does not overlap with other existing FILTER_* constants
|
This pull request has been marked as ready for review. |
ondrejmirtes
left a comment
There was a problem hiding this comment.
What's the effect of this on the type inference? Please include the failing test here in this PR too. So we can build on it and verify the actual returned type by this extension.
|
(I need to verify on a different computer with PHP 7.2 installed whether the test is working as intended) |
|
This pull request has been marked as ready for review. |
.github/workflows/e2e-tests.yml
Outdated
There was a problem hiding this comment.
This doesn't do what you think it does. See: https://github.com/phpstan/phpstan-src/actions/runs/8159444153/job/22303754862 - you basically disabled all of the tests.
Normal run: https://github.com/phpstan/phpstan-src/actions/runs/8170798487
There was a problem hiding this comment.
good catch. fixed it.
thank you
e2e/bug10483/src/bug10483.php
Outdated
There was a problem hiding this comment.
Alright so I found out that the usual result on PHP versions where this constant exists is the same as here. Which makes me a bit suspicious. Can you explain that? We surely must be losing information when we return the unknown constant somewhere, right?
|
Alright, and now add the same test to the usual NodeScopeResolverTest, so that the difference is obvious 😊 You'll most likely need different files for different PHP versions? |
|
reverted the previous test commit. I thnk there is no type inference difference to observe, because phpstan internally works with the same "wrong" constant value as the code beeing analyzed (because the constant is defined in a bootstrap.php script, therefore mangels the global scope of the whole process). the only thing we need to prevent is the crash |
|
I got one idea for which I am not sure whether its worth testing. the type inference might be different when we define the constant in bootstrap with a integer value of another existing FILTER_ constant. that way we would trigger an overlap of 2 constants. but thats not something the initial case we try to fix triggers |
This reverts commit 84de37845f8d4e4e907518a078786202c5f2763c.
This reverts commit 42307ceab4c12bf1b0c83f8b13be26ea7951b565.
test in phpstan/phpstan#10661
closes phpstan/phpstan#10483