Implement array shapes for preg_match() $matches #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

mvorisek · 2023-08-26T21:53:46Z

src/Type/Php/PregMatchTypeSpecifyingExtension.php

+
+	public function isFunctionSupported(FunctionReflection $functionReflection, FuncCall $node, TypeSpecifierContext $context): bool
+	{
+		return in_array(strtolower($functionReflection->getName()), ['preg_match'], true);


preg_match_all should be supported as well

also handle/test all relevant preg_ $flags params like PREG_OFFSET_CAPTURE, PREG_SET_ORDER, ... which have substantial impact on the result

lets do preg_match first and see what ondrey is thinking about it

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

fix n modifier support PREG_OFFSET_CAPTURE cs separated 8.2 tests support PREG_OFFSET_CAPTURE|PREG_UNMATCHED_AS_NULL fix

Co-authored-by: Gregor Harlan <mail@gh01.de>

Co-Authored-By: PrinsFrank <25006490+PrinsFrank@users.noreply.github.com>

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.

mvorisek · 2024-05-18T12:40:13Z

tests/PHPStan/Analyser/data/preg_match_shapes.php

+	}
+	assertType('array<string>', $matches);


Suggested change

}

assertType('array<string>', $matches);

} else {

assertType('array{}', $matches);

}

assertType('array{string}', $matches);

when pattern is not matched, the $matches is reset to array{} - https://3v4l.org/NE0bm

src/Type/Php/RegexShapeMatcher.php

ondrejmirtes · 2024-05-18T15:33:13Z

Some initial thoughts:

This should be only for bleeding edge. There is a big potential for bugs so we should limit the impact.
There are more preg_ functions that would benefit from the implemented logic, right?
- There's some regex syntax (\Q...\E) the Hoa library fails spectacularly with. We should test it in this PR: Invalid entry in ignoreErrors: While \Q...\E in effect.: "This leads to unintended behavior. Use '\$' instead." phpstan#10855 (comment)
I don't like bending TypeSpecifyingExtension for this. This should either be hardcoded in NodeScopeResolver, or there should be some kind of new ParameterOutTypeExtension. Probably the latter, given we want to make this accessible to userland code.
There are different flavours of custom PCRE wrappers. Some return matches by-ref as the original functions, some return the matches normally via returned value. The list of our options in Implement array shapes for preg_match() $matches #2589 (comment) is comprehensive, I like "create separate extensions which re-use the underlying implementation" the most. This means that the actual implementation should be in an extension-agnostic service that other extensions can simply inject. This means the service has to be marked with @api.

mvorisek · 2024-05-18T15:59:02Z

src/Type/Php/RegexShapeMatcher.php

+
+		$remainingNonOptionalGroupCount = $this->countNonOptionalGroups($regex);
+		if ($remainingNonOptionalGroupCount === null) {
+			// regex could not be parsed by Hoa/Regex


I would prefer emitting a PHPStan error with the parse exception message - the PCRE grammar is actually not so complex, so we can fix the Hoa grammar based on issue reports.

This would effectively also validate constant regexps for syntax errors.

The https://3v4l.org/sOXbn parsing hack can be then removed in favor of Hoa parser.

ondrejmirtes · 2024-05-19T07:49:34Z

An idea: send a separate PR with DynamicParameterOutType extensions, and once that is merged, you can base this PR on that.

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

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 Show resolved Hide resolved

PrinsFrank reviewed May 16, 2024

View reviewed changes

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

PrinsFrank reviewed May 16, 2024

View reviewed changes

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

PrinsFrank suggested changes May 16, 2024

View reviewed changes

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

staabm commented May 16, 2024

View reviewed changes

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

staabm and others added 14 commits May 18, 2024 10:52

Implement array shapes for preg_match()

9be42aa

fix n modifier support PREG_OFFSET_CAPTURE cs separated 8.2 tests support PREG_OFFSET_CAPTURE|PREG_UNMATCHED_AS_NULL fix

added tests

e9f260a

fix regex pattern

fdb23f2

Co-authored-by: Gregor Harlan <mail@gh01.de>

fix group name

831cf7e

Fix PHP < 7.4

b780357

fix tests per php version

84f9f12

add more tests

c213e05

Co-Authored-By: PrinsFrank <25006490+PrinsFrank@users.noreply.github.com>

added more tests

6b81977

Co-Authored-By: PrinsFrank <25006490+PrinsFrank@users.noreply.github.com>

moved php 8.2 only test

94f06dd

patch Grammer.pp to support capturing groups

c2ea821

ast

1d93604

fix

03ddeee

fix

c3f4442

fix

7289c72

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

staabm commented May 18, 2024

View reviewed changes

Fix hoa/regex bug31

7bb762b

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)

