[Second review] SE-0516: `Iterable`

I think that forcing recoverable errors to use an iterator of return types that need to be unwrapped would be fairly unfortunate, at least for my use case. The issue is that while we want to allow recoverable errors, the vast majority of users will not use them, and we would be giving up the more ergonomic throwing iterator for everybody.

I suppose we could do something where we implement LoadCommandIterator in terms of RecoverableLoadCommandIterator and expose both:

struct MachO {
  var loadCommands: LoadCommandIterator
  var recoverableLoadCommands: RecoverableLoadCommandIterator
}

It's unfortunate because we would need to expose both methods up and down the stack (my prototype ended up with about a dozen iterators between segments, sections, etc). The current behavior allows us to just expose one Iterator in each location, normal users get the ergonomic form, and advanced users use an adapter to get result types.

1 Like

Since we are stuck with the baggage of C's definition for undefined behavior, should we borrow its solution to this problem and use the term unspecified?

My preferred spelling of this would be along these lines, which would have a bonus of clarifying whether you want to continue iteration or not:

for try loadCommand in macho.loadCommands {
    processLoadCommand(loadCommand)
} catch {
    handleError(error)
    continue
}

Similar to guard, an explicit break, continue, or other scope exiting statement is required at the end of the catch block. continue will go back to the iterator to continue past the error, while any other scope-exiting statement will tear down the iterator and exit the loop. This could still be useful for iteration over Sequences, since it would also handle errors thrown in the main loop body as well.

4 Likes

So why include them at all then?

3 Likes

It's trivial to do but it doesn't mean you always should. Using a borrowing iterator is going to make iteration slower whenever the loop consumes the value (with an extra copy for that consumption).

So if at some point in the future the compiler starts to prefer Iterable over Sequence when desugaring for-in loops, types having adopted Iterable while producing temporaries will suffer a performance regression (for loops that consume the values).

I'd say 3 years is extremely optimistic. This especially on Apple platforms where the new protocols aren't going to be available on older OS versions. This will keep pressure in the ecosystem to keep Sequence supported for a long time, at least until Xcode drops support for all OS versions without the new protocols.

A note on terminology

Using the name Iterable for this protocol is also going make it difficult to talk about the concept of "types you can iterate on". A type conforming ot Sequence is iterable but is not Iterable. In my last post I used "sequence/iterable" to refer to types you can use with for-in because there's no better way to call them without causing confusion.

My evaluation is that we should relegate Sequence as the beginner-friendly legacy protocol while adding BorrowingSequence, ConsumingSequence, etc. as the better ones for performance and non-copyable types. Perhaps, in time, plain Sequence will fade away, but I doubt causing confusion by changing the terminology will make things go smoother.

8 Likes

Performance burden? You put the iterator into its end-of-iteration state before you throw. That's not exactly a high overhead. And it's probably the least complex part of a throwing, borrowing bulk iteration operation.

(Edit to add: Note that if your iterator is wrapping another IterableIterator, you can call underlyingIterator.skip(by: .max) to fast-forward to its end-of-iteration state.)

If we do not specify a behavior but most conformers behave in a certain way, then some clients of iterators will end up expecting the common behavior and will be unable to correctly use iterators that behave differently. The behavior least likely to cause clients to misbehave is for iterators to return the end-of-iteration sentinel on repeated calls.

That was exactly the logic that led the Core Team a decade ago to accept SE-0052, which retroactively imposed a repeated-nil requirement on IteratorProtocol.next(). (And yes, the most common behavior they were banning was trapping on repeated next() calls, but the proposal also discusses iterators that repeat the sequence after it ends—a pretty similar concept to this one—and concludes that they should be banned, too.)

6 Likes

borrowing get throws is indeed supported!

1 Like

I was thinking about borrow throws (i.e. a borrow accessor, not a get accessor).

Possibly a tangent, but in case it helps evaluate your suggestion… what’s the difference between borrowing get throws and borrow throws? The caller always has to have a least an immutable borrow of the callee to call any of its property accessors, so borrowing can’t describe the callee, can it?

