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
fix: TypeExpression
- fix "JIT stack limit exhausted" error
#7843
fix: TypeExpression
- fix "JIT stack limit exhausted" error
#7843
Conversation
src/DocBlock/TypeExpression.php
Outdated
@@ -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*|) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, rebased to have 2 commits (instead of 8) when in 1st both new test cases - valid and invalid are failing.
54b4a32
to
8701643
Compare
7da341d
to
37620c4
Compare
@Wirone this PR is waiting for 6b7562617765726c6f73#1 - or can you cherrypick it into this PR? |
@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. |
No, not really, 6b7562617765726c6f73#1 is adding 2 more 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 |
@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? |
@mvorisek I believe @kubawerlos would love to "fix all places" but as far as I understand test cases in his PR don't cover |
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 |
nobody can predict the future. I want to have all three newly added 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@kubawerlos , would you mind to solve the git conflict? I think only you have rights to update the branch. |
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 🙂. |
Head branch was pushed to by a user without write access
Let's do it in separate PR, especially that I have prepared next PR with bugfix that changes |
@kubawerlos feel free to tag me there, I am curious about this regex optimization :) |
Ref. #7841 (comment)