Implement array shapes for `preg_match()` `$matches` by-ref parameter #2589

staabm · 2023-08-26T18:55:11Z

I tried solving the shapes with Hoa Regex AST, but I couldn't find the information about capturing groups on the ast nodes.
I don't see a way on how to identify where capturing groups are based on the AST.

therefore I tried the way suggested in phpstan/phpstan#9502 (comment)

staabm · 2023-08-26T18:56:05Z

@mvorisek please provide feedback and if you could come up with a few more test-cases, that would be awesome.

whats the expected result for phpstan/phpstan#9502 ?

tests/PHPStan/Analyser/data/preg_match_shapes.php

src/Type/Php/PregMatchTypeSpecifyingExtension.php

staabm · 2023-08-27T09:38:49Z

memo to me: I need to fix PREG_UNMATCHED_AS_NULL on PHP <= 7.3
https://3v4l.org/3k0mr

PrinsFrank · 2024-05-14T19:44:48Z

@staabm It would be really great to have this implemented now reportPossiblyNonexistentConstantArrayOffset is available! Can I help here?

staabm · 2024-05-14T20:02:16Z

I have this PR on my todo list and will rebase it in the near future.

You can help by verifying existing test expectations of this PR or help explore missing tests/cases

src/Type/Php/RegexShapeMatcher.php

tests/PHPStan/Analyser/data/preg_match_shapes.php

src/Type/Php/RegexShapeMatcher.php

phpstan-bot · 2024-05-18T09:56:32Z

This pull request has been marked as ready for review.

staabm · 2024-05-18T10:00:00Z

In the end I combined both approaches. regex based retrieval of the capturing groups and the AST to identify how many non-optional groups are contained in the regex.

since the type inference now requires a regex which is parsable by Hoa\Regex I commented some type-assertions which currently don't work because the regex parser cannot parse it. I think these can be fixed in the future by applying fixes to the Grammar.pp.

I am neither a regex nor a grammer expert, therefore I left these things for other people more confident in this topic.
overall this means we get already a pretty good type narrowing based on a constant regex.

In a future PR I would improve the types to be even more precise for e.g.
'/Price: (£|€)/i' -> constant string £|€ instead of just string
'/Price: (\d)/i' -> numeric-string instead of string
'/Price: (\s){2,10}/i' -> non-falsey-string instead of string
...

src/Type/Php/RegexShapeMatcher.php

staabm · 2024-05-18T10:02:48Z

tests/PHPStan/Analyser/data/preg_match_shapes.php

