[Stdlib] Implement sequence(first:next:) and sequence(state:next:) #2717

lilyball · 2016-05-26T05:17:09Z

What's in this pull request?

This implements SE-0094 (sequence(first:next:) and sequence(state:next:)).

Resolved bug number: (SR-1622)

Before merging this pull request to apple/swift repository:

Resolve the unit test errors (see PR comments).
Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
All supported platforms	@swift-ci Please smoke test and merge
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
All supported platforms	@swift-ci Please test and merge
OS X platform	@swift-ci Please test OS X platform
OS X platform	@swift-ci Please benchmark
Linux platform	@swift-ci Please test Linux platform

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

lilyball · 2016-05-26T05:18:53Z

I'm submitting this now to get eyes on it, because I'm getting some very strange errors trying to compile the included unit test:

/Users/kevin/Dev/Swift/Apple/swift/test/1_stdlib/UnfoldSequence.swift:9:14: error: ambiguous reference to member 'sequence(first:next:)'
    let s0 = sequence(state: 0, next: { $0 += 1; return $0 < 10 ? $0 : nil })
             ^~~~~~~~
Swift.sequence:31:13: note: found this candidate
public func sequence<T>(first: T, next: (T) -> T?) -> UnfoldSequence<T, (T?, Bool)>
            ^
Swift.sequence:27:13: note: found this candidate
public func sequence<T, State>(state: State, next: (inout State) -> T?) -> UnfoldSequence<T, State>
            ^
/Users/kevin/Dev/Swift/Apple/swift/test/1_stdlib/UnfoldSequence.swift:15:76: error: missing argument for parameter #1 in call
    let s2 = sequence(state: (1...5).makeIterator(), next: { return $0.next() })
                                                                           ^
/Users/kevin/Dev/Swift/Apple/swift/test/1_stdlib/UnfoldSequence.swift:19:14: error: ambiguous reference to member 'sequence(first:next:)'
    var s3 = sequence(state: 0, next: { calls += 1; $0 += 1; return $0 })
             ^~~~~~~~
Swift.sequence:31:13: note: found this candidate
public func sequence<T>(first: T, next: (T) -> T?) -> UnfoldSequence<T, (T?, Bool)>
            ^
Swift.sequence:27:13: note: found this candidate
public func sequence<T, State>(state: State, next: (inout State) -> T?) -> UnfoldSequence<T, State>
            ^

For the first and third errors, I have no clue why it's considering this call to be ambiguous with candidates sequence(first:next:) and sequence(state:next:). The two methods have different external parameter names, so they're definitely not ambiguous.

For the second error, I can't figure out why it's saying I'm missing a parameter for argument #1 in the call to $0.next(). $0 is an IteratorProtocol instance, and its next() method does not take any parameters.

gribozavr · 2016-05-26T05:23:51Z

test/1_stdlib/UnfoldSequence.swift

+        expectEqual(i, s1.next())
+        expectEqual(i, calls)
+    }
+}


+ runAllTests()

lilyball · 2016-05-26T05:27:12Z

If I rewrite line 15 (the second error) to the following:

let s2 = sequence(state: (1...5).makeIterator(), next: { (iter: inout CountableRange<Int>.Iterator) -> Int? in return iter.next() })

I get the equally-bizarre error

/Users/kevin/Dev/Swift/Apple/swift/test/1_stdlib/UnfoldSequence.swift:15:60: error: generic parameter 'Elements' could not be inferred
    let s2 = sequence(state: (1...5).makeIterator(), next: { (iter: inout CountableRange<Int>.Iterator) -> Int? in return iter.next() })

The only type involved with a generic parameter named Elements is the IndexingIterator (the CountableRange<Int>.Iterator), but its Elements parameter isn't inferred in this expression, it's well-defined. I get the same error if I use the type IndexingIterator<CountableRange<Int>> as well.

lilyball · 2016-05-26T05:28:15Z

stdlib/public/core/GroupInfo.json

@@ -49,6 +49,7 @@
    "Indices.swift",
    "Existential.swift",
    "WriteBackMutableSlice.swift",
+    "UnfoldSequence.swift",


