Skip to content

[SE-0157] Standard library uses of Recursive Protocol Constraints #11923

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

Conversation

DougGregor
Copy link
Member

@DougGregor DougGregor commented Sep 14, 2017

This pull request implements the standard library part of SE-0157: Recursive Protocol Constraints, which includes:

  • Make the SubSequence associated type have the same capabilities as its enclosing protocol, e.g., Sequence.SubSequence conforms to Sequence, Collection.SubSequence conforms to Collection, and so on.
  • Make the Indices associated type have the same traversal requirements as its enclosing protocol, e.g., Collection.Indices conforms to Collection, BidirectionalCollection.Indices conforms to BidirectionalCollection, and so on
  • Make Numeric.Magnitude conform to Numeric
  • Use more efficient SubSequence types for lazy filter and map
  • Eliminate the *Indexable protocols.

Fixes SR-3453 and rdar://problem/20715031 rdar://problem/28330668.

@DougGregor DougGregor changed the title [SE-0157] Standard library uses of Recursive Protocol Constraints [WIP] [SE-0157] Standard library uses of Recursive Protocol Constraints Sep 14, 2017
@DougGregor DougGregor force-pushed the se-0157-recursive-constraints-stdlib branch from fdd5482 to 17df27f Compare September 14, 2017 17:22
@DougGregor
Copy link
Member Author

@swift-ci please smoke test

@DougGregor
Copy link
Member Author

@swift-ci please test source compatibility

@DougGregor
Copy link
Member Author

Note: rebased so the compiler changes don't show up in this pull request. Now it's all library + test suite updates. Also, improved the SubSequence type for lazy map and filter.

@DougGregor DougGregor force-pushed the se-0157-recursive-constraints-stdlib branch from 17df27f to 0e81e08 Compare September 14, 2017 20:12
@gribozavr
Copy link
Contributor

I have been waiting for this for years! Thanks, Doug!

@dabrahams
Copy link
Contributor

Wonderful to see!
I guess this means the type checker slowdowns are fixed?

@gottesmm
Copy link
Contributor

@gribozavr He lives!

@DougGregor
Copy link
Member Author

Type checker slowdowns have not been fixed yet; I'm collecting the standard library improvements here, and pushing the compiler improvements into master via separate PRs. I won't merge this until compile-time performance is acceptable.

@DougGregor
Copy link
Member Author

@gribozavr, your standard library annotations are invaluable in rolling out this feature.

@DougGregor DougGregor force-pushed the se-0157-recursive-constraints-stdlib branch from 6fe6ec0 to 8135593 Compare September 19, 2017 22:11
Copy link
Contributor

@moiseev moiseev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎆

@DougGregor
Copy link
Member Author

FWIW, most of those hacks were due to an intermediate compiler state and are no longer an issue. I'll update the PR soonish... I'm testing the local removal of said hacks.

@DougGregor DougGregor force-pushed the se-0157-recursive-constraints-stdlib branch from a2aef31 to 4aa8471 Compare September 19, 2017 23:53
@DougGregor
Copy link
Member Author

@swift-ci please test

@DougGregor
Copy link
Member Author

@swift-ci please test source compatibility

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 17df27f8258a337ebb95bd7205fbd244e755747f

@DougGregor
Copy link
Member Author

@swift-ci please test source compatibility

@DougGregor
Copy link
Member Author

That GSB commit doesn't belong here; I'll zap it later.

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 17df27f8258a337ebb95bd7205fbd244e755747f

@DougGregor
Copy link
Member Author

Well, that's encouraging!

@DougGregor
Copy link
Member Author

@slavapestov that last commit's for you

@airspeedswift
Copy link
Member

Any chance you could throw StringProtocol.SubSequence: StringProtocol in here too?

Addressed ABI FIXME’s #4, swiftlang#5, swiftlang#104 and swiftlang#105, making Sequence’s
SubSequence conform to Sequence, with the same element type, and for
which the SubSequence of a SubSequence is the same SubSequence.

