better support for empty patterns #677
Closed
Conversation
This file contains 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
|
For the test failure: should the documentation of hir and implementation of exec.rs be changed to accept Empty nodes in is_alternation_literal? |
|
@sliquister Good question. The docs suggest that |
To avoid this assertion in tests when empty alternations are allowed:
internal error: entered unreachable code: expected literal or concat, got Hir { kind: Empty, info: HirInfo { bools: 1795 } }', src/exec.rs:1568:18
The code in exec.rs relies on the documented invariant for is_alternation_literal:
/// ... This is only
/// true when this HIR expression is either itself a `Literal` or a
/// concatenation of only `Literal`s or an alternation of only `Literal`s.
Some were rejected explicitly with this "alternations cannot currently
contain empty sub-expressions" error. Some were causing alternations
to be compiled into automaton matching a different regex.
The root of the issue is that the compilation of the Empty pattern
returns Patch { entry: self.insts.len(), .. }, that is it claims its
entry point is the next instruction. This only works if the next
instruction is unconditionally executed, which is not true for
alternations.
So we return None instead, and let the callers decide how to handle an
empty regex.
|
Thanks, the tests all pass now (and I've learned of the existence of |
|
Have you had a chance to consider this? (I'm trying to decide if I need to workaround the underlying issue) |
BurntSushi
approved these changes
May 28, 2020
|
This PR is in As a bonus, this also fixed #659! |
|
Marvelous, thank you! (especially with this change being released immediately) |
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.
I propose fixing the known issue in compile.rs where empty patterns + alternations can cause compilation errors or be compiled into a wrong automaton.
I understand that the plan is to replace compile.rs with a better version, but this doesn't seem to be about to happen, and I think the fix is not an undue amount of work. If possible, I would prefer fixing
regexthan work around these issues (it's easy to avoid them in hand written regexes but when generating regexes, it's more annoying).About the change itself: all I do is compile
EmptyintoOk(None)instead ofOk(Patch{ entry: the-next-instruction, ..})(as that is not correct in general) and follow the type system. I tried fixing this in other ways with less diff, but I think this version is simpler: the diff is bigger (mostly due to changes types and indentation, which are easy to review) but the performance is clearly ok and the type system forces me to consider where changes need to happen. The kind of diffself.c(expr)?->self.c(expr)?.unwrap_or(self.next_inst());is saying "at this call site, the old behavior worked".The same automation should be generated in the case that were previously correctly and successfully compiled, except a few corner cases that involve empty subexpressions, where we may generate fewer instructions (like
(?:)?).