Skip to content

Add conditional LazySequenceProtocol conformance #29

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 3 commits into from
Oct 22, 2020

Conversation

timvermeulen
Copy link
Contributor

This adds overloads to LazySequenceProtocol of existing lazy-by-default sequence algorithms, for the sole purpose of keeping lazy chains lazy when one of these methods is called.

uniqued is left out of this because it (currently) isn't lazy to begin with. A LazySequenceProtocol overload will only make sense once we have a Uniqued sequence that produces unique elements on demand.

Two unresolved questions:

  • Do we want all of these overloads? I've added them for all algorithms that have a self and return a new sequence/collection, but that might be too lenient. (Is there a guideline for this somewhere?)
  • Should the guides mention these overloads, given that they don't add any actual functionality? The Chunked guide mentions them, but then again lazy.chunked and .chunked.lazy are entirely different things, unlike the ones I'm adding here. Mentioning them in a more general document such as the README is another option.

Checklist

  • I've added at least one test that validates that my change is working, if appropriate
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

@natecook1000
Copy link
Member

This is a great question, @timvermeulen. The goal as I understand it is to avoid requiring people to write .lazy after operations on lazy collections to make sure their collection stays lazy. As far as I can tell, the stdlib takes two different approaches. The one you've taken here is to apply the operation to self.elements, and then call .lazy on the result, which matches the behavior of FlattenSequence:

let a = (1...10).lazy
let b = a.map { $0...20 }.joined()
// b is a LazySequence<FlattenSequence<LazyMapSequence<LazySequence<(ClosedRange<Int>)>.Elements, ClosedRange<LazySequence<(ClosedRange<Int>)>.Element>>.Elements>>

ReversedCollection, on the other hand, conditionally conforms to the lazy protocols when its base collection does:

let c = a.reversed()
// c is a ReversedCollection<LazySequence<(ClosedRange<Int>)>>

This second approach seems cleaner to me (and could have been applied to FlattenSequence) — would it work in the case of this algorithms library?

Re, the guides: I think they should mention either the overloads or the conditional conformance in the detailed design section. It would be great to also have a test that verifies that the laziness passes through properly (with a fatalError-ing map, perhaps).

@timvermeulen
Copy link
Contributor Author

ReversedCollection, on the other hand, conditionally conforms to the lazy protocols when its base collection does

I completely missed this, this is MUCH better.

The only method not covered by these conditional conformances is cycled(times:) due to FlattenSequence not conditionally conforming to LazySequenceProtocol, so we would need a lazy overload to keep .lazy.cycled(times:) lazy. However, since cycled(times:) is essentially just a product(0..<times, self), I think we're probably better off having it return a custom FiniteCycle type that has its own conditional LazySequenceProtocol conformance (and more efficient random-access). Does that sound reasonable?

@natecook1000
Copy link
Member

That sounds like a good solution for cycled(times:) — let’s handle that as a separate change. I didn’t realize that FlattenSequence doesn’t forward down to its base collection’s index offsetting methods. Seems like something that we could improve!

@timvermeulen
Copy link
Contributor Author

Note that Product2 only needs to traverse either of its base collections at most once when doing index calculations, whereas returning a FlattenSequence gets rid of the information that the inner collection repeats, so making it a custom type is beneficial either way.

@timvermeulen timvermeulen changed the title Add overloads to LazySequenceProtocol that maintain laziness Add conditional LazySequenceProtocol conformance Oct 22, 2020
@natecook1000
Copy link
Member

Looks great, @timvermeulen! 🎉

@natecook1000 natecook1000 merged commit 53354f3 into apple:main Oct 22, 2020
@timvermeulen timvermeulen deleted the lazy-overloads branch October 22, 2020 05:06
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.

2 participants