Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Flag effect remains active outside of current group #640

Closed
okdana opened this issue Jan 30, 2020 · 3 comments · Fixed by #641
Closed

Flag effect remains active outside of current group #640

okdana opened this issue Jan 30, 2020 · 3 comments · Fixed by #641
Labels
bug

Comments

@okdana
Copy link

okdana commented Jan 30, 2020

The regex documentation suggests that flags ((?foo)) only apply within the 'current group', which would match the behaviour of Perl and PCRE, but that doesn't seem to be the case:

# With PCRE
% printf '%s\n' foo Foo bar Bar | \rg -sP '((?i)foo)|Bar'
foo
Foo
Bar

# With Rust regex
% printf '%s\n' foo Foo bar Bar | \rg -s '((?i)foo)|Bar'
foo
Foo
bar  # ???
Bar

(My test case uses ripgrep 11.0.2 from Homebrew, but i've confirmed that it works the same way if i compile a simple test app with regex 1.3.3.)

Am i misunderstanding something, or is this wrong?
@BurntSushi BurntSushi added the bug label Jan 30, 2020
BurntSushi added a commit that referenced this issue Jan 30, 2020
@BurntSushi
syntax: fix flag scoping issue
6eaf2f2 
This fixes a rather nasty bug where flags set inside a group were being
applies to expressions outside the group. e.g., In the simplest case,
`((?i)a)b)` would match `aB`, even though the case insensitive flag
_shouldn't_ be applied to `b`.

The issue here was that we were actually going out of our way to reset
the flags when a group is popped only _some_ of the time. Namely, when
flags were set via `(?i:a)b` syntax. Instead, flags should be reset to
their previous state _every_ time a group is popped in the translator.

The fix here is pretty simple. When we open a group, if the group itself
does not have any flags, then we simply record the current state of the
flags instead of trying to replace the current flags. Then, when we pop
the group, we are guaranteed to obtain the old flags, at which point, we
reset them.

Fixes #640
@BurntSushi BurntSushi mentioned this issue Jan 30, 2020
Merged
@BurntSushi
Copy link
Member

Yeesh. What a bug. This is definitely wrong. Interestingly, the code was actually going out of its way not to reset flags in this case. I'm not sure why. I don't think I ever intended for this behavior, and no tests were checking for it. And as you say, the docs seem to pretty strongly contradict the current behavior.

I have a fix up. Thanks for reporting this!

BurntSushi added a commit that referenced this issue Jan 30, 2020
@BurntSushi
syntax: fix flag scoping issue
ea4009a 
This fixes a rather nasty bug where flags set inside a group were being
applies to expressions outside the group. e.g., In the simplest case,
`((?i)a)b)` would match `aB`, even though the case insensitive flag
_shouldn't_ be applied to `b`.

The issue here was that we were actually going out of our way to reset
the flags when a group is popped only _some_ of the time. Namely, when
flags were set via `(?i:a)b` syntax. Instead, flags should be reset to
their previous state _every_ time a group is popped in the translator.

The fix here is pretty simple. When we open a group, if the group itself
does not have any flags, then we simply record the current state of the
flags instead of trying to replace the current flags. Then, when we pop
the group, we are guaranteed to obtain the old flags, at which point, we
reset them.

Fixes #640
BurntSushi added a commit that referenced this issue Jan 30, 2020
@BurntSushi
deps: require regex-syntax 0.6.14
5e6ee59 
This brings in the bug fix for #640 for flag scoping.
BurntSushi added a commit to BurntSushi/ripgrep that referenced this issue Jan 30, 2020
@BurntSushi
deps: update regex, regex-syntax and aho-corasick
9fd1e20 
Notably, this brings in a bug fix reported by @okdana:
rust-lang/regex#640
@BurntSushi
Copy link
Member

OK, this should be fixed in regex 1.3.4 and regex-syntax 0.6.14 on crates.io. I've also updated ripgrep master to include these dependency updates.

@okdana
Copy link
Author

okdana commented Jan 30, 2020

Thanks!

yvt added a commit to yvt/Stella2 that referenced this issue Feb 7, 2020
@yvt
test(designer): fix the flag scoping of a regex used to detect error …
d4a9b91 
…messages

This regex was relying on a bug with flag scoping that was fixed
(rust-lang/regex#640) in regex 1.3.4.
yvt added a commit to yvt/Stella2 that referenced this issue Feb 7, 2020
@yvt
test(designer): fix the flag scoping of the regex used to detect erro…
6a0ac71 
…r messages

This regex was relying on a bug with flag scoping that was fixed
(rust-lang/regex#640) in regex 1.3.4.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

syntax: fix flag scoping issue rust-lang/regex
2 participants
@okdana @BurntSushi