apple / swift-algorithms

Commonly used sequence and collection algorithms for Swift
Apache License 2.0
5.97k stars 443 forks source link

Add conditional LazySequenceProtocol conformance #29

Closed timvermeulen closed 4 years ago

timvermeulen commented 4 years ago

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:

Checklist

natecook1000 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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.

natecook1000 commented 4 years ago

Looks great, @timvermeulen! 🎉