Skip to content

Fix algorithms overload resolution issues #402

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

Merged
merged 6 commits into from
May 19, 2022

Conversation

natecook1000
Copy link
Member

This change addresses two overload resolution problems with the collection-based algorithm methods. First, when RegexBuilder is imported, String gains RegexComponent conformance, which means the RegexComponent-based overloads win for strings, which is undesirable. Second, if a collection has an element type that can be expressed as an array literal, collection-based methods get selected ahead of any standard library counterpart. These two problems combine in a tricky way for split and contains.

For split, both the collection-based and regex-based versions need to be marked as @_disfavoredOverload so that the problems above can be resolved. Unfortunately, this sets up an ambiguity once String has RegexComponent conformance, so the RegexBuilder module includes separate overloads for String and Substring that act as tie-breakers. If introduced in the standard library, these would be a source-breaking change, as they would win over the Element-based split when referencing the split method, as with let splitFunction = myString.split.

For contains, the same requirements hold, with the added complication that the Foundation overlay defines its own String.contains(_:) method with different behavior than included in these additions. For this reason, the more specific overloads for String and Substring can't live in the RegexBuilder module, which creates a problem for source compatibility. As it stands now, this existing code does not compile with the new algorithm methods added, as the type of vowelPredicate changes from (Character) -> Bool to (String) -> Bool:

let str = "abcde"
let vowelPredicate = "aeiou".contains
print(str.filter(vowelPredicate))

@natecook1000 natecook1000 requested review from itingliu and milseman May 11, 2022 16:32
@natecook1000 natecook1000 assigned beccadax and unassigned beccadax May 11, 2022
@natecook1000 natecook1000 requested a review from beccadax May 11, 2022 16:32
@natecook1000
Copy link
Member Author

@swift-ci Please test

1 similar comment
@natecook1000
Copy link
Member Author

@swift-ci Please test

This change addresses two overload resolution problems with the
collection-based algorithm methods. First, when RegexBuilder is
imported, `String` gains `RegexComponent` conformance, which means
the `RegexComponent`-based overloads win for strings, which is
undesirable. Second, if a collection has an element type that can
be expressed as an array literal, collection-based methods get
selected ahead of any standard library counterpart. These two problems
combine in a tricky way for `split` and `contains`.

For `split`, both the collection-based and regex-based versions need
to be marked as `@_disfavoredOverload` so that the problems above can
be resolved. Unfortunately, this sets up an ambiguity once `String`
has `RegexComponent` conformance, so the `RegexBuilder` module
includes separate overloads for `String` and `Substring` that act
as tie-breakers. If introduced in the standard library, these would
be a source-breaking change, as they would win over the `Element`-
based split when referencing the `split` method, as with
`let splitFunction = myString.split`.

For `contains`, the same requirements hold, with the added
complication that the Foundation overlay defines its own
`String.contains(_:)` method with different behavior than included
in these additions. For this reason, the more specific overloads for
`String` and `Substring` can't live in the `RegexBuilder` module,
which creates a problem for source compatibility. As it stands now,
this existing code does not compile with the new algorithm methods
added, as the type of `vowelPredicate` changes from `(Character) ->
Bool` to `(String) -> Bool`:

```
let str = "abcde"
let vowelPredicate = "aeiou".contains
print(str.filter(vowelPredicate))
```
This allows the stdlib's `contains(_:Character)` to be chosen when
referenced in an unconstrained context.
This makes sure that a plain String doesn't route through the
regex machinery when RegexBuilder is imported.
@natecook1000
Copy link
Member Author

@swift-ci Please test

@milseman
Copy link
Member

Did we discuss or agree to a solution here?

@natecook1000
Copy link
Member Author

No, not yet - this is for discussion in comparison to #411.

@natecook1000
Copy link
Member Author

@swift-ci Please test

@natecook1000 natecook1000 merged commit 88dc9dd into swiftlang:main May 19, 2022
@natecook1000 natecook1000 deleted the contains_overload branch May 19, 2022 23:28
natecook1000 added a commit to natecook1000/swift-experimental-string-processing that referenced this pull request May 20, 2022
This change addresses two overload resolution problems with the
collection-based algorithm methods. First, when RegexBuilder is
imported, `String` gains `RegexComponent` conformance, which means
the `RegexComponent`-based overloads win for strings, which is
undesirable. Second, if a collection has an element type that can
be expressed as an array literal, collection-based methods get
selected ahead of any standard library counterpart. These two problems
combine in a tricky way for `split` and `contains`.

For `split`, both the collection-based and regex-based versions need
to be marked as `@_disfavoredOverload` so that the problems above can
be resolved. Unfortunately, this sets up an ambiguity once `String`
has `RegexComponent` conformance, so the `RegexBuilder` module
includes separate overloads for `String` and `Substring` that act
as tie-breakers. If introduced in the standard library, these would
be a source-breaking change, as they would win over the `Element`-
based split when referencing the `split` method, as with
`let splitFunction = myString.split`.

For `contains`, the same requirements hold, with the added
complication that the Foundation overlay defines its own
`String.contains(_:)` method with different behavior than included
in these additions. For this reason, the more specific overloads for
`String` and `Substring` can't live in the `RegexBuilder` module,
which creates a problem for source compatibility. As it stands now,
this existing code does not compile with the new algorithm methods
added, as the type of `vowelPredicate` changes from `(Character) ->
Bool` to `(String) -> Bool`:

```
let str = "abcde"
let vowelPredicate = "aeiou".contains
print(str.filter(vowelPredicate))
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants