Trigger error on unsafe StrictGroups usages (#29)

Also fix issues on php 7.2/7.3

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": {

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

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);
     }

src/PHPStan/UnsafeStrictGroupsCallRule.php

@@ -0,0 +1,113 @@
+<?php declare(strict_types=1);
+
+namespace Composer\Pcre\PHPStan;
+
+use Composer\Pcre\Preg;
+use Composer\Pcre\Regex;
+use PhpParser\Node;
+use PhpParser\Node\Expr\StaticCall;
+use PhpParser\Node\Name\FullyQualified;
+use PHPStan\Analyser\Scope;
+use PHPStan\Analyser\SpecifiedTypes;
+use PHPStan\Rules\Rule;
+use PHPStan\Rules\RuleErrorBuilder;
+use PHPStan\TrinaryLogic;
+use PHPStan\Type\ObjectType;
+use PHPStan\Type\Type;
+use PHPStan\Type\TypeCombinator;
+use PHPStan\Type\Php\RegexArrayShapeMatcher;
+use function sprintf;
+
+/**
+ * @implements Rule<StaticCall>
+ */
+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 [];
+    }
+}

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

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
 {

tests/PHPStanTests/UnsafeStrictGroupsCallRuleTest.php

@@ -0,0 +1,57 @@
+<?php
+
+/*
+ * This file is part of composer/pcre.
+ *
+ * (c) Composer <https://github.com/composer>
+ *
+ * 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<UnsafeStrictGroupsCallRule>
+ */
+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',
+        ];
+    }
+}

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: (?<currency>£|€)\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(?: *</td><td class="v">| *=> *)(.*)(?:</td>|$)}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(?: *</td><td class="v">| *=> *)(.*)?(?:</td>|$)}m', $s);
+
+    if (Preg::matchAllStrictGroups('{((?<foo>.)?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

tests/PregTests/MatchAllTest.php

@@ -44,16 +44,17 @@ public function testFailure(): void
 
     public function testSuccessStrictGroups(): void
     {
-        $count = Preg::matchAllStrictGroups('{(?P<m>\d)(?<matched>a)?}', '3a', $matches);
+        $count = Preg::matchAllStrictGroups('{(?<m>\d)(?<matched>a)}', '3a', $matches);
         self::assertSame(1, $count);
         self::assertSame(array(0 => ['3a'], 'm' => ['3'], 1 => ['3'], 'matched' => ['a'], 2 => ['a']), $matches);
     }
 
     public function testFailStrictGroups(): void
     {
         self::expectException(UnexpectedNullMatchException::class);
-        self::expectExceptionMessage('Pattern "{(?P<m>\d)(?<unmatched>b)?}" had an unexpected unmatched group "unmatched", make sure the pattern always matches or use matchAll() instead.');
-        Preg::matchAllStrictGroups('{(?P<m>\d)(?<unmatched>b)?}', '123', $matches);
+        self::expectExceptionMessage('Pattern "{(?<m>\d)(?<unmatched>b)?}" had an unexpected unmatched group "unmatched", make sure the pattern always matches or use matchAll() instead.');
+        // @phpstan-ignore composerPcre.unsafeStrictGroups
+        Preg::matchAllStrictGroups('{(?<m>\d)(?<unmatched>b)?}', '123', $matches);
     }
 
     public function testBadPatternThrowsIfWarningsAreNotThrowing(): void

tests/PregTests/MatchTest.php

@@ -38,16 +38,17 @@ public function testSuccessWithInt(): void
 
     public function testSuccessStrictGroups(): void
     {
-        $count = Preg::matchStrictGroups('{(?P<m>\d)(?<matched>a)?}', '3a', $matches);
+        $count = Preg::matchStrictGroups('{(?<m>\d)(?<matched>a)}', '3a', $matches);
         self::assertSame(1, $count);
         self::assertSame(array(0 => '3a', 'm' => '3', 1 => '3', 'matched' => 'a', 2 => 'a'), $matches);
     }
 
     public function testFailStrictGroups(): void
     {
         self::expectException(UnexpectedNullMatchException::class);
-        self::expectExceptionMessage('Pattern "{(?P<m>\d)(?<unmatched>b)?}" had an unexpected unmatched group "unmatched", make sure the pattern always matches or use match() instead.');
-        Preg::matchStrictGroups('{(?P<m>\d)(?<unmatched>b)?}', '123', $matches);
+        self::expectExceptionMessage('Pattern "{(?<m>\d)(?<unmatched>b)?}" had an unexpected unmatched group "unmatched", make sure the pattern always matches or use match() instead.');
+        // @phpstan-ignore composerPcre.unsafeStrictGroups
+        Preg::matchStrictGroups('{(?<m>\d)(?<unmatched>b)?}', '123', $matches);
     }
 
     public function testTypeErrorWithNull(): void

tests/RegexTests/MatchTest.php

@@ -40,15 +40,16 @@ public function testFailure(): void
 
     public function testSuccessStrictGroups(): void
     {
-        $result = Regex::matchStrictGroups('{(?P<m>\d)(?<matched>a)?}', '3a');
+        $result = Regex::matchStrictGroups('{(?<m>\d)(?<matched>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<m>\d)(?<unmatched>b)?}" had an unexpected unmatched group "unmatched", make sure the pattern always matches or use match() instead.');
-        Regex::matchStrictGroups('{(?P<m>\d)(?<unmatched>b)?}', '123');
+        self::expectExceptionMessage('Pattern "{(?<m>\d)(?<unmatched>b)?}" had an unexpected unmatched group "unmatched", make sure the pattern always matches or use match() instead.');
+        // @phpstan-ignore composerPcre.unsafeStrictGroups
+        Regex::matchStrictGroups('{(?<m>\d)(?<unmatched>b)?}', '123');
     }
 
     public function testBadPatternThrowsIfWarningsAreNotThrowing(): void