[stdlib] Constrain Sequence methods that return AnySequence #12029
Conversation
This file contains hidden or 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
|
@swift-ci please test source compatibility |
|
@swift-ci please test |
|
There were a couple more in there too but applying the same trivial fix didn't work for them so checking compatibility of these first. |
|
Build failed |
|
@swift-ci please test linux platform |
|
Build failed |
There was a problem hiding this comment.
🎉 I found a couple others on Sequence, and this weird exception for RandomAccessIndices: https://github.com/airspeedswift/swift/blob/384989ea79c7f390fa8d408360acff35422352ed/stdlib/public/core/Indices.swift.gyb#L123 — any idea why that's emitted without the constraint?
stdlib/public/core/Sequence.swift
Outdated
| SubSequence : Sequence, | ||
| SubSequence.Element == Element, | ||
| SubSequence.SubSequence == SubSequence { | ||
| extension Sequence where SubSequence == AnySequence<Element> { |
There was a problem hiding this comment.
There are a few default implementations in other extensions that should be constrainable:
- suffix(_:)
- split(maxSplits:omitting...:whereSeparator:)
- split(separator:maxSplits:omitting...:) (Does it matter that this is a pure extension and not a requirement?)
There was a problem hiding this comment.
Re that last one, I suppose it does, since it means you couldn't just use that method in a Sequence where Element: Equatable extension/generic. Hmmm. There's no way to implement it just in terms of returning SubSequence, since it needs to eagerly consume the sequence in order to allow random access to the chunks in the array. Maybe that should be converted to return [[Element]] instead? Not sure what we're getting out of it returning an AnySequence wrapping an array if it doesn't need to match a SubSequence requirement anyway.
There was a problem hiding this comment.
Maybe that should be converted to return
[[Element]]instead?
Hmmm. There's possible a good case for the default split on sequences being lazy and non-consuming. Anyway, not such a slam-dunk as the prefix/suffix cases.
There was a problem hiding this comment.
It has to consume the sequence, since it returns an array and someone might write something like:
let result = mySinglePassSequence.split(separator: " ")
let lastChunk = result.last!We could have a lazy version that returns a non-array sequence, but these ones need to be eager.
|
@swift-ci Please test Linux platform |
airspeedswift
force-pushed
the
constrain-anysubsequence
branch
from
384989e to
75df81b
Compare
|
@swift-ci please test |
airspeedswift
force-pushed
the
constrain-anysubsequence
branch
from
75df81b to
f338aa2
Compare
|
This should be unblocked now |
airspeedswift
force-pushed
the
constrain-anysubsequence
branch
from
bf5c960 to
f85b87f
Compare
|
@swift-ci please test |
|
@swift-ci please test source compatibility |
airspeedswift
changed the title
|
@natecook1000 yeah I can't spot why that's necessary either. Possibly working around a since-fixed compiler crasher? |
|
@swift-ci please smoke benchmark |
|
@swift-ci please smoke test |
|
@swift-ci please smoke test compiler performance |
|
!!! Couldn't read commit file !!! |
Build comment file:Optimized (O)Regression (3)
Improvement (22)
No Changes (304)
Unoptimized (Onone)Regression (3)
Improvement (3)
No Changes (323)
Hardware Overview |
|
@swift-ci please smoke test compiler performance |
natecook1000
reviewed
Oct 3, 2017
stdlib/public/core/Indices.swift.gyb
Outdated
| @@ -120,9 +120,6 @@ public struct ${Self}< | |||
| } | |||
|
|
|||
| extension ${collectionForTraversal(Traversal)} | |||
| % if Traversal != 'RandomAccess': | |||
| where Indices == ${Self}<Self> | |||
There was a problem hiding this comment.
Only the % if ... should go away here, right? We want this extension to be constrained to where Self.Indices == DefaultIndices<Self> for each specialized version of Default____Indices.
There was a problem hiding this comment.
I'm going to do this part in a separate PR along with another change.
|
Hoping this will clear up the failures in the newly-re-enabled AsyncNinja. |
|
@swift-ci Please test source compatibility |
airspeedswift
force-pushed
the
constrain-anysubsequence
branch
from
fd4fec8 to
f85b87f
Compare
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.
Currently
Sequence's defaultSubSequenceisAnySequence, but it gets it in a roundabout way. This does it more explicitly, and then constrains the default implementations to sequences that don't have a custom subsequence (including collections, which do this via slicing instead).This means you no longer get a bunch of confusing overloads on collections like
Array:Built on top of #11923, for which I pine. It might not need it.
rdar://problem/34548873