staabm added 3 commits May 18, 2024 13:58

read the grammar only once

e545895

simplify

028e78e

remove outdated comment

f073f13

staabm added 2 commits May 18, 2024 14:10

fix test typo

228bdfe

test more quantifiers

47982b5

mvorisek reviewed May 18, 2024

View reviewed changes

staabm commented May 18, 2024

View reviewed changes

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

mvorisek reviewed May 18, 2024

View reviewed changes

staabm added 4 commits May 19, 2024 09:03

test hoa/regex unsupported regex syntax

2b5e74d

enable only in bleeding edge

a46e9ee

use nette/utils

58e1882

refactor into service

967af26

staabm marked this pull request as draft May 19, 2024 07:51

fix dash outside of capturing group

dcd09ca

staabm mentioned this pull request May 19, 2024

Implement ParameterOutTypeExtensions #3083

Open

staabm added 2 commits May 21, 2024 12:14

decouple RegexArrayShapeMatcher from TypeSpecifierContext

083e983

use separate feature flag

dbedf9e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement array shapes for preg_match() $matches #2589

Implement array shapes for preg_match() $matches #2589

staabm commented Aug 26, 2023 •

edited

staabm commented Aug 26, 2023

mvorisek Aug 26, 2023 •

edited

staabm Aug 27, 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

staabm May 18, 2024

mvorisek May 18, 2024

staabm May 18, 2024 •

edited

mvorisek May 18, 2024

staabm May 18, 2024 •

edited

staabm May 19, 2024

mvorisek May 19, 2024

staabm May 19, 2024 •

edited

mvorisek May 19, 2024

staabm May 19, 2024 •

edited

staabm May 18, 2024 •

edited

staabm May 18, 2024 •

edited

mvorisek May 18, 2024 •

edited

staabm May 18, 2024

Seldaek commented May 18, 2024

staabm commented May 18, 2024 •

edited

Seldaek commented May 18, 2024

mvorisek May 18, 2024

ondrejmirtes commented May 18, 2024

mvorisek May 18, 2024

ondrejmirtes commented May 19, 2024

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

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

-	}
-	assertType('array<string>', $matches);
+	} else {
+		assertType('array{}', $matches);
+	}
+	assertType('array{string}', $matches);

Implement array shapes for preg_match() $matches #2589

Are you sure you want to change the base?

Implement array shapes for preg_match() $matches #2589

Conversation

staabm commented Aug 26, 2023 • edited

staabm commented Aug 26, 2023

mvorisek Aug 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm May 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm May 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm May 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staabm May 19, 2024 • edited

Choose a reason for hiding this comment

staabm May 18, 2024 • edited

Choose a reason for hiding this comment

staabm May 18, 2024 • edited

Choose a reason for hiding this comment

mvorisek May 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Seldaek commented May 18, 2024

staabm commented May 18, 2024 • edited

Seldaek commented May 18, 2024

Choose a reason for hiding this comment

ondrejmirtes commented May 18, 2024

Choose a reason for hiding this comment

ondrejmirtes commented May 19, 2024

staabm commented Aug 26, 2023 •

edited

mvorisek Aug 26, 2023 •

edited

staabm commented May 18, 2024 •

edited

staabm May 18, 2024 •

edited

staabm May 18, 2024 •

edited

staabm May 19, 2024 •

edited

staabm May 19, 2024 •

edited

staabm May 18, 2024 •

edited

staabm May 18, 2024 •

edited

mvorisek May 18, 2024 •

edited

staabm commented May 18, 2024 •

edited