Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent ShouldNotHappenException in filter_* extensions #2951

Open
wants to merge 14 commits into
base: 1.10.x
Choose a base branch
from
20 changes: 19 additions & 1 deletion .github/workflows/e2e-tests.yml
Expand Up @@ -206,26 +206,39 @@ jobs:
- script: "bin/phpstan analyse -l 8 -a tests/e2e/data/timecop.php -c tests/e2e/data/empty.neon tests/e2e/data/timecop.php"
tools: "pecl"
extensions: "timecop-beta"
php-version: "8.1"
- script: "bin/phpstan analyse -l 8 -a tests/e2e/data/soap.php -c tests/e2e/data/empty.neon tests/e2e/data/soap.php"
extensions: "soap"
php-version: "8.1"
- script: "bin/phpstan analyse -l 8 -a tests/e2e/data/soap.php -c tests/e2e/data/empty.neon tests/e2e/data/soap.php"
extensions: ""
php-version: "8.1"
- script: "bin/phpstan analyse -l 8 tests/e2e/anon-class/Granularity.php"
extensions: ""
php-version: "8.1"
- script: "bin/phpstan analyse -l 8 e2e/phpstan-phpunit-190/test.php -c e2e/phpstan-phpunit-190/test.neon"
extensions: ""
php-version: "8.1"
- script: "bin/phpstan analyse e2e/only-files-not-analysed-trait/src -c e2e/only-files-not-analysed-trait/ignore.neon"
extensions: ""
php-version: "8.1"
- script: "bin/phpstan analyse e2e/only-files-not-analysed-trait/src/Foo.php e2e/only-files-not-analysed-trait/src/BarTrait.php -c e2e/only-files-not-analysed-trait/no-ignore.neon"
extensions: ""
php-version: "8.1"
- script: |
cd e2e/baseline-uninit-prop-trait
../../bin/phpstan analyse --debug --configuration test-no-baseline.neon --generate-baseline test-baseline.neon
../../bin/phpstan analyse --debug --configuration test.neon
php-version: "8.1"
- script: |
cd e2e/baseline-uninit-prop-trait
../../bin/phpstan analyse --configuration test-no-baseline.neon --generate-baseline test-baseline.neon
../../bin/phpstan analyse --configuration test.neon
php-version: "8.1"
- script: |
cd e2e/bug10483
../../bin/phpstan -vvv
php-version: "7.2"

steps:
- name: "Checkout"
Expand All @@ -235,12 +248,17 @@ jobs:
uses: "shivammathur/setup-php@v2"
with:
coverage: "none"
php-version: "8.1"
php-version: ${{ matrix.php-version }}
tools: ${{ matrix.tools }}
extensions: ${{ matrix.extensions }}

- name: "Install dependencies"
run: "composer install --no-interaction --no-progress"

- name: "Transform source code"
if: matrix.php-version != '8.1' && matrix.php-version != '8.2' && matrix.php-version != '8.3'
shell: bash
run: "vendor/bin/simple-downgrade downgrade -c build/downgrade.php ${{ matrix.php-version }}"

- name: "Test"
run: ${{ matrix.script }}
5 changes: 5 additions & 0 deletions e2e/bug10483/bootstrap.php
@@ -0,0 +1,5 @@
<?php

// constant that's used in the Filter extension that was introduced in a later version of PHP.
// on earlier php version introduce the same constant via a bootstrap file but with a wrong type
if(!defined("FILTER_SANITIZE_ADD_SLASHES"))define("FILTER_SANITIZE_ADD_SLASHES",false);
7 changes: 7 additions & 0 deletions e2e/bug10483/phpstan.dist.neon
@@ -0,0 +1,7 @@
parameters:
level: 9
paths:
- src

bootstrapFiles:
- bootstrap.php
5 changes: 5 additions & 0 deletions e2e/bug10483/src/bug10483.php
@@ -0,0 +1,5 @@
<?php

function doFoo(mixed $filter): void {
\PHPStan\Testing\assertType('non-falsy-string|false', filter_var("no", FILTER_VALIDATE_REGEXP));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
6 changes: 3 additions & 3 deletions src/Type/Php/FilterFunctionReturnTypeHelper.php
Expand Up @@ -5,7 +5,6 @@
use PhpParser\Node;
use PHPStan\Php\PhpVersion;
use PHPStan\Reflection\ReflectionProvider;
use PHPStan\ShouldNotHappenException;
use PHPStan\TrinaryLogic;
use PHPStan\Type\Accessory\AccessoryNonEmptyStringType;
use PHPStan\Type\Accessory\AccessoryNonFalsyStringType;
Expand All @@ -31,7 +30,6 @@
use function is_int;
use function octdec;
use function preg_match;
use function sprintf;
use const PHP_FLOAT_EPSILON;

final class FilterFunctionReturnTypeHelper
Expand All @@ -40,6 +38,8 @@ final class FilterFunctionReturnTypeHelper
/** All validation filters match 0x100. */
private const VALIDATION_FILTER_BITMASK = 0x100;

private const UNKNOWN_CONSTANT = -9999999;

private ConstantStringType $flagsString;

/** @var array<int, Type>|null */
Expand Down Expand Up @@ -240,7 +240,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;
Copy link
Contributor Author

@staabm staabm Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return $valueType->getValue();
Expand Down