@mvorisek could you have a look?

Note that both (?P<groupname>pattern) and without P (?<groupname>pattern) are both supported https://3v4l.org/qJI26

@mvorisek could you have a look?

I can help with grammar, but my time is very limited and creating unit tests would help me - phpstan/phpstan-src#2589 (comment)

@mvorisek put the contents of https://phpstan.org/r/3a6f6eef-9ecc-4304-b707-b9a688d7f48c
into a file, e.g. test.php in the root of the phpstan-src repo and invoke php bin/phpstan analyze test.php --debug --xdebug
and put a breakpoint into RegexArrayShapeMatcher->parseGroups

I can build a proper unit test when the fix is made

I will look into the hoa regex grammar fixes.

Is (?P currently the only known unsupported grammar?

Seems like without P it works yes https://phpstan.org/r/89d1bf05-2714-48fa-8274-0ee0dde90218

here is another example (which also uses ?P):

https://phpstan.org/r/5d304dcb-75e5-4233-a505-747055b9f84b

here are more examples, but I don't know whether these are related to hoa parsing

https://github.com/phpstan/phpstan-src/blob/07ed97b0c32fa951153d33bc712d8ad3be87a75c/tests/PHPStan/Analyser/nsrt/preg_match_shapes.php#L126-L148

patch for (?P<... is here: https://github.com/mvorisek/Hoa-Regex/commit/2cd9490df68fff675385b46fd990dee88f510bb9

@mvorisek thanks for looking into it. the suggested patch leads to errors (regressions), see phpstan/phpstan-src#3228

@mvorisek I might have spoken to fast and needs another look from my side. I mixed something up

Found another grammar issue @mvorisek https://phpstan.org/r/94070d37-7a16-4487-9098-f2a5677dfd43

The issue is that () are probably seen as a match group here when they are not escaped, but they are within a character class [] so escaping them is not needed (only - if it is between characters, and ], need to be escaped in character classes, no other character has special meaning).

See https://3v4l.org/m0Vrv for runtime proof :)

I will look into that.

@staabm I think it might be fixable by changing this line https://github.com/hoaproject/Regex/blob/master/Source/Grammar.pp#L168

to this:

    ( <class_> | range() | literal() | ::capturing_:: | ::_capturing:: )+ <range>?

Would https://github.com/hoaproject/Regex/blob/master/Source/Grammar.pp#L76C9-L76C23 be matched then too, ie. [(?:...]?

It would be good to run/parse all https://github.com/PCRE2Project/pcre2/tree/master/testdata testcases to verify all PCRE2 supported grammars are supported.

@Seldaek After the latest push in 1.12.x, PHPStan now reports different result with your code snippet:

@@ @@
  9: Dumped type: array<string>
-13: Dumped type: array{0: string, name: string, 1: string, email?: string, 2?: string}
+13: Dumped type: array<string>
 18: Dumped type: array<string>
 22: Dumped type: array{string}

From PCRE:

   Part  of  a  pattern  that is in square brackets is called a "character
   class". In a character class the only metacharacters are:

     \      general escape character
     ^      negate the class, but only if the first character
     -      indicates character range
     [      POSIX character class (only if followed by POSIX
              syntax)
     ]      terminates the character class

• \ is handled already
• ^ is handled too
• • is handled
• [ for POSIX is not handled it seems but I'd say irrelevant, these are [:digit:] which are not so widely used these days I think
• ] is handled

Everything else should be just parsed as literal in there.

So probably the better option is to add a new token to parse all the leftover characters:

%token class_literal [^\\\[\]-]

Then again change line 168 to include that as last resort:

    ( <class_> | range() | literal() | ::class_literal:: )+ <range>?

I am not super familiar with the syntax here so hopefully there is no mistake, but I think if this passes tests and it can parse /[[()+|.*?]/ as a character class matching any of [()+|.*? characters then it should be pretty good.

BTW just to test the pattern is valid https://regex101.com/r/9SxE17/1

As noted above, https://regex101.com/r/pOq5Jj/1 should be parsed as inner class, otherwise we might be limited in the future when we will analyse classes (like numerical string).

Where did you note this above? And well, right now [:digit:] already causes the parser to crap out, so my patch wouldn't break it any further.. It doesn't fix that though indeed. You could fix it by adding another %token posix_class \[^?:[a-z]+:\] and include that too in ( ::posix_class:: | <class_> | range() | literal() | ::class_literal:: )+ <range>? I think it has to go first before class_ there because that matches a single [ and it'd fail to consume the full posix class then.

Anyway if you want a fully compliant parser it might be better to go with https://github.com/bkiers/pcre-parser/tree/master/src/main/antlr4/nl/bigo/pcreparser + https://packagist.org/packages/antlr/antlr4-php-runtime or something else than retrofitting everything that's missing onto hoa/regex? But in the meantime I'd still be happy if this additional fix can be applied if it indeed works, as it should solve the last chunk of problem I've encountered so far.

I have no idea what to make from your comments.

If there is a grammar change I should apply please advise and give me a test-sample to verify the change

Thank you both

@staabm what I'd do is add this below https://github.com/hoaproject/Regex/blob/master/Source/Grammar.pp#L45-L49 :

%token class_literal [^\\\[\]-]
%token posix_class \[^?:[a-z]+:\]

then change https://github.com/hoaproject/Regex/blob/master/Source/Grammar.pp#L168 to:

    ( ::posix_class:: | <class_> | range() | literal() | ::class_literal:: )+ <range>?

Or this, I am not sure what the difference between <x> and ::x:: is there:

    ( <posix_class> | <class_> | range() | literal() | <class_literal> )+ <range>?

That should make it so that patterns like /([*|+?{}()]+)([^*|+[:digit:]?{}()]+)/ which fail completely to be parsed right now outputs an array{string, string, string} type.

If the patch fails maybe you can create a draft PR with instructions how to run it then I can try to figure it out here?

@Seldaek I have put the suggested change with a unit test into phpstan/phpstan-src#3241

atm it doesn't work though

@Seldaek After the latest push in 1.12.x, PHPStan now reports different result with your code snippet:

@@ @@
 10: Expected type array{0: string, currency: string, 1: string}, actual: array<string>
-14: Expected type array{}|array{0: string, currency: string, 1: string}, actual: array<string>
+14: Expected type array{}|array{0: string, currency: string, 1: string}, actual: array<string>
+17: Expected type array{0: string, currency: string, 1: string}, actual: array{0: string, currency: non-empty-string, 1: non-empty-string}
+21: Expected type array{}|array{0: string, currency: string, 1: string}, actual: array{}|array{0: string, currency: non-empty-string, 1: non-empty-string}

I think this one is good enough for now

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.