I have no clue at all what GroupInfo.json is. I stuck this in here because the compiler complained otherwise. I hope I put it in the right place.

This is the right place, thanks.

lilyball · 2016-05-26T05:30:35Z

Turns out the second error stems from the fact that ... apparently no longer returns an iterable range type, it only returns ClosedRange, and I have to use ..< to get CountableRange. This is very surprising, why did we remove the ability to use ... with iteration? (If it's not obvious, I haven't had time to pay a lot of attention to Swift 3 changes so I missed stuff like this).

gribozavr · 2016-05-26T05:32:31Z

stdlib/public/core/UnfoldSequence.swift

+        return _next(&_state)
+    }
+
+    internal init(state: State, next: (inout State) -> Element?) {


Internal inits should have an underscore:

internal init(_state state: State, next: (inout State) -> Element?)

lilyball · 2016-05-26T05:33:16Z

I take it back, even if I use (1..<6).makeIterator(), I still get the second error. The only way to fix it is to provide the full (iter: inout CountableRange<Int>.Iterator) type in the closure expression. This is very weird.

gribozavr · 2016-05-26T05:33:29Z

stdlib/public/core/UnfoldSequence.swift

+/// - Returns: A sequence that yields each successive value from `next.
+///
+/// - SeeAlso: `sequence(first:next:)`
+public func sequence<T, State>(state: State, next: (inout State) -> T?) -> UnfoldSequence<T, State> {


80 columns?

public func sequence<T, State>(state: State, next: (inout State) -> T?) -> UnfoldSequence<T, State> { ... }

Also, T => Element? (Here and in other APIs)

Regarding T => Element, I checked before writing this and our functions like map still use type parameters that look like T. My impression was that we renamed T to Element, etc when used as parameters to types themselves, but left them as T on the functions that return the types, because the type here is not defined as "the element of the sequence", but is instead defined as e.g. "the return value of the closure", and then the resulting sequence uses that return value for its elements.

I think this is the right call—repeatElement is probably the most similar existing function to sequence. It uses T, as well, to generate a Repeated<T> instance, while Repeated has an Element generic parameter.

lilyball · 2016-05-26T05:35:28Z

Turns out that providing a full type signature for the closure fixes the first and third errors too. This is troubling. Why is the compiler unable to figure out the type involved without the full signature, and why is the error message so bad?

lilyball · 2016-05-26T05:42:56Z

Oh great. Adding the full type signatures to all the closures now triggers a compiler crash ("Unhandled conversion to exploded tuple").

lilyball · 2016-05-26T05:44:36Z

Ah, that can be solved by replacing { _ in return 1 } with { (_: inout ()) -> Int? in return 1 }.

lilyball · 2016-05-26T06:36:59Z

@gribozavr I've updated the PR with all of your suggestions, save for T => Element. See my rationale why. If you still think it should be changed despite the reasons I gave, I'm willing to do so.

natecook1000 · 2016-05-26T16:13:14Z

stdlib/public/core/UnfoldSequence.swift

+
+/// A sequence whose elements are produced via repeated applications of a
+/// closure to some mutable state. The elements are computed lazily and the
+/// sequence may potentially be infinite in length.


The abstract should be a single sentence. Can you break this up? It also might help to state how you get an UnfoldSequence, since there's no initializer.

I figured the - SeeAlso: line was the hint for how to get this, but I can certainly be more explicit.

lilyball · 2016-05-27T03:57:02Z

@gribozavr PR updated. I put the checkSequence stuff in a validation test because it appears that normal tests aren't supposed to be able to use StdlibCollectionUnittest. The validation test currently fails due to the issue I mentioned in this comment.

gribozavr · 2016-05-27T05:35:04Z

test/1_stdlib/UnfoldSequence.swift

@@ -0,0 +1,48 @@
+// RUN: %target-run-simple-swift


Do we need two test files? Could we move everything into validation?

We could, but validation tests are run much less often so I kind of assumed anything that is reasonable to put into the normal test suite, should be put into the normal test suite. Am I wrong?

After giving it another minute's thought, I suppose this is fine. StdlibCollectionUnittest isn't exposed to normal tests, so it's probably safe to assume most of the sequence/collection tests are already validation tests, so I guess it wouldn't be unusual to put all of the sequence(...) tests into the validation test file.

Fixes [SR-1622](https://bugs.swift.org/browse/SR-1622).

lilyball · 2016-05-27T20:04:13Z

@gribozavr PR updated

gribozavr · 2016-05-27T21:47:12Z

@swift-ci Please test and merge

gribozavr · 2016-05-28T01:32:17Z

The failures seem to be unrelated.

lattner · 2016-05-28T21:46:45Z

Nice, thanks @kballard!

gribozavr reviewed May 26, 2016
View reviewed changes

lilyball reviewed May 26, 2016
View reviewed changes

gribozavr reviewed May 26, 2016
View reviewed changes

lilyball force-pushed the sequence-first-state-next branch from 1ec085f to 89e881c Compare May 26, 2016 06:35

natecook1000 reviewed May 26, 2016
View reviewed changes

lilyball force-pushed the sequence-first-state-next branch from 89e881c to 97ce1db Compare May 27, 2016 03:56

gribozavr reviewed May 27, 2016
View reviewed changes

[Stdlib] Implement sequence(first:next:) and sequence(state:next:)

d1d0e05

Fixes [SR-1622](https://bugs.swift.org/browse/SR-1622).

lilyball force-pushed the sequence-first-state-next branch from 97ce1db to d1d0e05 Compare May 27, 2016 20:03

gribozavr merged commit 87115f8 into swiftlang:master May 28, 2016

lilyball deleted the sequence-first-state-next branch May 28, 2016 02:28

lilyball mannequin mentioned this pull request May 26, 2016

[SR-1622] [SE-0094] Implement sequence(first:next:) and sequence(state:next:) #44231

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Stdlib] Implement sequence(first:next:) and sequence(state:next:) #2717

[Stdlib] Implement sequence(first:next:) and sequence(state:next:) #2717

lilyball commented May 26, 2016 •

edited

Loading

lilyball commented May 26, 2016

gribozavr May 26, 2016 •

edited

Loading

lilyball commented May 26, 2016 •

edited

Loading

lilyball May 26, 2016

gribozavr May 26, 2016

lilyball commented May 26, 2016

gribozavr May 26, 2016 •

edited

Loading

lilyball commented May 26, 2016

gribozavr May 26, 2016

gribozavr May 26, 2016 •

edited

Loading

lilyball May 26, 2016

natecook1000 May 26, 2016

lilyball commented May 26, 2016

lilyball commented May 26, 2016

lilyball commented May 26, 2016

lilyball commented May 26, 2016

natecook1000 May 26, 2016

lilyball May 27, 2016

lilyball commented May 27, 2016

gribozavr May 27, 2016

lilyball May 27, 2016

lilyball May 27, 2016

lilyball commented May 27, 2016

gribozavr commented May 27, 2016

gribozavr commented May 28, 2016

lattner commented May 28, 2016

[Stdlib] Implement sequence(first:next:) and sequence(state:next:) #2717

[Stdlib] Implement sequence(first:next:) and sequence(state:next:) #2717

Conversation

lilyball commented May 26, 2016 • edited Loading

What's in this pull request?

Resolved bug number: (SR-1622)

lilyball commented May 26, 2016

gribozavr May 26, 2016 • edited Loading

Choose a reason for hiding this comment

lilyball commented May 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilyball commented May 26, 2016

gribozavr May 26, 2016 • edited Loading

Choose a reason for hiding this comment

lilyball commented May 26, 2016

Choose a reason for hiding this comment

gribozavr May 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilyball commented May 26, 2016

lilyball commented May 26, 2016

lilyball commented May 26, 2016

lilyball commented May 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilyball commented May 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilyball commented May 27, 2016

gribozavr commented May 27, 2016

gribozavr commented May 28, 2016

lattner commented May 28, 2016

lilyball commented May 26, 2016 •

edited

Loading

gribozavr May 26, 2016 •

edited

Loading

lilyball commented May 26, 2016 •

edited

Loading

gribozavr May 26, 2016 •

edited

Loading

gribozavr May 26, 2016 •

edited

Loading