-
Notifications
You must be signed in to change notification settings - Fork 545
Fix regular expression parsing #3244
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
Conversation
f3b0770 to
56a35df
Compare
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.
@ondrejmirtes Are we ok loading the file from here instead? Not sure what this does in terms of licensing, nor if there's a better place to put it. If so I'll delete the patch file and drop the hoa/regex requirement.
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.
I am not sure this path will work in the packaged phpstan.phar - while I agree that having the RegexGrammar.pp in the phpstan-src repo might ease the contribution experience - as the patching is really cumbersome.
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.
I completely agree moving from the patching as the patching is very hard to understand and the changes needed are more and more complex.
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.
Ok I got rid of it, we'll see what @ondrejmirtes says :)
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.
I propose to add the whole and unpatched RegexGrammar.pp as first/separate commit, then the patched/actual grammar and then the changes from this PR. This will hugely improve the review and git history.
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.
Good idea, done 👍🏻
4373366 to
b022bba
Compare
This pull request has been marked as ready for review.
cf82d5d to
8288f09
Compare
@staabm please review the last (削除) 4 (削除ここまで)5 commits and especially test changes.
1c4308e to
a7df920
Compare
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.
this looks really good for me.
only open question (which Ondrej has to decide on): what todo with the RegexGrammar.pp.
resources/RegexGrammar.pp
Outdated
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.
\n can be in pcre class as well
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.
Yes I am unsure why this is here, I haven't experimented much, but it is rare to have actual newlines in regexes as they're usually escaped as \n, while this here matches a real newline.
I'll create a new issue for this to look at it later, together with proper support for the x modifier, because it's very much related. Edit: phpstan/phpstan#11360
292e211 to
64901f6
Compare
- Skip regex parsing if the pattern is not valid, but report errors then if parsing fails - Fix support for internal options to include more options, and fix []] (i.e. class end delimiter as first character is not a literal) not parsing correctly - Add a couple missing quantifiers in getQuantificationRange - Handle more literal values and fix escaped ones to be unescaped
f54912e to
630c1a2
Compare
I cherry-picked the grammar in the repo directly to 1.11.x, rebased this PR, and merged it. Thanks! :)
Uh oh!
There was an error while loading. Please reload this page.
I moved the grammar file into the repo instead of the hoa regex one with patch, that makes it a lot easier to work with.
This PR fixes parsing of complex character classes which was really not well supported in hoa/regex, along with a bunch of other smaller issues.