borrowing get throws means that self is explicitly passed by borrow and returns an owned value (or a value at +1). In fact you can pretty much just remove borrowing here as its the default convention for most scenarios. borrow throws means that it returns a borrowed value (or a value at +0). Imagine borrow throws as get throws but it returns Ref<T> instead of T.

3 Likes

Types like Array can do this, but adaptor/wrapper types can't, since their end state is driven by the thing they are wrapping. And those are the very things that tend to throw (iterating pure memory-based collections has no reason to throw, but "do this potentially throwing map operation on the elements of this underlying sequence" can).

That is not guaranteed to be free, and indeed might have effects (e.g. a lazy filtering wrapper still needs to execute the filter to skip, unless it special cases .max which seems unwise). Wrappers should not do this.

I don't think SE-0052 is quite the same. Producing more elements after indicating "I'm done" via a nil value has no good justification that I can think of. And wrappers don't need to track it, just rely on the thing they're wrapping conforming to the spec, which is a fair choice.

This is not like that, because there is a perfectly reasonable reason to throw, but still allow further iteration. Demanding all types do it seems unreasonable.

Even given this, I would push back on SE-0052 as precedent because it's really not a good proposal IMO. It gives almost no justification for why this proposal is good other than vague notions of safety – and I think this is probably insufficient to justify the branching cost.

Now, things were different in those days. Swift was nowhere near as competitive on performance at the time, so maybe the extra branch in the greater scheme was no big deal. This is no longer the case. Now, Swift has reached the point where it can be reasonably described as competitive with C, with the exception of the cost of safety – true memory safety of things like bounds checks. Where it imposes costs that are not memory safety driven – like checked arithmetic – it offers another option i.e. explicit choice of wrapping or saturating arithmetic, which can mitigate the cost.[1] Would there be a similar "it's not important that iteration continue" choice for users here? I fear there isn't, if this requirement is enshrined in the specification. This is why I think it's unnecessary, especially unless real concrete description of harm can be cited. Right now the only argument I've seen is consistency-based, and I think that's insufficient.


  1. Even in the case of memory safety, it offers things like unchecked bounds. ↩︎

3 Likes

I can imagine cases where the index is needed when handling the error, for example:

for do try (index, loadCommand) in macho.loadCommands.enumerated() {
  processLoadCommand(loadCommand)
} catch {
  handleError(error, atIndex: index) // index is unavailable here
}

But as shown, index is not in scope within the catch block. This makes the borrow throwssuggestion from @beccadax feel more approachable and practical becuase it would allow the error handler to retain context about which element caused the failure.

3 Likes

Okay, here's another one.

There is a class of iterator clients, like the iterators implementing lazy maps and filters, which are intended to apply a behavior to an arbitrary iterator. To ensure they can correctly wrap any iterator, they have to make maximally pessimistic assumptions about the underlying iterator's behavior. That is, if any iterator can resume after throwing, then all of these arbitrary-wrapping iterators need to do so, or at least to behave correctly in its presence.

What overheads will be imposed on, for instance, our lazy map and lazy filter operations if they need to support resumption? Are we willing to pay that cost in order to permit a behavior that even you are arguing is so niche that we don't need to support it in the for loop?

Or do you disagree with my premise and think it's okay to have maps and filters behave in buggy ways, like skipping the remainder of the current span after an error is thrown, if they're combined with them? Because that sounds to me like a "feature" that's so broken we ought not to pretend that we support it.

5 Likes

Do we plan to add an async version of this as a followup? It would be great to have something more performant than the async byte sequences we use now.

I get where you are coming from here, but I want to push back a bit on the claims that this is a just a niche behavior... I agree that it is a niche behavior for in memory Spans that form the object storage of ADTs, which I agree is its normal use of Iterable, but Iterable is also very important for people writing code that cannot use allocators.

My interest in this feature is not because nextSpan will allow more efficient Array iteration (I am all for that and think it is very important, but that is not why I am participating in this thread). My reason for getting involved is that the code I write needs to be run without an allocator, which is incompatible with Sequence and requires the changes being brought in by BorrowingSequence/Iterable. I care because writing code without for loops is incredibly painful, and this is the feature that enables for loops.

