fix: `TypeExpression` - fix "JIT stack limit exhausted" error #7843

kubawerlos · 2024-02-17T17:02:19Z

coveralls · 2024-02-17T17:14:00Z

coverage: 96.034%. remained the same
when pulling de58497 on 6b7562617765726c6f73:fix_TypeExpression
into 951d3c2 on PHP-CS-Fixer:master.

src/DocBlock/TypeExpression.php

tests/DocBlock/TypeExpressionTest.php

mvorisek · 2024-02-18T01:46:29Z

src/DocBlock/TypeExpression.php

@@ -53,13 +53,13 @@ final class TypeExpression
                    (?<array_shape_start>(?i)(?:array|list|object)(?-i)\h*\{\h*)
                    (?<array_shape_inners>
                        (?<array_shape_inner>
-                            (?<array_shape_inner_key>(?:(?&constant)|(?&identifier)|(?&name))\h*\??\h*:\h*|)
+                            (?<array_shape_inner_key>(?:[^$\s,\{\|]+)\h*\??\h*:\h*|)


the (?&constant)|(?&identifier) is/was here correctly

and maybe the fix with |(?&name) is not that bad

here is phpstan parser code for "array shape key": https://github.com/phpstan/phpdoc-parser/blob/1.25.0/src/Parser/TypeParser.php#L839 - it allows basically integer, quoted strings, and identifier

we have a little different definion of identifier, thus the difference

here is an example of wrong phpdoc: https://phpstan.org/r/5effc31f-ddff-4e77-ad01-d7844d09beec - we should not parse more, so I prefer to revert this line

mvorisek · 2024-02-18T01:50:04Z

tests/DocBlock/TypeExpressionTest.php

@@ -234,6 +234,17 @@ public static function provideGetTypesCases(): iterable
        yield ['array{a: int, b: int, c: int, d: int, e: int, f: int, g: int, h: int, i: int, j: int, with-dash: int}'];

        yield ['array{a: int, b: int, c: int, d: int, e: int, f: int, g: int, h: int, i: int, j: int, k: int, l: int, with-dash: int}'];
+
+        yield [sprintf(


the "backtrack limit" was a problem when the type was not matched, so we should add also some large types that are invalid, like in #7843 (comment)

Added, rebased to have 2 commits (instead of 8) when in 1st both new test cases - valid and invalid are failing.

mvorisek · 2024-03-08T11:20:45Z

@Wirone this PR is waiting for 6b7562617765726c6f73#1 - or can you cherrypick it into this PR?

Wirone · 2024-03-08T11:27:48Z

@mvorisek I can't, I'm on mobile now, and I see there are merge conflicts in that PR. I will just cancel my approval for now until it's resolved.

Needs resolving external PR

kubawerlos · 2024-03-09T05:52:05Z

@Wirone this PR is waiting for 6b7562617765726c6f73#1 - or can you cherrypick it into this PR?

No, not really, 6b7562617765726c6f73#1 is adding 2 more + that are not covered by tests.

I'd go with this fix as is - as it has tests and fix. Then we can separately figure out what tests would cover the other two + - do I have to mention that adding logical changes to src without tests is not how we do things in this repo?

mvorisek · 2024-03-09T11:49:30Z

@kubawerlos we need to be on the same page first. I did that PR to fix #7841 (comment) in all places. The test you made is logical, but can you explain the #7843 (comment) need? What part of the regex is ambiguous that needs deep backtracking? TBH, I do not know, so from maintainer perspective, I fully agree "we need test always", but let me ask you another question - is this PR good to be merged without fixing the issue on all places?

Wirone · 2024-03-09T14:09:59Z

@mvorisek I believe @kubawerlos would love to "fix all places" but as far as I understand test cases in his PR don't cover + added by you, so either you should provide them with tests or Kuba will be forced to figure out how to test it, which is not that good from his perspective. Let's figure out the best way so we can have a complete fix with proper test coverage 😊.

mvorisek · 2024-03-09T14:28:24Z

I know how to create some test case like @kubawerlos did, but I am not sure what exactly does make the regex ambiguous, ie. I am/would be not able to reason it 😊 nor guarantee the testcase will be still valid with next PCRE release. For me, this is the deeper question I like to be answered.

Also, I am curious what S modifier (https://www.php.net/manual/en/reference.pcre.pattern.modifiers.php) does and if it imply any real performance improvement.

kubawerlos · 2024-03-10T02:17:06Z

nor guarantee the testcase will be still valid with next PCRE release

nobody can predict the future.

I want to have all three newly added + covered with tests, so nobody will remove them and have all tests passing. I covered the one added here, I have not tried to handle others.

I'm still hoping we can merge them separately to have nice proofs that are all covered by tests (maybe as a bonus we can make some refactoring for tests because having testHugeType and createHugeArrayShapeType is not perfect and definitely adding 2 more similar methods is not a way to go).

keradus

For state of this PR, I like what I see.

for state of eagenerss of extra * -> *+ updates in more places, please go for it with separated PR.

I understand eagerness with having it covered with UTests, but I also acknowledge the trickyness of having utest showing internal stack limit crash without +. With that, I'm also OK to have further update to *+ relying on current testbase.

keradus · 2024-03-18T17:07:38Z

@kubawerlos , would you mind to solve the git conflict? I think only you have rights to update the branch.
auto-merge should then merge, when CI is green and no git conflict anymore 🙇🏻

Wirone · 2024-03-18T17:38:06Z

I can update @kubawerlos' PR but I'll let him decide if he wants to incorporate @mvorisek's PR before the rebase and merge here 🙂.

kubawerlos · 2024-03-18T18:18:57Z

I can update @kubawerlos' PR but I'll let him decide if he wants to incorporate @mvorisek's PR before the rebase and merge here 🙂.

Let's do it in separate PR, especially that I have prepared next PR with bugfix that changes *+ to * 😉 (not related to this pare, you'll see very soon).

mvorisek · 2024-03-18T18:51:44Z

@kubawerlos feel free to tag me there, I am curious about this regex optimization :)

Wirone reviewed Feb 17, 2024

View reviewed changes

src/DocBlock/TypeExpression.php Show resolved Hide resolved

tests/DocBlock/TypeExpressionTest.php Outdated Show resolved Hide resolved

mvorisek reviewed Feb 18, 2024

View reviewed changes

kubawerlos force-pushed the fix_TypeExpression branch from 54b4a32 to 8701643 Compare February 18, 2024 08:52

mvorisek mentioned this pull request Feb 25, 2024

Fix TypeExpression "JIT stack limit exhausted" 6b7562617765726c6f73/PHP-CS-Fixer#1

Merged

Wirone mentioned this pull request Feb 26, 2024

Backtrack limit exhausted #7839

Closed

Wirone previously approved these changes Mar 8, 2024

View reviewed changes

kubawerlos added 2 commits March 8, 2024 12:18

Add test

8fdd4e8

Add fix

37620c4

Wirone force-pushed the fix_TypeExpression branch from 7da341d to 37620c4 Compare March 8, 2024 11:18

Wirone added the kind/bug label Mar 8, 2024

Merge branch 'master' into fix_TypeExpression

e021577

keradus approved these changes Mar 18, 2024

View reviewed changes

keradus enabled auto-merge (squash) March 18, 2024 17:06

Merge branch 'master' into fix_TypeExpression

de58497

auto-merge was automatically disabled March 18, 2024 18:17
Head branch was pushed to by a user without write access

keradus merged commit 5380db3 into PHP-CS-Fixer:master Mar 18, 2024
25 checks passed

kubawerlos mentioned this pull request Mar 18, 2024

fix: PhpdocArrayTypeFixer - JIT stack limit exhausted #7895

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: `TypeExpression` - fix "JIT stack limit exhausted" error #7843

fix: `TypeExpression` - fix "JIT stack limit exhausted" error #7843

kubawerlos commented Feb 17, 2024

coveralls commented Feb 17, 2024 •

edited

mvorisek Feb 18, 2024

mvorisek Feb 18, 2024

kubawerlos Feb 18, 2024

mvorisek commented Mar 8, 2024

Wirone commented Mar 8, 2024

kubawerlos commented Mar 9, 2024

mvorisek commented Mar 9, 2024

Wirone commented Mar 9, 2024

mvorisek commented Mar 9, 2024 •

edited

kubawerlos commented Mar 10, 2024

keradus left a comment

keradus commented Mar 18, 2024

Wirone commented Mar 18, 2024

kubawerlos commented Mar 18, 2024

mvorisek commented Mar 18, 2024

fix: TypeExpression - fix "JIT stack limit exhausted" error #7843

fix: TypeExpression - fix "JIT stack limit exhausted" error #7843

Conversation

kubawerlos commented Feb 17, 2024

coveralls commented Feb 17, 2024 • edited

mvorisek Feb 18, 2024

Choose a reason for hiding this comment

mvorisek Feb 18, 2024

Choose a reason for hiding this comment

kubawerlos Feb 18, 2024

Choose a reason for hiding this comment

mvorisek commented Mar 8, 2024

Wirone commented Mar 8, 2024

kubawerlos commented Mar 9, 2024

mvorisek commented Mar 9, 2024

Wirone commented Mar 9, 2024

mvorisek commented Mar 9, 2024 • edited

kubawerlos commented Mar 10, 2024

keradus left a comment

Choose a reason for hiding this comment

keradus commented Mar 18, 2024

Wirone commented Mar 18, 2024

kubawerlos commented Mar 18, 2024

mvorisek commented Mar 18, 2024

fix: `TypeExpression` - fix "JIT stack limit exhausted" error #7843

fix: `TypeExpression` - fix "JIT stack limit exhausted" error #7843

coveralls commented Feb 17, 2024 •

edited

mvorisek commented Mar 9, 2024 •

edited