+
+function doNamedSubpattern(string $s): void {
+	if (preg_match('/\w-(?P<num>\d+)-(\w)/', $s, $matches)) {
+		// could be assertType('array{0: string, num: string, 1: string, 2: string, 3: string}', $matches);


here we can see one example of a regex which is not parsable with the current hoa grammar:

Fatal error: Uncaught Hoa\Compiler\Llk\Parser::parse(): (0) Unexpected token "-" (range) at line 1 and column 4: /\w-(?P<num>\d+)-(\w)/ ↑ in /Users/staabm/workspace/phpstan-src/vendor/hoa/compiler/Llk/Parser.php at line 1. thrown in /Users/staabm/workspace/phpstan-src/vendor/hoa/compiler/Llk/Parser.php on line 1

There are two issues:

a) (?P<xxx>... is equivalent of supported (?<xxx>... - spec https://www.pcre.org/original/doc/html/pcrepattern.html (search for ˙?p<˙), grammar: https://github.com/hoaproject/Regex/blob/master/Source/Grammar.pp#L73
b) - range - it is reserved character strictly if in [ character group only (otherwise it is regular chanracter and very often used)

Thanks. Please provide code suggestions for fixes.

the example work in plain php, so we either need to fix the grammar or leave the typing for these cases behind

Would you mind forking hoaproject/Regex and saving there a php test file I can run with all the issues you found?

look into this PR here and find all commented assertions. these are the cases which hoa cannot parse.

I have spent the last the 3 days of freetime for your feature request. take your time to do the tests yourself. Thanks

ohh I see. I got the impression you are working on all these cases.

I can look into more cases later. This one was the most important. What about adding proper Hoa Regex unit testing to phpstan-src? We can assert the parsed regexes using https://github.com/mvorisek/Hoa-Regex/blob/0c8a5cbcf696df8f65168704af14ad5ebd48c641/test.php#L13-L25 function.

Sure, why not. Please send a PR to my branch/fork

I am not super familiar with phpstan-src codebase - where, what class can I base this unit test on?

I would create a new file in tests/PHPStan/Type. we can later on move it somewhere else if this doesn't work for ondrej.

staabm · 2024-05-18T10:28:40Z

patches/Grammar.patch

+-%token anchor                    \\(bBAZzG)|\^|\$
+%token anchor                    \\([bBAZzG])|\^|\$


unreleased hoa/regex upstream fix
hoaproject/Regex@5f670af

staabm · 2024-05-18T10:29:16Z

patches/Grammar.patch

+-    ( range() | literal() )+
+    ( <class_> | range() | literal() )+ <range>?


unreleased hoa/regex upstream fix
hoaproject/Regex@ce7fd7b
hoaproject/Regex@e770ada

2nd patch was reverted, see #2589 (comment)

staabm · 2024-05-18T10:30:34Z

patches/Grammar.patch

+-capturing:
+#capturing:


upstream hoa fix
hoaproject/Regex#38 (comment)

Seldaek · 2024-05-18T11:42:45Z

This looks amazing, thanks @staabm.

Now I'm just wondering how I can get it to recognize Composer\Pcre\Preg::match and co as well to get the same quality of type output there :s

If you can think of some way to make this more reusable by third party libs wrapping preg functions it'd be great.

staabm · 2024-05-18T11:52:37Z

Now I'm just wondering how I can get it to recognize Composer\Pcre\Preg::match and co as well to get the same quality of type output there :s

If you can think of some way to make this more reusable by third party libs wrapping preg functions it'd be great.

at first I want to see what @ondrejmirtes thinks about all this. he was not yet involved.

After that: I can think of different ways to make this re-usable. for wrapper-libs which are 100% api compatible with the pcre_* native functions we could e.g.

rewrite the AST at analysis time
create separate extensions which re-use the underlying implementation
make it configurable via NEON config
introduce a new phpdoc type
...

Seldaek · 2024-05-18T12:07:47Z

Ok cool thanks. Yes composer/pcre is almost one to one.. Except it throws instead of returning false. For the other api returning object results there it's a bit more complex but anyway I see your point and I'll let you wrap things up here first. Just wanted to let you know in case it can influence design decisions along the way but it's not urgent at all.

tests/PHPStan/Analyser/data/preg_match_shapes.php

staabm · 2024-06-21T07:53:56Z

I am not sure why we have these 7.4/7.2 remaining errors. are you fine with merging anyway?

staabm · 2024-06-21T08:12:10Z

I am not sure why we have these 7.4/7.2 remaining errors. are you fine with merging anyway?

made the expectations more php version specific so we can move on and get a green build.

we see some PHPStan extension builds failing because of the problem mentioned in #2589 (comment)

ondrejmirtes · 2024-06-21T08:20:40Z

What about the errors here? https://github.com/phpstan/phpstan-src/actions/runs/9610380632

Like: https://github.com/phpstan/phpstan-symfony/blob/bca27f1701fc1a297749e6c2a1e3da4462c1a6af/src/Type/Symfony/ParameterDynamicReturnTypeExtension.php#L185-L188

I know you mentioned === 1 not being able to handle that in the type-specifying extension.

I don't know whether you solved that, based on the results probably not. My opinion is that we could hardcode this specifically in TypeSpecifier, so that extension is called correctly with the right context.

staabm · 2024-06-21T08:29:09Z

I don't know whether you solved that, based on the results probably not. My opinion is that we could hardcode this specifically in TypeSpecifier, so that extension is called correctly with the right context.

if you have an idea how this need to look like, please go ahead and implement it.

the only idea I have atm would be to add a AST visitor which detects a if (preg_match(...) === 1) and sets a attribute which later on can be detected by the TypeSpecifier.

hopefully you have something better in mind :)

mvorisek · 2024-06-21T08:34:41Z

What exactly is the problem, why we cannot check if the context/preg_match result is surely non-falsy?

…ch()`

ondrejmirtes · 2024-06-21T09:35:36Z

@staabm I meant this: 328fcb9

staabm · 2024-06-21T09:40:48Z

I see, thanks. should we have the same for ==?

ondrejmirtes · 2024-06-21T09:41:35Z

@staabm Yeah, it's probably harmless, but do that in a next PR, not here :)

ondrejmirtes · 2024-06-21T09:47:22Z

Thank you!

ondrejmirtes · 2024-06-21T11:22:24Z

An extension can be as simple as this phpstan/phpstan-nette@3e68a5d

staabm · 2024-06-21T11:25:57Z

the extension for composer/pcre is beeing worked on in composer/pcre#24

westonruter · 2024-07-01T17:48:54Z

I just noticed that this doesn't seem to play well with strict rules.

This example without strict rules works: https://phpstan.org/r/6187bcca-8fc8-459b-9790-21a82e62b345

But when I enable strict rules, I need to cast the return value of preg_match() to a bool and this seems to block the types from passing through. For example: https://phpstan.org/r/f368fc07-6438-43f1-b80b-93dee22535f4

Same thing happens when I cast to an int: https://phpstan.org/r/0d011355-4446-4bd8-9b8d-0c44907f9a07

ondrejmirtes · 2024-07-01T17:51:32Z

@westonruter Please open a new bug report about this.

westonruter · 2024-07-01T17:54:27Z

Done: phpstan/phpstan#11262

staabm · 2024-07-05T06:55:45Z

extensions implemented for PHP-CS-Fixer Preg::match in PHP-CS-Fixer/PHP-CS-Fixer#8103

staabm · 2024-07-11T12:51:52Z

initial support for composer/pcre 2.x landed in composer/pcre#25
coverage for more methods is beeing worked on in separate PRs

staabm force-pushed the regexshapes branch from 081ad30 to 78b1776 Compare August 26, 2023 19:09

staabm commented Aug 26, 2023

View reviewed changes

tests/PHPStan/Analyser/data/preg_match_shapes.php Outdated Show resolved Hide resolved

mvorisek reviewed Aug 26, 2023

View reviewed changes

src/Type/Php/PregMatchTypeSpecifyingExtension.php Outdated Show resolved Hide resolved

gharlan reviewed May 14, 2024

View reviewed changes

src/Type/Php/RegexShapeMatcher.php Outdated Show resolved Hide resolved

staabm force-pushed the regexshapes branch 2 times, most recently from c08a944 to 0ce2624 Compare May 15, 2024 17:20

staabm changed the base branch from 1.10.x to 1.11.x May 15, 2024 17:20

PrinsFrank reviewed May 16, 2024

View reviewed changes

tests/PHPStan/Analyser/data/preg_match_shapes.php Outdated Show resolved Hide resolved

PrinsFrank reviewed May 16, 2024

View reviewed changes

tests/PHPStan/Analyser/data/preg_match_shapes.php Outdated Show resolved Hide resolved

PrinsFrank reviewed May 16, 2024

View reviewed changes

tests/PHPStan/Analyser/data/preg_match_shapes.php Outdated Show resolved Hide resolved

PrinsFrank suggested changes May 16, 2024

View reviewed changes

tests/PHPStan/Analyser/data/preg_match_shapes.php Outdated Show resolved Hide resolved

staabm commented May 16, 2024

View reviewed changes

src/Type/Php/RegexShapeMatcher.php Outdated Show resolved Hide resolved

staabm force-pushed the regexshapes branch 3 times, most recently from 39d2b3b to 7ed47c4 Compare May 18, 2024 09:51

staabm marked this pull request as ready for review May 18, 2024 09:56

staabm commented May 18, 2024

View reviewed changes

patches/Grammar.patch

Comment on lines +25 to +26

-capturing:

+#capturing:

Copy link

Contributor Author

staabm May 18, 2024

Choose a reason for hiding this comment

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

upstream hoa fix
hoaproject/Regex#38 (comment)

mvorisek reviewed May 18, 2024

View reviewed changes

tests/PHPStan/Analyser/data/preg_match_shapes.php Outdated Show resolved Hide resolved

staabm added 3 commits June 21, 2024 09:59

move test into separate file

d74d107

turn test into a php 7.3 only test

1ccfcfe

Update preg_match_shapes_php73.php

71f4223

TypeSpecifier - understand preg_match() === 1 same way as `preg_mat…

328fcb9

…ch()`

PHPStan fix

cf3fc13

ondrejmirtes changed the title ~~Implement array shapes for preg_match() $matches~~ Implement array shapes for preg_match() $matches by-ref parameter Jun 21, 2024

ondrejmirtes approved these changes Jun 21, 2024

View reviewed changes

ondrejmirtes merged commit 721a0a6 into phpstan:1.11.x Jun 21, 2024
447 of 452 checks passed

staabm deleted the regexshapes branch June 21, 2024 09:47

This was referenced Jun 21, 2024

TypeSpecifier - understand preg_match() == 1 same way as === 1 #3175

Merged

more precise preg_match types phpstan/phpstan#11222

Closed

Implement array shapes for Preg::match $matches by-ref parameter composer/pcre#24

Closed

westonruter mentioned this pull request Jul 1, 2024

Casting preg_match() return value breaks array shapes for $matches by-ref parameter phpstan/phpstan#11262

Closed

mvorisek mentioned this pull request Jul 11, 2024

preg_match missing array shape for pattern phpstan/phpstan#11323

Closed

		-%token anchor \\(bBAZzG)\|\^\|\$
		+%token anchor \\([bBAZzG])\|\^\|\$

		- ( range() \| literal() )+
		+ ( <class_> \| range() \| literal() )+ <range>?

Implement array shapes for preg_match() $matches by-ref parameter #2589

Implement array shapes for preg_match() $matches by-ref parameter #2589

Conversation

staabm commented Aug 26, 2023 • edited Loading

staabm commented Aug 26, 2023

staabm commented Aug 27, 2023

PrinsFrank commented May 14, 2024

staabm commented May 14, 2024

phpstan-bot commented May 18, 2024

staabm commented May 18, 2024 • edited Loading

staabm May 18, 2024

Choose a reason for hiding this comment

mvorisek May 18, 2024

Choose a reason for hiding this comment

staabm May 18, 2024 • edited Loading

Choose a reason for hiding this comment

mvorisek May 18, 2024

Choose a reason for hiding this comment

staabm May 18, 2024 • edited Loading

Choose a reason for hiding this comment

staabm May 19, 2024

Choose a reason for hiding this comment

mvorisek May 19, 2024

Choose a reason for hiding this comment

staabm May 19, 2024 • edited Loading

Choose a reason for hiding this comment

mvorisek May 19, 2024

Choose a reason for hiding this comment

staabm May 19, 2024 • edited Loading

Choose a reason for hiding this comment

staabm May 18, 2024 • edited Loading

Choose a reason for hiding this comment

staabm May 18, 2024 • edited Loading

Choose a reason for hiding this comment

mvorisek May 18, 2024 • edited Loading

Choose a reason for hiding this comment

staabm May 18, 2024

Choose a reason for hiding this comment

Seldaek commented May 18, 2024

staabm commented May 18, 2024 • edited Loading

Seldaek commented May 18, 2024

staabm commented Jun 21, 2024 • edited Loading

staabm commented Jun 21, 2024 • edited Loading

ondrejmirtes commented Jun 21, 2024

staabm commented Jun 21, 2024 • edited Loading

mvorisek commented Jun 21, 2024

ondrejmirtes commented Jun 21, 2024

staabm commented Jun 21, 2024

ondrejmirtes commented Jun 21, 2024

ondrejmirtes commented Jun 21, 2024

ondrejmirtes commented Jun 21, 2024

staabm commented Jun 21, 2024 • edited Loading

westonruter commented Jul 1, 2024

ondrejmirtes commented Jul 1, 2024

westonruter commented Jul 1, 2024

staabm commented Jul 5, 2024

staabm commented Jul 11, 2024 • edited Loading

Implement array shapes for `preg_match()` `$matches` by-ref parameter #2589

Implement array shapes for `preg_match()` `$matches` by-ref parameter #2589

staabm commented Aug 26, 2023 •

edited

Loading

staabm commented May 18, 2024 •

edited

Loading

staabm May 18, 2024 •

edited

Loading

staabm May 18, 2024 •

edited

Loading

staabm May 19, 2024 •

edited

Loading

staabm May 19, 2024 •

edited

Loading

staabm May 18, 2024 •

edited

Loading

staabm May 18, 2024 •

edited

Loading

mvorisek May 18, 2024 •

edited

Loading

staabm commented May 18, 2024 •

edited

Loading

staabm commented Jun 21, 2024 •

edited

Loading

staabm commented Jun 21, 2024 •

edited

Loading

staabm commented Jun 21, 2024 •

edited

Loading

staabm commented Jun 21, 2024 •

edited

Loading

staabm commented Jul 11, 2024 •

edited

Loading