In many ways it is a shame these two concerns are intermingled... 90% of the conformances I wrote bringing up a a MachO parser have a nextSpan that returns one a single object span because I have to fake an object in memory due to the fact that the format has variable length fields and parsing them ahead of time is often non-trivial... IOW most of the performance concerns this proposal is trying to address are not relevant to my use case, but I still desperately want it to land.

I bring that up because I suspect this is going to be the case for everyone writing non-allocating embedded code, which is a niche, but one that is significant enough that there is an entire language mode to support it. And I bring that up because non-allocating code has some interesting properties:

  1. It is going to have more custom conformances to this than most other code, since most of the stdlib ADTs don't work and it is the only way to get working for loops
  2. Failures there are usually particularly harsh... non-allocating code is used in firmwares, dynamic linkers, the guts of allocators, etc. In a lot of those places we need to handle errors, and ergonomic error handling is a big win in so many ways.

While I have preferences with respect to resumable errors, I honestly don't care (well I care, but would be happy to accept) if I need to handle those as result types, they are a niche within error handling. But when there is no consensus and people keep referring to throwing iterators as "niche" I worry that becomes a reason to punt it entirely and never come back to it, which I think really would be a shame for non-allocating embedded swift users.

6 Likes

A function that has a generic error type would need to receive an additional pointer parameter to the out location for that error, but since Never has size zero and can never be dereferenced, in a caller context where the error is known to be Never, the caller does not need to allocate any additional stack space, and should be able to allow an arbitrary undefined value to be left in the parameter register. There is also no need for the caller to check for error returns, so there should be no additional cost.

Similarly, on the callee side, when the implementation concretely produces Never as its error type, the calling convention will still use up a parameter register for the error address, but the implementation will never write to that address. However, generic wrappers that pass errors through their implementations and can't be specialized would need to check for errors and potentially forward the error value when calling down to the inner wrapped implementation, so there may be some overhead in that layer.

6 Likes

Thanks everyone for all of the review discussion so far! The Language Steering Group discussed this yesterday. On the topic of end-of-iteration and post-throw policy, we believe the best tradeoff is to specify:

  1. Once the iterator returns an empty span, all subsequent calls to nextSpan must also return an empty span.
  2. After an iterator throws an error, callers are expected to not call nextSpan again. This avoids a need to accept additional overhead in generic iterators that abstract over both throwing and non-throwing iterator types while preserving predictable semantics across Iterable types. Use cases that need to support recoverable errors can instead model that using a non-throwing iterator of Results.

I'm extending the review until June 30th to discuss this. Please continue the discussion in this thread!

Holly Borla
Review Manager

13 Likes

A minor nitpick:

extension Span: Iterable
   where Self: ~Copyable & ~Escapable, Element: ~Copyable

extension MutableSpan: Iterable
   where Self: ~Copyable & ~Escapable, Element: ~Copyable

extension InlineArray: Iterable
   where Self: ~Copyable & ~Escapable, Element: ~Copyable

The requirement on Self is redundant in each of these three cases. It's not wrong, just confusing, so I would suggest removing it.

1 Like

Without Self: ~Copyable & ~Escapable, the extension would only apply to Copyable and Escapable conformers (at least in the case of InlineArray).

1 Like

Actually I checked, and we reject this with an error:

protocol P1: ~Copyable {}

extension InlineArray: P1 where Self: ~Copyable & ~Escapable, Element: ~Copyable {}
ext.swift:3:37: error: type 'InlineArray<count, Element>' in conformance requirement does not refer to a generic parameter or associated type
1 | protocol P1: ~Copyable {}
2 | 
3 | extension InlineArray: P1 where Self: ~Copyable & ~Escapable, Element: ~Copyable {}
  |                                     `- error: type 'InlineArray<count, Element>' in conformance requirement does not refer to a generic parameter or associated type
4 | 

This makes the behavior inconsistent between "inverse requirements" and ordinary requirements, because for example you can state where Int: Hashable in a where clause, and it is simply ignored.

In any case, the proposal should be updated to remove these requirements.