Allow Regex<AnyRegexOutput> to be used in the DSL.
#504
Conversation
What does this do to anchors? Do we need to fix up the search bounds? |
| @@ -278,6 +278,10 @@ extension Instruction { | |||
| /// | |||
| case backreference | |||
|
|
|||
| case beginTypeErase | |||
|
|
|||
| case endTypeErase | |||
There was a problem hiding this comment.
What do these instructions do? What's their semantics?
There was a problem hiding this comment.
Added comments. The processor will maintain a stack of capture lists. beginTypeErase will push a new list onto the capture stack, so that all captures get added to that list. endTypeErase(_: ValReg) will convert all elements of the current capture list to an AnyRegexOutput, and moves it to the given value register, and pop the stack. It's similar to how matcher works.
There was a problem hiding this comment.
I'm not fully parsing this. Could you write some XFAIL tests that illustrate the work that remains to be done after this PR?
There was a problem hiding this comment.
There's no XFAIL-able tests. This PR fixes the bug, but it's not as efficient as handling this in the bytecode directly. I'd be happy to chat in person also.
There was a problem hiding this comment.
How would begin/endTypeErase work in the context of backtracking. For that matter, how does this PR support any backtracking into ARO at all, i.e. isn't this forcing it to be atomic?
There was a problem hiding this comment.
I think backtracking into ARO will be supported, since savePoints currently stores the capture list. It can be extended to store the capture stack.
| func testTypeErasedRegexInDSL() throws { | ||
| do { | ||
| let input = "johnappleseed: 12." | ||
| let numberRegex = try! Regex(#"(\d+)\.?"#) |
There was a problem hiding this comment.
Can you add a test case where the dynamic regex begins with a ^? I think that would help clarify the behavior you're proposing.
There was a problem hiding this comment.
Added one. The idea is to fix up the search bounds so that ^ refers to the start of the input.
It happens to work today because anchors currently use the base string's bounds, not the input slice's bounds.
There was a problem hiding this comment.
Right, there is a bug here that another bug is masking, but when that other bug is fixed, how do we fix this bug?
There was a problem hiding this comment.
You can illustrate the difference with a substring input, where the subject bounds are the substring's bounds and the search bounds are contained within.
rxwei
force-pushed
the
rdar94320030
branch
2 times, most recently
from
aa0ca5b to
63124f5
Compare
Apply a workaround to allow `Regex<AnyRegexOutput>` to be used in the DSL. This workaround emits each nested `Regex<AnyRegexOutput>` as a custom matcher so that it's essentially treated as a separate compilation unit. A proper fix for this is to introduce scoped type erasure in the matching engine so that all type erasure (including the top-level one) goes through this model. I left some stubs in (`beginTypeErase`, `endTypeErase`) which I'll implement in a follow-up. Since this implementation is using `Executor.match(...) -> Regex.Match` in the regex compiler, we need to add availability annotations to the `Executor` and `Compiler` types. Resolves rdar://94320030.
Hope this clarifies the intended design. Let me know if you have any feedback. |
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
There's a small bug in there and the fix should be relatively simple. I'll take a look tomorrow. |
| // endTypeErase | ||
| let program = try Compiler(tree: DSLTree(child)).emit() | ||
| let executor = Executor(program: program) | ||
| return emitMatcher { input, startIndex, range in |
There was a problem hiding this comment.
Matchers don't get the subject bounds, so I don't see how this would support anchors that refer to the subject bounds. We could have a different matcher interface or else add subject bounds as an additional parameter for the internal code (probably not API though).
| if case .typeErase = root { | ||
| return self | ||
| } | ||
| return .init(node: .typeErase(root)) |
There was a problem hiding this comment.
Can we do this on creation instead, or somewhere else?
| var root = root | ||
| while case let .typeErase(child) = root { | ||
| root = child | ||
| } |
There was a problem hiding this comment.
Why is this scan in here? It seems like you'd want to capture an ARO, is that not possible?
| ZeroOrMore(.whitespace) | ||
| Capture { numberRegex } | ||
| } | ||
| XCTAssertNil(input.wholeMatch(of: regex)) |
There was a problem hiding this comment.
Positive match tests with anchors?
| func testTypeErasedRegexInDSL() throws { | ||
| do { | ||
| let input = "johnappleseed: 12." | ||
| let numberRegex = try! Regex(#"(\d+)\.?"#) |
There was a problem hiding this comment.
Right, there is a bug here that another bug is masking, but when that other bug is fixed, how do we fix this bug?
| func testTypeErasedRegexInDSL() throws { | ||
| do { | ||
| let input = "johnappleseed: 12." | ||
| let numberRegex = try! Regex(#"(\d+)\.?"#) |
There was a problem hiding this comment.
You can illustrate the difference with a substring input, where the subject bounds are the substring's bounds and the search bounds are contained within.
Apply a workaround to allow
Regex<AnyRegexOutput>to be used in the DSL. This workaround emits each nestedRegex<AnyRegexOutput>as a custom matcher so that it's essentially treated as a separate compilation unit.A proper fix for this is to introduce scoped type erasure in the matching engine so that all type erasure (including the top-level one) goes through this model. I left some stubs in (
beginTypeErase,endTypeErase) which I'll implement in a follow-up.Since this implementation is using
Executor.match(...) -> Regex.Matchin the regex compiler, we need to add availability annotations to theExecutorandCompilertypes.Resolves rdar://94320030.