fix: treat ? and + after a closing extglob as glob tokens - #191
Open
Jaybhade wants to merge 1 commit into
Open
Conversation
A metacharacter following a closing extglob paren was emitted straight into the regex, where it acted as a quantifier on the preceding group: @(a|b)? -> /^(?:(a|b)?)$/ group made optional *(a)? -> /^(?:(?=.)(?:a)*?)$/ star made lazy ?(a)+ -> /$^/ invalid regex, matches nothing The star handler already applies the intended rule, using regex semantics after a closing paren or bracket only when options.regex is true. Apply the same rule to ? and +, so ? is a single character wildcard and + is a literal plus, as in bash. Groups opened by @ are tokenized as plain parens rather than through extglobOpen/extglobClose, so track their paren depth to tell @(a|b) apart from a bare (a|b) capture group. Quantifiers on plain groups are unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A
?or+that follows a closing extglob is passed straight through to the generated regex, where it binds to the preceding group as a quantifier instead of being treated as a glob token.The last one is the worst case.
(?:a)?+is not valid JavaScript, socompileRecatches theSyntaxErrorand falls back to/$^/. Nothing is thrown and nothing is logged;?(a)+,*(a)+and+(a)+simply match nothing at all.For reference:
Why
*is already correctThe star handler in
parse.jsonly uses regex semantics after a closing paren or bracket when the caller asks for it:which is what the
regexoption is documented to control:So
@(a|b)*is right today.?and+never got that check, so they always take the regex branch. This PR applies the same rule to them: after a closing extglob,?is a single character wildcard and+is a literal plus, unlessoptions.regexis true.Scope
Only extglobs change.
(a|b)?and(a|b)+are ordinary capture groups and keep their quantifiers, so this does not touch the ambiguity raised in #101, which is about plain parens and looked to be blocked on how many tests depend on the current behaviour. There is a test pinning that.@(...)needed a little care. Unlike the other four, it is tokenized as anattoken followed by plain parens, so its closing)is indistinguishable from a capture group's. The patch records the paren depth of a group opened by@and marks the matching)as an extglob, which is what lets@(a|b)?and(a|b)?be told apart.!(...)composed with a trailing token still diverges from bash for a separate reason. Its negative lookahead is only$-anchored when the extglob ends the pattern, so!(a)?rejects anything starting witharather than anything that is exactlya. That is the same mechanism as #154 and #93 and I have left it alone.Verification
npm test: 1985 passing, up from 1977. The four new bug-covering tests fail onmain; the four guard tests (*unchanged,?(/+(still opening a nested extglob,regex: trueopt-out, plain groups unchanged) pass both before and after.npm run lintis clean.I found this by differentially testing against
bash 3.2withshopt -s extglob, generating pattern/subject pairs from a small extglob grammar. Subjects are slash-free so bash's fnmatch semantics line up, and I excluded the two known divergences (a leading!is picomatch negation, and[!...]is #187). Over 20,000 seeded pairs:The 40 that remain are other classes, mostly an extglob that can match empty following a
*(*?(a),*@(a|)) and the!(...)anchoring above. Happy to look at those separately.I also ran micromatch's suite against a picomatch carrying the equivalent change, since it is the largest consumer: 1954 passing, unchanged.