Fixes SR-318 / rdar://problem/31418206.
…ection.

Introduce (recursive) constraints that make the *Collection constraint
of SubSequence match that of its enclosing *Collection, e.g.,
MutableCollection.SubSequence conforms to MutableCollection.

Fixes rdar://problem/20715031 and more of SR-3453.
Make the Indices types conform to the appropriate Collection protocol:
* Collection.Indices: Collection
* BidirectionalCollection.Indices: BidirectionalCollection
* RandomAccessCollection.Indices: RandomAccessCollection
…traints.

Eliminates a few explicit uses of the Indexable protocols.
Rather than using the default slice type when slicing the collection produced
by a lazy map or filter, slice the base collection and form a new
lazy map/filter collection from it. This allows any optimizations provided by
the collection SubSequence type to kick in, as well as ensuring that slicing
a lazy collection provides the same type as producing a lazy collection of a
slice.

This is technically source-breaking, because someone could have spelled out
the types of slicing a lazy filter or map… but it seems unlikely to matter
in practice and the benefits could be significant.

Fixes ABI FIXME’s swiftlang#28 and swiftlang#46.
The various _*Indexable protocols only exist to work around the lack of
recursive protocol constraints. Eliminate all of the *_Indexable protocols,
collapsing their requirements into the corresponding Collection protocol
(e.g., _MutableIndexable —> Collection).

This introduces a number of extraneous requirements into the various
Collection protocols to work around bugs in associated type
inference. Specifically, to work around the lack of "global" inference
of associated type witnesses. These hacks were implicitly present in
the *Indexable protocols; I've made marked them as ABI FIXMEs here so
we can remove them when associated type inference improves.

Fixes rdar://problem/21935030 and a number of ABI FIXMEs in the library.
@DougGregor DougGregor force-pushed the se-0157-recursive-constraints-stdlib branch from 430a86d to 772352e Compare October 1, 2017 22:08
@DougGregor DougGregor changed the title [WIP] [SE-0157] Standard library uses of Recursive Protocol Constraints [SE-0157] Standard library uses of Recursive Protocol Constraints Oct 1, 2017
@DougGregor
Copy link
Member Author

@swift-ci please test

@DougGregor
Copy link
Member Author

@swift-ci please test source compatibility

@DougGregor
Copy link
Member Author

@swift-ci please test

@swift-ci
Copy link
Contributor

swift-ci commented Oct 1, 2017

Build failed
Swift Test Linux Platform
Git Sha - db7e9df0df4061d3a886c53073fbd830f330fa1d

@DougGregor
Copy link
Member Author

@swift-ci please test source compatibility

@swift-ci
Copy link
Contributor

swift-ci commented Oct 2, 2017

Build failed
Swift Test Linux Platform
Git Sha - 772352e

@swift-ci
Copy link
Contributor

swift-ci commented Oct 2, 2017

Build failed
Swift Test OS X Platform
Git Sha - 772352e

@DougGregor
Copy link
Member Author

@swift-ci please test

@swift-ci
Copy link
Contributor

swift-ci commented Oct 2, 2017

Build failed
Swift Test Linux Platform
Git Sha - 772352e

@swift-ci
Copy link
Contributor

swift-ci commented Oct 2, 2017

Build failed
Swift Test OS X Platform
Git Sha - 772352e

@DougGregor
Copy link
Member Author

Fixed a crasher. Huh.

@DougGregor
Copy link
Member Author

@swift-ci please smoke test and merge

1 similar comment
@DougGregor
Copy link
Member Author

@swift-ci please smoke test and merge

@swift-ci swift-ci merged commit 24821cc into swiftlang:master Oct 2, 2017
@DougGregor DougGregor deleted the se-0157-recursive-constraints-stdlib branch October 2, 2017 04:07
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.

8 participants