Prevent ShouldNotHappenException in filter_* extensions #2951
Conversation
| @@ -240,7 +242,7 @@ private function getConstant(string $constantName): int | |||
| $constant = $this->reflectionProvider->getConstant(new Node\Name($constantName), null); | |||
| $valueType = $constant->getValueType(); | |||
| if (!$valueType instanceof ConstantIntegerType) { | |||
| throw new ShouldNotHappenException(sprintf('Constant %s does not have integer type.', $constantName)); | |||
| return self::UNKNOWN_CONSTANT; | |||
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. |
|
(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
| @@ -202,6 +202,8 @@ jobs: | |||
|
|
|||
| strategy: | |||
| matrix: | |||
| php-version: ["8.1"] | |||
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
| <?php | ||
|
|
||
| function doFoo(mixed $filter): void { | ||
| \PHPStan\Testing\assertType('non-falsy-string|false', filter_var("no", FILTER_VALIDATE_REGEXP)); |
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