-
Notifications
You must be signed in to change notification settings - Fork 46
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
Convert the RegexBuilder overloads to use variadic generics #726
Open
natecook1000
wants to merge
12
commits into
main
Choose a base branch
from
actual_variadics
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
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
This uses parameter packs to replace the existing multitude of buildPartialBlock implementations in RegexBuilder.
This converts the RegexBuilder quantifiers to use variadic generics instead of explicit overloads. This transition uses a particular generic pattern so that a variadic number of output parameters are captured correctly, without matching a "zero captures" component as having `()` as its output type. To accomplish this, the generic parameters for each quantification method are like: some RegexComponent<(W, Capture0, repeat each Capture)> The presence of `Capture0` requires that at least one sub-capture beyond the whole match is present. In addition to the variadic versions, unconstrained versions of each initializer are included, marked `@_disfavoredOverload`. These are only selected when the component's output type is a single type (e.g. `Regex<Substring>`).
Not totally sure why the `Capture0` trick from the quantification variadics isn't necessary here, so more source compatibility testing should be done before landing.
Some trickiness involved in getting the optionality of captures to line up, but I think I got this right. Will need some more source- compatibility validation here, as well.
This one was a suspiciously simple transition, but all seems to be working.
natecook1000
force-pushed
the
actual_variadics
branch
from
March 25, 2024 19:51
ef5dbd4
to
8ffd04e
Compare
natecook1000
force-pushed
the
actual_variadics
branch
from
April 23, 2024 17:20
8ffd04e
to
a82307b
Compare
@swift-ci Please test |
@swift-ci Please test |
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.
This change switches all the RegexBuilder overload-based "variadics" to use proper variadic generics. As far as our tests show, this is largely compatible with the existing API, though there is one exception, detailed below. That said, I'm a bit worried about changes in output type resolution for edge cases, particularly with capture groups nested into the various structures.
The incompatibility is for composed regexes with more than 10 capture groups, which is the current limit of our overloads in RegexBuilder. Right now, when a regex with a large number of capture groups is included in a builder-syntax closure, its captures are dropped from the output type of the resulting regex. Though this is definitely a bug in the current system, fixing it represents a source break that may or may not be visible to clients.
The
dslWithTooManyCaptures
regex currently has an output type of(Substring, Substring, Int, Substring)
because the captures inregexWithTooManyCaptures
are left out of the resulting output type. With this change, the output type becomes(Substring, Substring, Substring, Substring, Substring, Substring, Substring, Substring, Substring, Substring, Substring, Substring, Substring, Substring, Substring, Int, Substring)
. If a client matches and only accessesoutput.3
, the value of the captured value will silently change with this change in API. Trying to access a capture group with a different type (e.g,output.2
is currentlyInt
, but would change toSubstring
) or using the output in a typed context (e.g passing the tuple as an initializer's parameters) would result in a compilation failure.(Note that when converted to
Regex<AnyRegexOutput>
, the full set of captures is available in both the current and new versions of the API.)