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

Strict groups support #27

Merged
merged 2 commits into from
Jul 11, 2024
Merged

Conversation

Seldaek
Copy link
Member

@Seldaek Seldaek commented Jul 11, 2024

@staabm

I think these should be doable in a similar way you did in this PR

I am not sure how to do the non-null part. Maybe it's doable by modifying the result of $this->regexShapeMatcher->matchType() to ensure string|null becomes string.. but I suck at phpstan internals :D

If you have a sec to help any time it'd be great, I believe the tests are correct.

@Seldaek Seldaek force-pushed the strict-groups-support branch from 4b2e643 to 9918fa8 Compare July 11, 2024 13:32
@staabm
Copy link
Contributor

staabm commented Jul 11, 2024

wdyt about

$ git diff
diff --git a/src/PHPStan/PregMatchTypeSpecifyingExtension.php b/src/PHPStan/PregMatchTypeSpecifyingExtension.php
index b83ed12..7ee7ec5 100644
--- a/src/PHPStan/PregMatchTypeSpecifyingExtension.php
+++ b/src/PHPStan/PregMatchTypeSpecifyingExtension.php
@@ -11,8 +11,11 @@
 use PHPStan\Analyser\TypeSpecifierContext;
 use PHPStan\Reflection\MethodReflection;
 use PHPStan\TrinaryLogic;
+use PHPStan\Type\Constant\ConstantArrayType;
 use PHPStan\Type\Php\RegexArrayShapeMatcher;
 use PHPStan\Type\StaticMethodTypeSpecifyingExtension;
+use PHPStan\Type\TypeCombinator;
+use PHPStan\Type\Type;

 final class PregMatchTypeSpecifyingExtension implements StaticMethodTypeSpecifyingExtension, TypeSpecifierAwareExtension
 {
@@ -70,6 +73,22 @@ public function specifyTypes(MethodReflection $methodReflection, StaticCall $nod
             return new SpecifiedTypes();
         }

+        if (
+            in_array($methodReflection->getName(), ['matchStrictGroups', 'isMatchStrictGroups'], true)
+            && $matchedType instanceof ConstantArrayType
+        ) {
+            $matchedType = new ConstantArrayType(
+                $matchedType->getKeyTypes(),
+                array_map(static function (Type $valueType): Type {
+                    return TypeCombinator::removeNull($valueType);
+
+                }, $matchedType->getValueTypes()),
+                [0],
+                [],
+                $matchedType->isList()
+            );
+        }
+
         $overwrite = false;
         if ($context->false()) {
             $overwrite = true;

I am not 100% sure, I got the difference of 'matchStrictGroups' vs. 'isMatchStrictGroups'.

the above patch removes all nullable types and also turns all groups into mandatory groups.

@staabm
Copy link
Contributor

staabm commented Jul 11, 2024

I am not 100% sure, I got the difference of 'matchStrictGroups' vs. 'isMatchStrictGroups'.

I wonder whether this even means $matches can never be the empty-array?

@Seldaek
Copy link
Member Author

Seldaek commented Jul 11, 2024

Yeah that looks great thanks!

I wonder whether this even means $matches can never be the empty-array?

Hmm if it matches it cannot, but it is still allowed to NOT match. It only throws if it matches but some groups are null, thus improving the type safety within the code handling the matches as we know we don't have to check for nulls.. Now with better support for optional groups etc in PHPStan this whole thing might be less needed or not at all anymore tbh. I'll have to see.

@Seldaek Seldaek merged commit 59a4eec into composer:2.x Jul 11, 2024
11 of 12 checks passed
@Seldaek Seldaek deleted the strict-groups-support branch July 11, 2024 14:25
@Seldaek
Copy link
Member Author

Seldaek commented Jul 11, 2024

BTW there was a deprecation warning on $matchedType instanceof ConstantArrayType which I hacked around, possibly wrong :)

@Seldaek
Copy link
Member Author

Seldaek commented Jul 11, 2024

Woops, that does not go so well on the composer repo:

     Internal error: Internal error. while analysing file /var/www/composer/src/Composer/Repository/Vcs/GitHubDriver.php
     Post the following stack trace to https://github.com/phpstan/phpstan/issues/new?template=Bug_report.yaml:
     ## phar:///var/www/composer/vendor/phpstan/phpstan/phpstan.phar/src/Type/Constant/ConstantArrayTypeBuilder.php(150)
     #0 phar:///var/www/composer/vendor/phpstan/phpstan/phpstan.phar/src/Type/Constant/ConstantArrayType.php(576):
     PHPStan\Type\Constant\ConstantArrayTypeBuilder->setOffsetValueType()
     #1 phar:///var/www/composer/vendor/phpstan/phpstan/phpstan.phar/src/Type/TypeCombinator.php(915):
     PHPStan\Type\Constant\ConstantArrayType->setOffsetValueType()

@staabm
Copy link
Contributor

staabm commented Jul 11, 2024

how to reproduce?

@Seldaek
Copy link
Member Author

Seldaek commented Jul 11, 2024

in the composer project:

composer require composer/pcre:2.x-dev

And add - ../vendor/composer/pcre/extension.neon to phpstan/config.neon

Then composer phpstan crashes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants