diff --git a/composer.json b/composer.json index f81a2e4..857feae 100644 --- a/composer.json +++ b/composer.json @@ -21,11 +21,11 @@ }, "require-dev": { "phpunit/phpunit": "^8 || ^9", - "phpstan/phpstan": "^1.11.6", + "phpstan/phpstan": "^1.11.8@dev", "phpstan/phpstan-strict-rules": "^1.1" }, "conflict": { - "phpstan/phpstan": "<1.11.6" + "phpstan/phpstan": "<1.11.8" }, "autoload": { "psr-4": { diff --git a/extension.neon b/extension.neon index 282b8d4..2147431 100644 --- a/extension.neon +++ b/extension.neon @@ -8,9 +8,14 @@ conditionalTags: phpstan.staticMethodParameterOutTypeExtension: %featureToggles.narrowPregMatches% Composer\Pcre\PHPStan\PregMatchTypeSpecifyingExtension: phpstan.typeSpecifier.staticMethodTypeSpecifyingExtension: %featureToggles.narrowPregMatches% + Composer\Pcre\PHPStan\UnsafeStrictGroupsCallRule: + phpstan.rules.rule: %featureToggles.narrowPregMatches% services: - class: Composer\Pcre\PHPStan\PregMatchParameterOutTypeExtension - class: Composer\Pcre\PHPStan\PregMatchTypeSpecifyingExtension + +rules: + - Composer\Pcre\PHPStan\UnsafeStrictGroupsCallRule diff --git a/src/PHPStan/PregMatchFlags.php b/src/PHPStan/PregMatchFlags.php index 9cd3598..ace3e8a 100644 --- a/src/PHPStan/PregMatchFlags.php +++ b/src/PHPStan/PregMatchFlags.php @@ -7,13 +7,14 @@ use PHPStan\Type\TypeCombinator; use PHPStan\Type\Type; use PhpParser\Node\Arg; +use PHPStan\Type\Php\RegexArrayShapeMatcher; final class PregMatchFlags { static public function getType(?Arg $flagsArg, Scope $scope): ?Type { if ($flagsArg === null) { - return new ConstantIntegerType(PREG_UNMATCHED_AS_NULL); + return new ConstantIntegerType(PREG_UNMATCHED_AS_NULL | RegexArrayShapeMatcher::PREG_UNMATCHED_AS_NULL_ON_72_73); } $flagsType = $scope->getType($flagsArg->value); @@ -29,7 +30,7 @@ static public function getType(?Arg $flagsArg, Scope $scope): ?Type return null; } - $internalFlagsTypes[] = new ConstantIntegerType($constantScalarValue | PREG_UNMATCHED_AS_NULL); + $internalFlagsTypes[] = new ConstantIntegerType($constantScalarValue | PREG_UNMATCHED_AS_NULL | RegexArrayShapeMatcher::PREG_UNMATCHED_AS_NULL_ON_72_73); } return TypeCombinator::union(...$internalFlagsTypes); } diff --git a/src/PHPStan/UnsafeStrictGroupsCallRule.php b/src/PHPStan/UnsafeStrictGroupsCallRule.php new file mode 100644 index 0000000..5a1c492 --- /dev/null +++ b/src/PHPStan/UnsafeStrictGroupsCallRule.php @@ -0,0 +1,113 @@ + + */ +final class UnsafeStrictGroupsCallRule implements Rule +{ + /** + * @var RegexArrayShapeMatcher + */ + private $regexShapeMatcher; + + public function __construct(RegexArrayShapeMatcher $regexShapeMatcher) + { + $this->regexShapeMatcher = $regexShapeMatcher; + } + + public function getNodeType(): string + { + return StaticCall::class; + } + + public function processNode(Node $node, Scope $scope): array + { + if (!$node->class instanceof FullyQualified) { + return []; + } + $isRegex = $node->class->toString() === Regex::class; + $isPreg = $node->class->toString() === Preg::class; + if (!$isRegex && !$isPreg) { + return []; + } + if (!$node->name instanceof Node\Identifier || !in_array($node->name->name, ['matchStrictGroups', 'isMatchStrictGroups', 'matchAllStrictGroups', 'isMatchAllStrictGroups'], true)) { + return []; + } + + $args = $node->getArgs(); + if (!isset($args[0])) { + return []; + } + + $patternArg = $args[0] ?? null; + if ($isPreg) { + if (!isset($args[2])) { // no matches set, skip as the matches won't be used anyway + return []; + } + $flagsArg = $args[3] ?? null; + } else { + $flagsArg = $args[2] ?? null; + } + + if ($patternArg === null) { + return []; + } + + $flagsType = PregMatchFlags::getType($flagsArg, $scope); + if ($flagsType === null) { + return []; + } + $patternType = $scope->getType($patternArg->value); + + $matchedType = $this->regexShapeMatcher->matchType($patternType, $flagsType, TrinaryLogic::createYes()); + if ($matchedType === null) { + return [ + RuleErrorBuilder::message(sprintf('The %s call is potentially unsafe as $matches\' type could not be inferred.', $node->name->name)) + ->identifier('composerPcre.maybeUnsafeStrictGroups') + ->build(), + ]; + } + + if (count($matchedType->getConstantArrays()) === 1) { + $matchedType = $matchedType->getConstantArrays()[0]; + $nullableGroups = []; + foreach ($matchedType->getValueTypes() as $index => $type) { + if (TypeCombinator::containsNull($type)) { + $nullableGroups[] = $matchedType->getKeyTypes()[$index]->getValue(); + } + } + + if (\count($nullableGroups) > 0) { + return [ + RuleErrorBuilder::message(sprintf( + 'The %s call is unsafe as match group%s "%s" %s optional and may be null.', + $node->name->name, + \count($nullableGroups) > 1 ? 's' : '', + implode('", "', $nullableGroups), + \count($nullableGroups) > 1 ? 'are' : 'is' + ))->identifier('composerPcre.unsafeStrictGroups')->build(), + ]; + } + } + + return []; + } +} diff --git a/src/Regex.php b/src/Regex.php index 8c4158a..ed61f86 100644 --- a/src/Regex.php +++ b/src/Regex.php @@ -43,6 +43,7 @@ public static function match(string $pattern, string $subject, int $flags = 0, i */ public static function matchStrictGroups(string $pattern, string $subject, int $flags = 0, int $offset = 0): MatchStrictGroupsResult { + // @phpstan-ignore composerPcre.maybeUnsafeStrictGroups $count = Preg::matchStrictGroups($pattern, $subject, $matches, $flags, $offset); return new MatchStrictGroupsResult($count, $matches); @@ -87,6 +88,7 @@ public static function matchAllStrictGroups(string $pattern, string $subject, in self::checkOffsetCapture($flags, 'matchAllWithOffsets'); self::checkSetOrder($flags); + // @phpstan-ignore composerPcre.maybeUnsafeStrictGroups $count = Preg::matchAllStrictGroups($pattern, $subject, $matches, $flags, $offset); return new MatchAllStrictGroupsResult($count, $matches); diff --git a/tests/PHPStanTests/TypeInferenceTest.php b/tests/PHPStanTests/TypeInferenceTest.php index b669f0a..04ee357 100644 --- a/tests/PHPStanTests/TypeInferenceTest.php +++ b/tests/PHPStanTests/TypeInferenceTest.php @@ -17,8 +17,6 @@ * Run with "vendor/bin/phpunit --testsuite phpstan" * * This is excluded by default to avoid side effects with the library tests - * - * @group phpstan */ class TypeInferenceTest extends TypeInferenceTestCase { diff --git a/tests/PHPStanTests/UnsafeStrictGroupsCallRuleTest.php b/tests/PHPStanTests/UnsafeStrictGroupsCallRuleTest.php new file mode 100644 index 0000000..74eb9e0 --- /dev/null +++ b/tests/PHPStanTests/UnsafeStrictGroupsCallRuleTest.php @@ -0,0 +1,57 @@ + + * + * For the full copyright and license information, please view + * the LICENSE file that was distributed with this source code. + */ + +namespace Composer\Pcre\PHPStanTests; + +use PHPStan\Testing\RuleTestCase; +use Composer\Pcre\PHPStan\UnsafeStrictGroupsCallRule; +use PHPStan\Type\Php\RegexArrayShapeMatcher; + +/** + * Run with "vendor/bin/phpunit --testsuite phpstan" + * + * This is excluded by default to avoid side effects with the library tests + * + * @extends RuleTestCase + */ +class UnsafeStrictGruopsCallRuleTest extends RuleTestCase +{ + protected function getRule(): \PHPStan\Rules\Rule + { + return new UnsafeStrictGroupsCallRule(self::getContainer()->getByType(RegexArrayShapeMatcher::class)); + } + + public function testRule(): void + { + $this->analyse([__DIR__ . '/nsrt/preg-match.php'], [ + [ + 'The matchStrictGroups call is unsafe as match group "1" is optional and may be null.', + 80, + ], + [ + 'The matchAllStrictGroups call is unsafe as match groups "foo", "2" are optional and may be null.', + 82, + ], + [ + 'The isMatchStrictGroups call is potentially unsafe as $matches\' type could not be inferred.', + 86, + ], + ]); + } + + public static function getAdditionalConfigFiles(): array + { + return [ + 'phar://' . __DIR__ . '/../../vendor/phpstan/phpstan/phpstan.phar/conf/bleedingEdge.neon', + __DIR__ . '/../../extension.neon', + ]; + } +} diff --git a/tests/PHPStanTests/nsrt/preg-match.php b/tests/PHPStanTests/nsrt/preg-match.php index 7ffb2c2..5dd23db 100644 --- a/tests/PHPStanTests/nsrt/preg-match.php +++ b/tests/PHPStanTests/nsrt/preg-match.php @@ -3,6 +3,7 @@ namespace PregMatchShapes; use Composer\Pcre\Preg; +use Composer\Pcre\Regex; use function PHPStan\Testing\assertType; function doMatch(string $s): void @@ -15,25 +16,33 @@ function doMatch(string $s): void assertType('array{}|array{string}', $matches); if (Preg::match('/Price: (£|€)\d+/', $s, $matches)) { + assertType('array{string, string}', $matches); + } else { + assertType('array{}', $matches); + } + assertType('array{}|array{string, string}', $matches); + + if (Preg::match('/Price: (£|€)?\d+/', $s, $matches)) { assertType('array{string, string|null}', $matches); } else { assertType('array{}', $matches); } assertType('array{}|array{string, string|null}', $matches); - if (Preg::match('/Price: (£|€)?\d+/', $s, $matches)) { - assertType('array{0: string, 1?: string|null}', $matches); + // passing the PREG_UNMATCHED_AS_NULL should change nothing compared to above as it is always set + if (Preg::match('/Price: (£|€)?\d+/', $s, $matches, PREG_UNMATCHED_AS_NULL)) { + assertType('array{string, string|null}', $matches); } else { assertType('array{}', $matches); } - assertType('array{}|array{0: string, 1?: string|null}', $matches); + assertType('array{}|array{string, string|null}', $matches); if (Preg::isMatch('/Price: (?£|€)\d+/', $s, $matches)) { - assertType('array{0: string, currency: string|null, 1: string|null}', $matches); + assertType('array{0: string, currency: string, 1: string}', $matches); } else { assertType('array{}', $matches); } - assertType('array{}|array{0: string, currency: string|null, 1: string|null}', $matches); + assertType('array{}|array{0: string, currency: string, 1: string}', $matches); } function doMatchStrictGroups(string $s): void @@ -60,6 +69,25 @@ function doMatchStrictGroups(string $s): void assertType('array{}|array{0: string, test: string, 1: string}', $matches); } +function doMatchStrictGroupsUnsafe(string $s): void +{ + if (Preg::isMatchStrictGroups('{Configure Command(?: *| *=> *)(.*)(?:|$)}m', $s, $matches)) { + // does not error because the match group might be empty but is not optional + assertType('array{string, string}', $matches); + } + + // should error as it is unsafe due to the optional group 1 + Regex::matchStrictGroups('{Configure Command(?: *| *=> *)(.*)?(?:|$)}m', $s); + + if (Preg::matchAllStrictGroups('{((?.)?z)}m', $s, $matches)) { + // should error as it is unsafe due to the optional group foo/2 + } + + if (Preg::isMatchStrictGroups('{'.$s.'}', $s, $matches)) { + // should error as it is unsafe due not being introspectable with the dynamic string + } +} + // disabled until https://github.com/phpstan/phpstan-src/pull/3185 can be resolved // //function identicalMatch(string $s): void diff --git a/tests/PregTests/MatchAllTest.php b/tests/PregTests/MatchAllTest.php index 862d234..dada88d 100644 --- a/tests/PregTests/MatchAllTest.php +++ b/tests/PregTests/MatchAllTest.php @@ -44,7 +44,7 @@ public function testFailure(): void public function testSuccessStrictGroups(): void { - $count = Preg::matchAllStrictGroups('{(?P\d)(?a)?}', '3a', $matches); + $count = Preg::matchAllStrictGroups('{(?\d)(?a)}', '3a', $matches); self::assertSame(1, $count); self::assertSame(array(0 => ['3a'], 'm' => ['3'], 1 => ['3'], 'matched' => ['a'], 2 => ['a']), $matches); } @@ -52,8 +52,9 @@ public function testSuccessStrictGroups(): void public function testFailStrictGroups(): void { self::expectException(UnexpectedNullMatchException::class); - self::expectExceptionMessage('Pattern "{(?P\d)(?b)?}" had an unexpected unmatched group "unmatched", make sure the pattern always matches or use matchAll() instead.'); - Preg::matchAllStrictGroups('{(?P\d)(?b)?}', '123', $matches); + self::expectExceptionMessage('Pattern "{(?\d)(?b)?}" had an unexpected unmatched group "unmatched", make sure the pattern always matches or use matchAll() instead.'); + // @phpstan-ignore composerPcre.unsafeStrictGroups + Preg::matchAllStrictGroups('{(?\d)(?b)?}', '123', $matches); } public function testBadPatternThrowsIfWarningsAreNotThrowing(): void diff --git a/tests/PregTests/MatchTest.php b/tests/PregTests/MatchTest.php index 4038405..ae993d9 100644 --- a/tests/PregTests/MatchTest.php +++ b/tests/PregTests/MatchTest.php @@ -38,7 +38,7 @@ public function testSuccessWithInt(): void public function testSuccessStrictGroups(): void { - $count = Preg::matchStrictGroups('{(?P\d)(?a)?}', '3a', $matches); + $count = Preg::matchStrictGroups('{(?\d)(?a)}', '3a', $matches); self::assertSame(1, $count); self::assertSame(array(0 => '3a', 'm' => '3', 1 => '3', 'matched' => 'a', 2 => 'a'), $matches); } @@ -46,8 +46,9 @@ public function testSuccessStrictGroups(): void public function testFailStrictGroups(): void { self::expectException(UnexpectedNullMatchException::class); - self::expectExceptionMessage('Pattern "{(?P\d)(?b)?}" had an unexpected unmatched group "unmatched", make sure the pattern always matches or use match() instead.'); - Preg::matchStrictGroups('{(?P\d)(?b)?}', '123', $matches); + self::expectExceptionMessage('Pattern "{(?\d)(?b)?}" had an unexpected unmatched group "unmatched", make sure the pattern always matches or use match() instead.'); + // @phpstan-ignore composerPcre.unsafeStrictGroups + Preg::matchStrictGroups('{(?\d)(?b)?}', '123', $matches); } public function testTypeErrorWithNull(): void diff --git a/tests/RegexTests/MatchTest.php b/tests/RegexTests/MatchTest.php index 519e935..1dcdffe 100644 --- a/tests/RegexTests/MatchTest.php +++ b/tests/RegexTests/MatchTest.php @@ -40,15 +40,16 @@ public function testFailure(): void public function testSuccessStrictGroups(): void { - $result = Regex::matchStrictGroups('{(?P\d)(?a)?}', '3a'); + $result = Regex::matchStrictGroups('{(?\d)(?a)}', '3a'); self::assertSame(array(0 => '3a', 'm' => '3', 1 => '3', 'matched' => 'a', 2 => 'a'), $result->matches); } public function testFailStrictGroups(): void { self::expectException(UnexpectedNullMatchException::class); - self::expectExceptionMessage('Pattern "{(?P\d)(?b)?}" had an unexpected unmatched group "unmatched", make sure the pattern always matches or use match() instead.'); - Regex::matchStrictGroups('{(?P\d)(?b)?}', '123'); + self::expectExceptionMessage('Pattern "{(?\d)(?b)?}" had an unexpected unmatched group "unmatched", make sure the pattern always matches or use match() instead.'); + // @phpstan-ignore composerPcre.unsafeStrictGroups + Regex::matchStrictGroups('{(?\d)(?b)?}', '123'); } public function testBadPatternThrowsIfWarningsAreNotThrowing(): void