Removing enumerated?


(Chris Eidhof) #1

Hey everyone,

I've organized a number of Swift workshops over the last two years. There
are a couple of things that keep coming up, and a couple of mistakes that I
see people making over and over again. One of them is that in almost every
workshop, there's someone who thinks that `enumerated()` returns a list of
(index, element) pairs. This is only true for arrays. It breaks when using
array slices, or any other kind of collection. In our workshops, I
sometimes see people doing something like `x.reversed().enumerated()`,
where `x` is an array, and somehow it produces behavior they don't
understand.

A few ways I think this could be improved:

- Move enumerated to Array
- Change enumerated to return `(Index, Iterator.Element)` (this would mean
we at least need to move it to collection)
- Remove enumerated
- Keep things as is

In any case, just wanted to share my experience (gained from teaching
people).

···

--
Chris Eidhof


Add forEachWithIndex to Array
(Ben Cohen) #2

I think whether enumerated() is justified as a method on Sequence is one example of a wider question which definitely needs some discussion, which is: where should the standard library draw the line in providing convenience functions that can easily be composed from other functions in the std lib? Here’s another example:

SE-100 <https://github.com/apple/swift-evolution/blob/master/proposals/0100-add-sequence-based-init-and-merge-to-dictionary.md> is a proposal to add an init to Dictionary from a sequence of key/value pairs. It’s a commonly requested feature, and IMO much needed and should be added as soon as we move to the appropriate phase in Swift’s evolution.

Another commonly requested Dictionary feature is similar: add a Dictionary.init that takes a sequence, and a closure that maps that sequence to keys. This is useful, for example, when you have a sequence of objects that you frequently need to index into via one property on those objects, so you want to build a fast lookup cache using that property.

Now, if we implement SE-100, that second request can be easily composed. It would be something like Dictionary(sequence.lazy.map { (key: $0.someProperty, value: $0) } )

Some people look at that line of code and think sure, that’s what I’d do and it’s easy enough that the second helper shouldn’t be added as it’s superfluous. Others look at it and say that it is unreadable clever-code FP nonsense, and we should just add the helper method because most programmers wouldn’t be able to read or write that easily.

As we expand (and maybe contract :slight_smile: the standard library, this kind of question comes up a lot, so it is worth setting out some criteria for judging these “helper” methods. Here’s my take on such a list (warning: objectivity and subjectivity blended together in the below).

1. Is it truly a frequent operation?

The operation needs to carry its weight. Even once we have ABI stability, so the size of the std lib becomes less of a concern as it could ship as part of the OS, we still need to keep helper method growth under control. APIs bristling with methods like an over-decorated Xmas tree are bad for usability. As mentioned in the String manifesto, String+Foundation currently has over 200 methods/properties. Helpers are no good if you can’t find them to use them.

Someone mentioned that they actually don’t find themselves using enumerated() all that often. I suspect enumerated in its current form isn’t all that useful. In a quick (and non-scientific) review of search results for its use on GitHub, nearly all the examples I see of it are either 1) misuse – see more below, or 2) use of it to perform the equivalent of in-place map where the index is used to subscript into the array and replace it with a transformed element.

I think the std lib needs an in-place map, and if enumerated() is removed, this one most-common use case should be added at the same time.

2. Is the helper more readable? Is the composed equivalent obvious at a glance?

When an operation is very common, the big win with a helper method is it creates a readable vocabulary. If enumerated() is very common, and everyone sees it everywhere, it makes that code easier to read at a glance.

That said, I think that the alternative – zip(0…, sequence) – is just as readable once you are familiar with zip and ranges, two concepts that, IMO at least, it is important that every Swift programmer learn about early on. I would even go so far as to say that enumerated is harmful if it discourages new users from discovering zip.

OTOH, an example that I think is strongly justified on readability grounds is Sequence.all(match:), a function that checks that every element in a sequence matches a predicate. This is by far the most common extension for Sequence on GitHub. Yes, you can write this as !seq.contains(!predicate) but that far less readable – especially since it requires you write it with a closure that !’s the predicate i.e. !seq.contains { !predicate($0) }, because we don’t have a ! for composing with predicate functions (though maybe we should).

3. Does the helper have the flexibility to cover all common cases?

This, I think, is where enumerated() really starts to fall down.

Sometimes you want to start from not-zero, so maybe it should be Sequence.enumerated(startingFrom: Int = 0)
Sometimes you want non-integer indices so maybe we should add indexed().
Sometimes you want it the other way around (not for a for loop but for passing the elements directly into a mapping function) so now you need a flip.
Sometimes you want to enumeration to run backwards in some way.

Less of a problem if you’re already accustomed to composing your enumeration:

Enumerate starting from 1: zip(1…, c)
Enumerate indices: zip(c.indices, c)
Need the pair to be the other way around: zip(c, 0…)
Need the enumeration to run the other way: zip((0..<c.count).reversed,c) or zip(0…,c).reversed() or zip(0…,c.reversed()).

Similarly, for the Dictionary helper – what if you want a sequence, and a closure to map keys to values, rather than values to keys?

(zip also needs to retain its collection-ness, which is filed as SR-3760, a great starter proposal if anyone wants to take it on :slight_smile:

4. Is there a correctness trap with the composed equivalent? Is there a correctness trap with the helper?

As others noted on the thread, enumerated() used for indices encourages a possible correctness error:

for (idx, element) = someSlice.enumerated() { } // slices aren’t zero based!

Now, chances are that since out-of-bounds subscripting traps on arrays, this bug would be caught early on. But maybe not – especially if you ended up using it on an UnsafeBufferPointer slice (perhaps dropped in for performance reasons to replace an Array) and now all of a sudden you have a memory access violation that could go undetected.

On the flip side: many operations on collections that use indices are hard to get right, especially ones that involve mutating as you go, like removing elements from a collection based on some criteria, where you have to work around index invalidation or fencepost errors. For this reason, the std lib probably ought to have a RangeReplaceableCollection.remove(where:) method so people don’t have to reinvent it and risk getting it wrong.

5. Is there a performance trap with the composed equivalent? Or with the helper?

The composed example of Dictionary from a sequence+closure, you need to remember the .lazy part in sequence.lazy.map to avoid creating a temporary array for no reason. A helper method might lift that burden from the user. remove(where:) can easily be accidentally quadtratic, or perform needless element copies when there’s little or nothing to remove.

Counter example: the fact that map is eager and chaining it creates temporary arrays needlessly is a performance problem caused by introducing it as a helper method.

6. Does the helper actually encourage misuse?

This is a very common pattern when searching GitHub for uses of enumerated():

for (index, _) in collection.enumerated() {
    mutate collect[index] in-place i.e. collection[index] += 1
}.

(or even they assign the element then don’t use it, for which specific case we don’t currently emit a warning)

What the user really needs is just:

for index in collection.indices { etc. }

I expect if the standard way to do enumerated was with zip, people wouldn’t do this as often. In-place map would be even better.

8. Can a native implementation be made more performant than the equivalent composition?

Dictionary.mapValues(transform: (Value)->T) can be implemented internally much more efficiently than just composing it with map and the key/value sequence initializer, because the layout of the hash table storage can be re-used in the new dictionary, even when the Value type is different.

···

On Jan 31, 2017, at 6:24 AM, Chris Eidhof via swift-evolution <swift-evolution@swift.org> wrote:

Hey everyone,

I've organized a number of Swift workshops over the last two years. There are a couple of things that keep coming up, and a couple of mistakes that I see people making over and over again. One of them is that in almost every workshop, there's someone who thinks that `enumerated()` returns a list of (index, element) pairs. This is only true for arrays. It breaks when using array slices, or any other kind of collection. In our workshops, I sometimes see people doing something like `x.reversed().enumerated()`, where `x` is an array, and somehow it produces behavior they don't understand.

A few ways I think this could be improved:

- Move enumerated to Array
- Change enumerated to return `(Index, Iterator.Element)` (this would mean we at least need to move it to collection)
- Remove enumerated
- Keep things as is

In any case, just wanted to share my experience (gained from teaching people).

--
Chris Eidhof
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Ben Cohen) #3

I think whether enumerated() is justified as a method on Sequence is one example of a wider question which definitely needs some discussion, which is: where should the standard library draw the line in providing convenience functions that can easily be composed from other functions in the std lib? Here’s another example:

SE-100 <https://github.com/apple/swift-evolution/blob/master/proposals/0100-add-sequence-based-init-and-merge-to-dictionary.md> is a proposal to add an init to Dictionary from a sequence of key/value pairs. It’s a commonly requested feature, and IMO much needed and should be added as soon as we move to the appropriate phase in Swift’s evolution.

Another commonly requested Dictionary feature is similar: add a Dictionary.init that takes a sequence, and a closure that maps that sequence to keys. This is useful, for example, when you have a sequence of objects that you frequently need to index into via one property on those objects, so you want to build a fast lookup cache using that property.

Now, if we implement SE-100, that second request can be easily composed. It would be something like Dictionary(sequence.lazy.map { (key: $0.someProperty, value: $0) } )

Some people look at that line of code and think sure, that’s what I’d do and it’s easy enough that the second helper shouldn’t be added as it’s superfluous. Others look at it and say that it is unreadable clever-code FP nonsense, and we should just add the helper method because most programmers wouldn’t be able to read or write that easily.

As we expand (and maybe contract :slight_smile: the standard library, this kind of question comes up a lot, so it is worth setting out some criteria for judging these
“helper” methods. Here’s my take on such a list (warning: objectivity and subjectivity blended together in the below).

This is a great analysis and list of criteria. Thanks for putting this together Ben!

1. Is it truly a frequent operation?

The operation needs to carry its weight. Even once we have ABI stability, so the size of the std lib becomes less of a concern as it could ship as part of the OS, we still need to keep helper method growth under control. APIs bristling with methods like an over-decorated Xmas tree are bad for usability. As mentioned in the String manifesto, String+Foundation currently has over 200 methods/properties. Helpers are no good if you can’t find them to use them.

Someone mentioned that they actually don’t find themselves using enumerated() all that often. I suspect enumerated in its current form isn’t all that useful. In a quick (and non-scientific) review of search results for its use on GitHub, nearly all the examples I see of it are either 1) misuse – see more below, or 2) use of it to perform the equivalent of in-place map where the index is used to subscript into the array and replace it with a transformed element.

I think the std lib needs an in-place map, and if enumerated() is removed, this one most-common use case should be added at the same time.

I just did a quick search in a couple of projects and found a handful of good examples of valid uses of `enumerated`. I have simplified the examples from real world code, but I think they demonstrate the kinds of things this is good for.

// only show the first 5 views
for (i, view) in views.enumerated() {
    view.hidden = i >= 5
}

Interesting that several of these rely on reference types as elements. This does make the loops a lot simpler. In-place amendment of an array would be messier, and the combination with zip definitely pushes back against my notion that an in-place map would be a useful alternative (since you couldn’t mutate the mutable half of the zipped collection in-place without us jumping through some hoops).

// apply alternating view background colors
for (i, view) in views.enumerated() {
    view.backgroundColor = i % 2 ? bgColor1 : bgColor2
}

This is an interesting one because it highlights something else I think is missing from the std lib: Sequence.cycle, which would make an infinite sequence by repeating a sequence, enabling what I think is probably a readability win in some cases:

let colors = [bgColor1, bgColor2].cycle
for (color, view) in zip(colors, views) {
  view.backgroundColor = color
}

Also i%2 used as a boolean – what language is this? :slight_smile:

// linear layout
for (i, view) in views.enumerated() {
   let x = width * CGFloat(i)
   view.frame = CGRect(x: x, y: 0, width: width, height: height)
}

// deriving locations for an equally spaced gradient
let locations = colors.enumerated.map { CGFloat($0.0) / CGFloat(colors.count - 1) }

This one feels like it’s an example of enumerate discouraging use of stride, which hits cases 2: readability and 4: performance (unlike the others, where the alternatives are no better or worse)

There are other ways to accomplish similar things (use `prefix` and `dropFirst` comes to mind), but many reasonable people would argue that using `enumerated` is a very straightforward way to do a lot of things.

Totally. Enumerating things is certainly sometimes useful, I just don’t think it’s useful enough to have a dedicated but inflexible method to do it.

2. Is the helper more readable? Is the composed equivalent obvious at a glance?

When an operation is very common, the big win with a helper method is it creates a readable vocabulary. If enumerated() is very common, and everyone sees it everywhere, it makes that code easier to read at a glance.

That said, I think that the alternative – zip(0…, sequence) – is just as readable once you are familiar with zip and ranges, two concepts that, IMO at least, it is important that every Swift programmer learn about early on. I would even go so far as to say that enumerated is harmful if it discourages new users from discovering zip.

This is a very good point. Requiring programmers to zip with a range could help them to learn to think a little bit differently and learn more general tools. That’s a good thing.

The other thing it does is make the order of the index and the value more clear at the call site. I think this is a very important point. I always forget which order `enumerated` uses every time I reach to use it.

Of course the current alternative is actually `zip(0..<sequence.count, sequence)` which is not so great.

Totally. zip replacing enumerated will only fly in combination with one-sided ranges.

In order to make `zip(0…, sequence)` a workable alternative we would need to grab `…` as a unary postfix operator which produces a sequence by incrementing values. Is that something we want to do? If the answer turns out to be yes, then I am in agreement with your analysis. The composed alternative would be superior and `enumerated` wouldn’t carry it’s weight.

I think based on the String discussions there’s fairly well-established support for one-sided …

···

On Jan 31, 2017, at 12:18 PM, Matthew Johnson <matthew@anandabits.com> wrote:

On Jan 31, 2017, at 1:16 PM, Ben Cohen via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

OTOH, an example that I think is strongly justified on readability grounds is Sequence.all(match:), a function that checks that every element in a sequence matches a predicate. This is by far the most common extension for Sequence on GitHub. Yes, you can write this as !seq.contains(!predicate) but that far less readable – especially since it requires you write it with a closure that !’s the predicate i.e. !seq.contains { !predicate($0) }, because we don’t have a ! for composing with predicate functions (though maybe we should).

3. Does the helper have the flexibility to cover all common cases?

This, I think, is where enumerated() really starts to fall down.

Sometimes you want to start from not-zero, so maybe it should be Sequence.enumerated(startingFrom: Int = 0)
Sometimes you want non-integer indices so maybe we should add indexed().
Sometimes you want it the other way around (not for a for loop but for passing the elements directly into a mapping function) so now you need a flip.
Sometimes you want to enumeration to run backwards in some way.

Less of a problem if you’re already accustomed to composing your enumeration:

Enumerate starting from 1: zip(1…, c)
Enumerate indices: zip(c.indices, c)
Need the pair to be the other way around: zip(c, 0…)
Need the enumeration to run the other way: zip((0..<c.count).reversed,c) or zip(0…,c).reversed() or zip(0…,c.reversed()).

Similarly, for the Dictionary helper – what if you want a sequence, and a closure to map keys to values, rather than values to keys?

(zip also needs to retain its collection-ness, which is filed as SR-3760, a great starter proposal if anyone wants to take it on :slight_smile:

4. Is there a correctness trap with the composed equivalent? Is there a correctness trap with the helper?

As others noted on the thread, enumerated() used for indices encourages a possible correctness error:

for (idx, element) = someSlice.enumerated() { } // slices aren’t zero based!

Now, chances are that since out-of-bounds subscripting traps on arrays, this bug would be caught early on. But maybe not – especially if you ended up using it on an UnsafeBufferPointer slice (perhaps dropped in for performance reasons to replace an Array) and now all of a sudden you have a memory access violation that could go undetected.

On the flip side: many operations on collections that use indices are hard to get right, especially ones that involve mutating as you go, like removing elements from a collection based on some criteria, where you have to work around index invalidation or fencepost errors. For this reason, the std lib probably ought to have a RangeReplaceableCollection.remove(where:) method so people don’t have to reinvent it and risk getting it wrong.

5. Is there a performance trap with the composed equivalent? Or with the helper?

The composed example of Dictionary from a sequence+closure, you need to remember the .lazy part in sequence.lazy.map to avoid creating a temporary array for no reason. A helper method might lift that burden from the user. remove(where:) can easily be accidentally quadtratic, or perform needless element copies when there’s little or nothing to remove.

Counter example: the fact that map is eager and chaining it creates temporary arrays needlessly is a performance problem caused by introducing it as a helper method.

6. Does the helper actually encourage misuse?

This is a very common pattern when searching GitHub for uses of enumerated():

for (index, _) in collection.enumerated() {
    mutate collect[index] in-place i.e. collection[index] += 1
}.

(or even they assign the element then don’t use it, for which specific case we don’t currently emit a warning)

What the user really needs is just:

for index in collection.indices { etc. }

I expect if the standard way to do enumerated was with zip, people wouldn’t do this as often. In-place map would be even better.

8. Can a native implementation be made more performant than the equivalent composition?

Dictionary.mapValues(transform: (Value)->T) can be implemented internally much more efficiently than just composing it with map and the key/value sequence initializer, because the layout of the hash table storage can be re-used in the new dictionary, even when the Value type is different.

On Jan 31, 2017, at 6:24 AM, Chris Eidhof via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

Hey everyone,

I've organized a number of Swift workshops over the last two years. There are a couple of things that keep coming up, and a couple of mistakes that I see people making over and over again. One of them is that in almost every workshop, there's someone who thinks that `enumerated()` returns a list of (index, element) pairs. This is only true for arrays. It breaks when using array slices, or any other kind of collection. In our workshops, I sometimes see people doing something like `x.reversed().enumerated()`, where `x` is an array, and somehow it produces behavior they don't understand.

A few ways I think this could be improved:

- Move enumerated to Array
- Change enumerated to return `(Index, Iterator.Element)` (this would mean we at least need to move it to collection)
- Remove enumerated
- Keep things as is

In any case, just wanted to share my experience (gained from teaching people).

--
Chris Eidhof
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution


(Matthew Johnson) #4

I think whether enumerated() is justified as a method on Sequence is one example of a wider question which definitely needs some discussion, which is: where should the standard library draw the line in providing convenience functions that can easily be composed from other functions in the std lib? Here’s another example:

SE-100 <https://github.com/apple/swift-evolution/blob/master/proposals/0100-add-sequence-based-init-and-merge-to-dictionary.md> is a proposal to add an init to Dictionary from a sequence of key/value pairs. It’s a commonly requested feature, and IMO much needed and should be added as soon as we move to the appropriate phase in Swift’s evolution.

Another commonly requested Dictionary feature is similar: add a Dictionary.init that takes a sequence, and a closure that maps that sequence to keys. This is useful, for example, when you have a sequence of objects that you frequently need to index into via one property on those objects, so you want to build a fast lookup cache using that property.

Now, if we implement SE-100, that second request can be easily composed. It would be something like Dictionary(sequence.lazy.map { (key: $0.someProperty, value: $0) } )

Some people look at that line of code and think sure, that’s what I’d do and it’s easy enough that the second helper shouldn’t be added as it’s superfluous. Others look at it and say that it is unreadable clever-code FP nonsense, and we should just add the helper method because most programmers wouldn’t be able to read or write that easily.

As we expand (and maybe contract :slight_smile: the standard library, this kind of question comes up a lot, so it is worth setting out some criteria for judging these
“helper” methods. Here’s my take on such a list (warning: objectivity and subjectivity blended together in the below).

This is a great analysis and list of criteria. Thanks for putting this together Ben!

1. Is it truly a frequent operation?

The operation needs to carry its weight. Even once we have ABI stability, so the size of the std lib becomes less of a concern as it could ship as part of the OS, we still need to keep helper method growth under control. APIs bristling with methods like an over-decorated Xmas tree are bad for usability. As mentioned in the String manifesto, String+Foundation currently has over 200 methods/properties. Helpers are no good if you can’t find them to use them.

Someone mentioned that they actually don’t find themselves using enumerated() all that often. I suspect enumerated in its current form isn’t all that useful. In a quick (and non-scientific) review of search results for its use on GitHub, nearly all the examples I see of it are either 1) misuse – see more below, or 2) use of it to perform the equivalent of in-place map where the index is used to subscript into the array and replace it with a transformed element.

I think the std lib needs an in-place map, and if enumerated() is removed, this one most-common use case should be added at the same time.

I just did a quick search in a couple of projects and found a handful of good examples of valid uses of `enumerated`. I have simplified the examples from real world code, but I think they demonstrate the kinds of things this is good for.

// only show the first 5 views
for (i, view) in views.enumerated() {
    view.hidden = i >= 5
}

// apply alternating view background colors
for (i, view) in views.enumerated() {
    view.backgroundColor = i % 2 ? bgColor1 : bgColor2
}

// linear layout
for (i, view) in views.enumerated() {
   let x = width * CGFloat(i)
   view.frame = CGRect(x: x, y: 0, width: width, height: height)
}

// deriving locations for an equally spaced gradient
let locations = colors.enumerated.map { CGFloat($0.0) / CGFloat(colors.count - 1) }

There are other ways to accomplish similar things (use `prefix` and `dropFirst` comes to mind), but many reasonable people would argue that using `enumerated` is a very straightforward way to do a lot of things.

2. Is the helper more readable? Is the composed equivalent obvious at a glance?

When an operation is very common, the big win with a helper method is it creates a readable vocabulary. If enumerated() is very common, and everyone sees it everywhere, it makes that code easier to read at a glance.

That said, I think that the alternative – zip(0…, sequence) – is just as readable once you are familiar with zip and ranges, two concepts that, IMO at least, it is important that every Swift programmer learn about early on. I would even go so far as to say that enumerated is harmful if it discourages new users from discovering zip.

This is a very good point. Requiring programmers to zip with a range could help them to learn to think a little bit differently and learn more general tools. That’s a good thing.

The other thing it does is make the order of the index and the value more clear at the call site. I think this is a very important point. I always forget which order `enumerated` uses every time I reach to use it.

Of course the current alternative is actually `zip(0..<sequence.count, sequence)` which is not so great.

In order to make `zip(0…, sequence)` a workable alternative we would need to grab `…` as a unary postfix operator which produces a sequence by incrementing values. Is that something we want to do? If the answer turns out to be yes, then I am in agreement with your analysis. The composed alternative would be superior and `enumerated` wouldn’t carry it’s weight.

···

On Jan 31, 2017, at 1:16 PM, Ben Cohen via swift-evolution <swift-evolution@swift.org> wrote:

OTOH, an example that I think is strongly justified on readability grounds is Sequence.all(match:), a function that checks that every element in a sequence matches a predicate. This is by far the most common extension for Sequence on GitHub. Yes, you can write this as !seq.contains(!predicate) but that far less readable – especially since it requires you write it with a closure that !’s the predicate i.e. !seq.contains { !predicate($0) }, because we don’t have a ! for composing with predicate functions (though maybe we should).

3. Does the helper have the flexibility to cover all common cases?

This, I think, is where enumerated() really starts to fall down.

Sometimes you want to start from not-zero, so maybe it should be Sequence.enumerated(startingFrom: Int = 0)
Sometimes you want non-integer indices so maybe we should add indexed().
Sometimes you want it the other way around (not for a for loop but for passing the elements directly into a mapping function) so now you need a flip.
Sometimes you want to enumeration to run backwards in some way.

Less of a problem if you’re already accustomed to composing your enumeration:

Enumerate starting from 1: zip(1…, c)
Enumerate indices: zip(c.indices, c)
Need the pair to be the other way around: zip(c, 0…)
Need the enumeration to run the other way: zip((0..<c.count).reversed,c) or zip(0…,c).reversed() or zip(0…,c.reversed()).

Similarly, for the Dictionary helper – what if you want a sequence, and a closure to map keys to values, rather than values to keys?

(zip also needs to retain its collection-ness, which is filed as SR-3760, a great starter proposal if anyone wants to take it on :slight_smile:

4. Is there a correctness trap with the composed equivalent? Is there a correctness trap with the helper?

As others noted on the thread, enumerated() used for indices encourages a possible correctness error:

for (idx, element) = someSlice.enumerated() { } // slices aren’t zero based!

Now, chances are that since out-of-bounds subscripting traps on arrays, this bug would be caught early on. But maybe not – especially if you ended up using it on an UnsafeBufferPointer slice (perhaps dropped in for performance reasons to replace an Array) and now all of a sudden you have a memory access violation that could go undetected.

On the flip side: many operations on collections that use indices are hard to get right, especially ones that involve mutating as you go, like removing elements from a collection based on some criteria, where you have to work around index invalidation or fencepost errors. For this reason, the std lib probably ought to have a RangeReplaceableCollection.remove(where:) method so people don’t have to reinvent it and risk getting it wrong.

5. Is there a performance trap with the composed equivalent? Or with the helper?

The composed example of Dictionary from a sequence+closure, you need to remember the .lazy part in sequence.lazy.map to avoid creating a temporary array for no reason. A helper method might lift that burden from the user. remove(where:) can easily be accidentally quadtratic, or perform needless element copies when there’s little or nothing to remove.

Counter example: the fact that map is eager and chaining it creates temporary arrays needlessly is a performance problem caused by introducing it as a helper method.

6. Does the helper actually encourage misuse?

This is a very common pattern when searching GitHub for uses of enumerated():

for (index, _) in collection.enumerated() {
    mutate collect[index] in-place i.e. collection[index] += 1
}.

(or even they assign the element then don’t use it, for which specific case we don’t currently emit a warning)

What the user really needs is just:

for index in collection.indices { etc. }

I expect if the standard way to do enumerated was with zip, people wouldn’t do this as often. In-place map would be even better.

8. Can a native implementation be made more performant than the equivalent composition?

Dictionary.mapValues(transform: (Value)->T) can be implemented internally much more efficiently than just composing it with map and the key/value sequence initializer, because the layout of the hash table storage can be re-used in the new dictionary, even when the Value type is different.

On Jan 31, 2017, at 6:24 AM, Chris Eidhof via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

Hey everyone,

I've organized a number of Swift workshops over the last two years. There are a couple of things that keep coming up, and a couple of mistakes that I see people making over and over again. One of them is that in almost every workshop, there's someone who thinks that `enumerated()` returns a list of (index, element) pairs. This is only true for arrays. It breaks when using array slices, or any other kind of collection. In our workshops, I sometimes see people doing something like `x.reversed().enumerated()`, where `x` is an array, and somehow it produces behavior they don't understand.

A few ways I think this could be improved:

- Move enumerated to Array
- Change enumerated to return `(Index, Iterator.Element)` (this would mean we at least need to move it to collection)
- Remove enumerated
- Keep things as is

In any case, just wanted to share my experience (gained from teaching people).

--
Chris Eidhof
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Matthew Johnson) #5

I think whether enumerated() is justified as a method on Sequence is one example of a wider question which definitely needs some discussion, which is: where should the standard library draw the line in providing convenience functions that can easily be composed from other functions in the std lib? Here’s another example:

SE-100 <https://github.com/apple/swift-evolution/blob/master/proposals/0100-add-sequence-based-init-and-merge-to-dictionary.md> is a proposal to add an init to Dictionary from a sequence of key/value pairs. It’s a commonly requested feature, and IMO much needed and should be added as soon as we move to the appropriate phase in Swift’s evolution.

Another commonly requested Dictionary feature is similar: add a Dictionary.init that takes a sequence, and a closure that maps that sequence to keys. This is useful, for example, when you have a sequence of objects that you frequently need to index into via one property on those objects, so you want to build a fast lookup cache using that property.

Now, if we implement SE-100, that second request can be easily composed. It would be something like Dictionary(sequence.lazy.map { (key: $0.someProperty, value: $0) } )

Some people look at that line of code and think sure, that’s what I’d do and it’s easy enough that the second helper shouldn’t be added as it’s superfluous. Others look at it and say that it is unreadable clever-code FP nonsense, and we should just add the helper method because most programmers wouldn’t be able to read or write that easily.

As we expand (and maybe contract :slight_smile: the standard library, this kind of question comes up a lot, so it is worth setting out some criteria for judging these
“helper” methods. Here’s my take on such a list (warning: objectivity and subjectivity blended together in the below).

This is a great analysis and list of criteria. Thanks for putting this together Ben!

1. Is it truly a frequent operation?

The operation needs to carry its weight. Even once we have ABI stability, so the size of the std lib becomes less of a concern as it could ship as part of the OS, we still need to keep helper method growth under control. APIs bristling with methods like an over-decorated Xmas tree are bad for usability. As mentioned in the String manifesto, String+Foundation currently has over 200 methods/properties. Helpers are no good if you can’t find them to use them.

Someone mentioned that they actually don’t find themselves using enumerated() all that often. I suspect enumerated in its current form isn’t all that useful. In a quick (and non-scientific) review of search results for its use on GitHub, nearly all the examples I see of it are either 1) misuse – see more below, or 2) use of it to perform the equivalent of in-place map where the index is used to subscript into the array and replace it with a transformed element.

I think the std lib needs an in-place map, and if enumerated() is removed, this one most-common use case should be added at the same time.

I just did a quick search in a couple of projects and found a handful of good examples of valid uses of `enumerated`. I have simplified the examples from real world code, but I think they demonstrate the kinds of things this is good for.

// only show the first 5 views
for (i, view) in views.enumerated() {
    view.hidden = i >= 5
}

Interesting that several of these rely on reference types as elements. This does make the loops a lot simpler. In-place amendment of an array would be messier, and the combination with zip definitely pushes back against my notion that an in-place map would be a useful alternative (since you couldn’t mutate the mutable half of the zipped collection in-place without us jumping through some hoops).

// apply alternating view background colors
for (i, view) in views.enumerated() {
    view.backgroundColor = i % 2 ? bgColor1 : bgColor2
}

This is an interesting one because it highlights something else I think is missing from the std lib: Sequence.cycle, which would make an infinite sequence by repeating a sequence, enabling what I think is probably a readability win in some cases:

let colors = [bgColor1, bgColor2].cycle
for (color, view) in zip(colors, views) {
  view.backgroundColor = color
}

Also i%2 used as a boolean – what language is this? :slight_smile:

Oops! :slight_smile:

// linear layout
for (i, view) in views.enumerated() {
   let x = width * CGFloat(i)
   view.frame = CGRect(x: x, y: 0, width: width, height: height)
}

// deriving locations for an equally spaced gradient
let locations = colors.enumerated.map { CGFloat($0.0) / CGFloat(colors.count - 1) }

This one feels like it’s an example of enumerate discouraging use of stride, which hits cases 2: readability and 4: performance (unlike the others, where the alternatives are no better or worse)

That’s fair.

There are other ways to accomplish similar things (use `prefix` and `dropFirst` comes to mind), but many reasonable people would argue that using `enumerated` is a very straightforward way to do a lot of things.

Totally. Enumerating things is certainly sometimes useful, I just don’t think it’s useful enough to have a dedicated but inflexible method to do it.

2. Is the helper more readable? Is the composed equivalent obvious at a glance?

When an operation is very common, the big win with a helper method is it creates a readable vocabulary. If enumerated() is very common, and everyone sees it everywhere, it makes that code easier to read at a glance.

That said, I think that the alternative – zip(0…, sequence) – is just as readable once you are familiar with zip and ranges, two concepts that, IMO at least, it is important that every Swift programmer learn about early on. I would even go so far as to say that enumerated is harmful if it discourages new users from discovering zip.

This is a very good point. Requiring programmers to zip with a range could help them to learn to think a little bit differently and learn more general tools. That’s a good thing.

The other thing it does is make the order of the index and the value more clear at the call site. I think this is a very important point. I always forget which order `enumerated` uses every time I reach to use it.

Of course the current alternative is actually `zip(0..<sequence.count, sequence)` which is not so great.

Totally. zip replacing enumerated will only fly in combination with one-sided ranges.

In order to make `zip(0…, sequence)` a workable alternative we would need to grab `…` as a unary postfix operator which produces a sequence by incrementing values. Is that something we want to do? If the answer turns out to be yes, then I am in agreement with your analysis. The composed alternative would be superior and `enumerated` wouldn’t carry it’s weight.

I think based on the String discussions there’s fairly well-established support for one-sided …

Agree, but that conversation feels like it hasn’t fully resolved yet.

The primary concern I have with one-sided … is whether it would limit our options for variadics and tuple unpacking in a way we would come to regret. I know that was mentioned during the String discussion but I didn’t get a sense that this question was explored all they way through yet.

But maybe I missed some details - I haven’t been following every aspect of the String discussion and it’s possible I missed a few posts I would have been interested in, including this topic.

···

On Jan 31, 2017, at 2:46 PM, Ben Cohen <ben_cohen@apple.com> wrote:

On Jan 31, 2017, at 12:18 PM, Matthew Johnson <matthew@anandabits.com <mailto:matthew@anandabits.com>> wrote:

On Jan 31, 2017, at 1:16 PM, Ben Cohen via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

OTOH, an example that I think is strongly justified on readability grounds is Sequence.all(match:), a function that checks that every element in a sequence matches a predicate. This is by far the most common extension for Sequence on GitHub. Yes, you can write this as !seq.contains(!predicate) but that far less readable – especially since it requires you write it with a closure that !’s the predicate i.e. !seq.contains { !predicate($0) }, because we don’t have a ! for composing with predicate functions (though maybe we should).

3. Does the helper have the flexibility to cover all common cases?

This, I think, is where enumerated() really starts to fall down.

Sometimes you want to start from not-zero, so maybe it should be Sequence.enumerated(startingFrom: Int = 0)
Sometimes you want non-integer indices so maybe we should add indexed().
Sometimes you want it the other way around (not for a for loop but for passing the elements directly into a mapping function) so now you need a flip.
Sometimes you want to enumeration to run backwards in some way.

Less of a problem if you’re already accustomed to composing your enumeration:

Enumerate starting from 1: zip(1…, c)
Enumerate indices: zip(c.indices, c)
Need the pair to be the other way around: zip(c, 0…)
Need the enumeration to run the other way: zip((0..<c.count).reversed,c) or zip(0…,c).reversed() or zip(0…,c.reversed()).

Similarly, for the Dictionary helper – what if you want a sequence, and a closure to map keys to values, rather than values to keys?

(zip also needs to retain its collection-ness, which is filed as SR-3760, a great starter proposal if anyone wants to take it on :slight_smile:

4. Is there a correctness trap with the composed equivalent? Is there a correctness trap with the helper?

As others noted on the thread, enumerated() used for indices encourages a possible correctness error:

for (idx, element) = someSlice.enumerated() { } // slices aren’t zero based!

Now, chances are that since out-of-bounds subscripting traps on arrays, this bug would be caught early on. But maybe not – especially if you ended up using it on an UnsafeBufferPointer slice (perhaps dropped in for performance reasons to replace an Array) and now all of a sudden you have a memory access violation that could go undetected.

On the flip side: many operations on collections that use indices are hard to get right, especially ones that involve mutating as you go, like removing elements from a collection based on some criteria, where you have to work around index invalidation or fencepost errors. For this reason, the std lib probably ought to have a RangeReplaceableCollection.remove(where:) method so people don’t have to reinvent it and risk getting it wrong.

5. Is there a performance trap with the composed equivalent? Or with the helper?

The composed example of Dictionary from a sequence+closure, you need to remember the .lazy part in sequence.lazy.map to avoid creating a temporary array for no reason. A helper method might lift that burden from the user. remove(where:) can easily be accidentally quadtratic, or perform needless element copies when there’s little or nothing to remove.

Counter example: the fact that map is eager and chaining it creates temporary arrays needlessly is a performance problem caused by introducing it as a helper method.

6. Does the helper actually encourage misuse?

This is a very common pattern when searching GitHub for uses of enumerated():

for (index, _) in collection.enumerated() {
    mutate collect[index] in-place i.e. collection[index] += 1
}.

(or even they assign the element then don’t use it, for which specific case we don’t currently emit a warning)

What the user really needs is just:

for index in collection.indices { etc. }

I expect if the standard way to do enumerated was with zip, people wouldn’t do this as often. In-place map would be even better.

8. Can a native implementation be made more performant than the equivalent composition?

Dictionary.mapValues(transform: (Value)->T) can be implemented internally much more efficiently than just composing it with map and the key/value sequence initializer, because the layout of the hash table storage can be re-used in the new dictionary, even when the Value type is different.

On Jan 31, 2017, at 6:24 AM, Chris Eidhof via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

Hey everyone,

I've organized a number of Swift workshops over the last two years. There are a couple of things that keep coming up, and a couple of mistakes that I see people making over and over again. One of them is that in almost every workshop, there's someone who thinks that `enumerated()` returns a list of (index, element) pairs. This is only true for arrays. It breaks when using array slices, or any other kind of collection. In our workshops, I sometimes see people doing something like `x.reversed().enumerated()`, where `x` is an array, and somehow it produces behavior they don't understand.

A few ways I think this could be improved:

- Move enumerated to Array
- Change enumerated to return `(Index, Iterator.Element)` (this would mean we at least need to move it to collection)
- Remove enumerated
- Keep things as is

In any case, just wanted to share my experience (gained from teaching people).

--
Chris Eidhof
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution


(Jon Hull) #6

+1000 to all of this!

···

On Jan 31, 2017, at 11:16 AM, Ben Cohen via swift-evolution <swift-evolution@swift.org> wrote:

I think whether enumerated() is justified as a method on Sequence is one example of a wider question which definitely needs some discussion, which is: where should the standard library draw the line in providing convenience functions that can easily be composed from other functions in the std lib? Here’s another example:

SE-100 <https://github.com/apple/swift-evolution/blob/master/proposals/0100-add-sequence-based-init-and-merge-to-dictionary.md> is a proposal to add an init to Dictionary from a sequence of key/value pairs. It’s a commonly requested feature, and IMO much needed and should be added as soon as we move to the appropriate phase in Swift’s evolution.

Another commonly requested Dictionary feature is similar: add a Dictionary.init that takes a sequence, and a closure that maps that sequence to keys. This is useful, for example, when you have a sequence of objects that you frequently need to index into via one property on those objects, so you want to build a fast lookup cache using that property.

Now, if we implement SE-100, that second request can be easily composed. It would be something like Dictionary(sequence.lazy.map { (key: $0.someProperty, value: $0) } )

Some people look at that line of code and think sure, that’s what I’d do and it’s easy enough that the second helper shouldn’t be added as it’s superfluous. Others look at it and say that it is unreadable clever-code FP nonsense, and we should just add the helper method because most programmers wouldn’t be able to read or write that easily.

As we expand (and maybe contract :slight_smile: the standard library, this kind of question comes up a lot, so it is worth setting out some criteria for judging these “helper” methods. Here’s my take on such a list (warning: objectivity and subjectivity blended together in the below).

1. Is it truly a frequent operation?

The operation needs to carry its weight. Even once we have ABI stability, so the size of the std lib becomes less of a concern as it could ship as part of the OS, we still need to keep helper method growth under control. APIs bristling with methods like an over-decorated Xmas tree are bad for usability. As mentioned in the String manifesto, String+Foundation currently has over 200 methods/properties. Helpers are no good if you can’t find them to use them.

Someone mentioned that they actually don’t find themselves using enumerated() all that often. I suspect enumerated in its current form isn’t all that useful. In a quick (and non-scientific) review of search results for its use on GitHub, nearly all the examples I see of it are either 1) misuse – see more below, or 2) use of it to perform the equivalent of in-place map where the index is used to subscript into the array and replace it with a transformed element.

I think the std lib needs an in-place map, and if enumerated() is removed, this one most-common use case should be added at the same time.

2. Is the helper more readable? Is the composed equivalent obvious at a glance?

When an operation is very common, the big win with a helper method is it creates a readable vocabulary. If enumerated() is very common, and everyone sees it everywhere, it makes that code easier to read at a glance.

That said, I think that the alternative – zip(0…, sequence) – is just as readable once you are familiar with zip and ranges, two concepts that, IMO at least, it is important that every Swift programmer learn about early on. I would even go so far as to say that enumerated is harmful if it discourages new users from discovering zip.

OTOH, an example that I think is strongly justified on readability grounds is Sequence.all(match:), a function that checks that every element in a sequence matches a predicate. This is by far the most common extension for Sequence on GitHub. Yes, you can write this as !seq.contains(!predicate) but that far less readable – especially since it requires you write it with a closure that !’s the predicate i.e. !seq.contains { !predicate($0) }, because we don’t have a ! for composing with predicate functions (though maybe we should).

3. Does the helper have the flexibility to cover all common cases?

This, I think, is where enumerated() really starts to fall down.

Sometimes you want to start from not-zero, so maybe it should be Sequence.enumerated(startingFrom: Int = 0)
Sometimes you want non-integer indices so maybe we should add indexed().
Sometimes you want it the other way around (not for a for loop but for passing the elements directly into a mapping function) so now you need a flip.
Sometimes you want to enumeration to run backwards in some way.

Less of a problem if you’re already accustomed to composing your enumeration:

Enumerate starting from 1: zip(1…, c)
Enumerate indices: zip(c.indices, c)
Need the pair to be the other way around: zip(c, 0…)
Need the enumeration to run the other way: zip((0..<c.count).reversed,c) or zip(0…,c).reversed() or zip(0…,c.reversed()).

Similarly, for the Dictionary helper – what if you want a sequence, and a closure to map keys to values, rather than values to keys?

(zip also needs to retain its collection-ness, which is filed as SR-3760, a great starter proposal if anyone wants to take it on :slight_smile:

4. Is there a correctness trap with the composed equivalent? Is there a correctness trap with the helper?

As others noted on the thread, enumerated() used for indices encourages a possible correctness error:

for (idx, element) = someSlice.enumerated() { } // slices aren’t zero based!

Now, chances are that since out-of-bounds subscripting traps on arrays, this bug would be caught early on. But maybe not – especially if you ended up using it on an UnsafeBufferPointer slice (perhaps dropped in for performance reasons to replace an Array) and now all of a sudden you have a memory access violation that could go undetected.

On the flip side: many operations on collections that use indices are hard to get right, especially ones that involve mutating as you go, like removing elements from a collection based on some criteria, where you have to work around index invalidation or fencepost errors. For this reason, the std lib probably ought to have a RangeReplaceableCollection.remove(where:) method so people don’t have to reinvent it and risk getting it wrong.

5. Is there a performance trap with the composed equivalent? Or with the helper?

The composed example of Dictionary from a sequence+closure, you need to remember the .lazy part in sequence.lazy.map to avoid creating a temporary array for no reason. A helper method might lift that burden from the user. remove(where:) can easily be accidentally quadtratic, or perform needless element copies when there’s little or nothing to remove.

Counter example: the fact that map is eager and chaining it creates temporary arrays needlessly is a performance problem caused by introducing it as a helper method.

6. Does the helper actually encourage misuse?

This is a very common pattern when searching GitHub for uses of enumerated():

for (index, _) in collection.enumerated() {
    mutate collect[index] in-place i.e. collection[index] += 1
}.

(or even they assign the element then don’t use it, for which specific case we don’t currently emit a warning)

What the user really needs is just:

for index in collection.indices { etc. }

I expect if the standard way to do enumerated was with zip, people wouldn’t do this as often. In-place map would be even better.

8. Can a native implementation be made more performant than the equivalent composition?

Dictionary.mapValues(transform: (Value)->T) can be implemented internally much more efficiently than just composing it with map and the key/value sequence initializer, because the layout of the hash table storage can be re-used in the new dictionary, even when the Value type is different.

On Jan 31, 2017, at 6:24 AM, Chris Eidhof via swift-evolution <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

Hey everyone,

I've organized a number of Swift workshops over the last two years. There are a couple of things that keep coming up, and a couple of mistakes that I see people making over and over again. One of them is that in almost every workshop, there's someone who thinks that `enumerated()` returns a list of (index, element) pairs. This is only true for arrays. It breaks when using array slices, or any other kind of collection. In our workshops, I sometimes see people doing something like `x.reversed().enumerated()`, where `x` is an array, and somehow it produces behavior they don't understand.

A few ways I think this could be improved:

- Move enumerated to Array
- Change enumerated to return `(Index, Iterator.Element)` (this would mean we at least need to move it to collection)
- Remove enumerated
- Keep things as is

In any case, just wanted to share my experience (gained from teaching people).

--
Chris Eidhof
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org <mailto:swift-evolution@swift.org>
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Charlie Monroe) #7

Hey everyone,

I've organized a number of Swift workshops over the last two years. There are a couple of things that keep coming up, and a couple of mistakes that I see people making over and over again. One of them is that in almost every workshop, there's someone who thinks that `enumerated()` returns a list of (index, element) pairs. This is only true for arrays. It breaks when using array slices, or any other kind of collection. In our workshops, I sometimes see people doing something like `x.reversed().enumerated()`, where `x` is an array, and somehow it produces behavior they don't understand.

What is the behavior that you'd like?

for x in [1, 2, 3].reversed().enumerated() {
    print(x)
}

produces (first column is the index, second the element):

(0, 3)
(1, 2)
(2, 1)

- it IMHO behaves as one would expect - it returns an index into the sequence that you are enumerating...

···

On Jan 31, 2017, at 3:24 PM, Chris Eidhof via swift-evolution <swift-evolution@swift.org> wrote:

A few ways I think this could be improved:

- Move enumerated to Array
- Change enumerated to return `(Index, Iterator.Element)` (this would mean we at least need to move it to collection)
- Remove enumerated
- Keep things as is

In any case, just wanted to share my experience (gained from teaching people).

--
Chris Eidhof
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Jon Hull) #8

This is a common enough need for me that I have built a special class for it. Here is the code in case it is useful to others trying to do the same thing:
https://gist.github.com/jonhull/b9cd8a50abca16ea49eade2c91edf8a2

At it’s heart, it takes an array (or a single value) and turns it into an endless repeating collection. It also has facilities for random output, and to compose with other sources, allowing complex repeating patterns from simple sources. I originally made it for creating fill patterns in a drawing framework (e.g. stripes like your example above), but I have found it generally useful over time.

Thanks,
Jon

···

On Jan 31, 2017, at 12:46 PM, Ben Cohen via swift-evolution <swift-evolution@swift.org> wrote:

// apply alternating view background colors
for (i, view) in views.enumerated() {
    view.backgroundColor = i % 2 ? bgColor1 : bgColor2
}

This is an interesting one because it highlights something else I think is missing from the std lib: Sequence.cycle, which would make an infinite sequence by repeating a sequence, enabling what I think is probably a readability win in some cases:

let colors = [bgColor1, bgColor2].cycle
for (color, view) in zip(colors, views) {
  view.backgroundColor = color
}


(Ole Begemann) #9

Here are three previous discussion about this topic:

1) December 2015: [Idea] Add an (Index, Element) sequence to CollectionType
https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20151221/004561.html and https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20151228/004626.html

2) April 2016: [Idea] Replace enumerate() with something more explicit
https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20160411/015074.html

3) September 2016: [Proposal draft] Introducing `indexed()` collections
https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20160926/027355.html

···

On 31/01/2017 15:24, Chris Eidhof via swift-evolution wrote:

There are a couple of things that keep coming up, and a couple of
mistakes that I see people making over and over again. One of them is
that in almost every workshop, there's someone who thinks that
`enumerated()` returns a list of (index, element) pairs. This is only
true for arrays. It breaks when using array slices, or any other kind of
collection. In our workshops, I sometimes see people doing something
like `x.reversed().enumerated()`, where `x` is an array, and somehow it
produces behavior they don't understand.

A few ways I think this could be improved:

- Move enumerated to Array
- Change enumerated to return `(Index, Iterator.Element)` (this would
mean we at least need to move it to collection)
- Remove enumerated
- Keep things as is


(Xiaodi Wu) #10

So, in general, I agree with your overarching points. But I have to push
back on this part:

Although it's clearly a misuse when one assumes `startIndex == 0` while
working with a generic Collection, it's been said on this list many times
that `0..<array.count` is a perfectly legitimate spelling for the indices
of a concrete `Array<T>`. That is, it's not an unwarranted assumption, but
rather a semantic guarantee that the first index of an array is 0. For the
same reason, using `enumerated()` on an _array_ for indices is not a misuse.

With that in mind, I took the time to do a systematic review of how
`enumerated()` is used. I took the top trending Swift language repositories
over the last 90(?--whatever the longest choice in the dropdown menu was)
days, and searched for uses of this API. Here are the results:

# Hero
Multiple uses: all on arrays; all correct

# iina
Multiple uses: two on arrays, one `string.utf8CString.enumerated()`; all
correct

# Sharaku
No uses

# swift-algorithm-club
Multiple uses: some on arrays and some on `string.characters`; all correct

# Files
No uses
One example in README: correct

# awesome-ios
No uses

# Alamofire
One use: correct

# ODUIThreadGuard
No uses

# Vapor
One use: extension on Collection, *** incorrectly assumes `startIndex == 0`

···

On Tue, Jan 31, 2017 at 1:16 PM, Ben Cohen via swift-evolution < swift-evolution@swift.org> wrote:

I think whether enumerated() is justified as a method on Sequence is one
example of a wider question which definitely needs some discussion, which
is: where should the standard library draw the line in providing
convenience functions that can easily be composed from other functions in
the std lib? Here’s another example:

SE-100
<https://github.com/apple/swift-evolution/blob/master/proposals/0100-add-sequence-based-init-and-merge-to-dictionary.md> is
a proposal to add an init to Dictionary from a sequence of key/value pairs.
It’s a commonly requested feature, and IMO much needed and should be added
as soon as we move to the appropriate phase in Swift’s evolution.

Another commonly requested Dictionary feature is similar: add a
Dictionary.init that takes a sequence, and a closure that maps that
sequence to keys. This is useful, for example, when you have a sequence of
objects that you frequently need to index into via one property on those
objects, so you want to build a fast lookup cache using that property.

Now, if we implement SE-100, that second request can be easily composed.
It would be something like Dictionary(sequence.lazy.map { (key:
$0.someProperty, value: $0) } )

Some people look at that line of code and think sure, that’s what I’d do
and it’s easy enough that the second helper shouldn’t be added as it’s
superfluous. Others look at it and say that it is unreadable clever-code FP
nonsense, and we should just add the helper method because most programmers
wouldn’t be able to read or write that easily.

As we expand (and maybe contract :slight_smile: the standard library, this kind of
question comes up a lot, so it is worth setting out some criteria for
judging these “helper” methods. Here’s my take on such a list (warning:
objectivity and subjectivity blended together in the below).

*1. Is it truly a frequent operation?*

The operation needs to carry its weight. Even once we have ABI stability,
so the size of the std lib becomes less of a concern as it could ship as
part of the OS, we still need to keep helper method growth under control.
APIs bristling with methods like an over-decorated Xmas tree are bad for
usability. As mentioned in the String manifesto, String+Foundation
currently has over 200 methods/properties. Helpers are no good if you can’t
find them to use them.

Someone mentioned that they actually don’t find themselves using
enumerated() all that often. I suspect enumerated in its current form isn’t
all that useful. In a quick (and non-scientific) review of search results
for its use on GitHub, nearly all the examples I see of it are either 1)
misuse – see more below, or 2) use of it to perform the equivalent of
in-place map where the index is used to subscript into the array and
replace it with a transformed element.

***

# Charts
Multiple uses: all on arrays; all correct

# Moya
No uses

# LocalizationKit_iOS
No usages

# SwifterSwift
Multiple uses: almost all on arrays, one
`array.reversed().lazy.enumerated()`; all correct

# ios-oss
Multiple uses: all on arrays; all correct

# RxSwift
Multiple uses: all on arrays; all correct

# SwiftyJSON
No uses

# SwiftLint
Multiple uses: all on arrays; all correct

I think the std lib needs an in-place map, and if enumerated() is removed,
this one most-common use case should be added at the same time.

*2. Is the helper more readable? Is the composed equivalent obvious at
a glance**?*

When an operation is very common, the big win with a helper method is it
creates a readable vocabulary. If enumerated() is very common, and everyone
sees it everywhere, it makes that code easier to read at a glance.

That said, I think that the alternative – zip(0…, sequence) – is just as
readable once you are familiar with zip and ranges, two concepts that, IMO
at least, it is important that every Swift programmer learn about early on.
I would even go so far as to say that enumerated is harmful if it
discourages new users from discovering zip.

As others have already chimed in, `zip(0..., sequence)` is not yet
possible; as I mentioned in another thread, I was once a big fan of `0...`
but now think there are some issues that may need ironing our or may not be
easily solvable with it. In either case, I think my survey above shows that
`enumerated()` is indeed quite common. It is familiar to those coming from
at least one other popular language, Python.

OTOH, an example that I think is strongly justified on readability grounds
is Sequence.all(match:), a function that checks that every element in a
sequence matches a predicate. This is by far the most common extension for
Sequence on GitHub. Yes, you can write this as !seq.contains(!predicate)
but that far less readable – especially since it requires you write it with
a closure that !’s the predicate i.e. !seq.contains { !predicate($0) },
because we don’t have a ! for composing with predicate functions (though
maybe we should).

*3. Does the helper have the flexibility to cover all common cases?*

This, I think, is where enumerated() really starts to fall down.

Sometimes you want to start from not-zero, so maybe it should be
Sequence.enumerated(startingFrom: Int = 0)

Python, which has `enumerate`, has indeed extended that function to allow
this. We could trivially do the same.

Sometimes you want non-integer indices so maybe we should add indexed().

That has been floated and, like Jacob, I think it's a fine addition.

Sometimes you want it the other way around (not for a for loop but for
passing the elements directly into a mapping function) so now you need a
flip.

Flip the order of the number and the value in the tuple? Wouldn't you just
use `map` for that?

Sometimes you want to enumeration to run backwards in some way.

Less of a problem if you’re already accustomed to composing your
enumeration:

Enumerate starting from 1: zip(1…, c)
Enumerate indices: zip(c.indices, c)
Need the pair to be the other way around: zip(c, 0…)
Need the enumeration to run the other way: zip((0..<c.count).reversed,c)
or zip(0…,c).reversed() or zip(0…,c.reversed()).

As shown above, people do indeed compose with `enumerated()`.

Similarly, for the Dictionary helper – what if you want a sequence, and a
closure to map keys to values, rather than values to keys?

(zip also needs to retain its collection-ness, which is filed as SR-3760,
a great starter proposal if anyone wants to take it on :slight_smile:

*4. Is there a correctness trap with the composed equivalent? Is there a
correctness trap with the helper?*

As others noted on the thread, enumerated() used for indices encourages a
possible correctness error:

for (idx, element) = someSlice.enumerated() { } // slices aren’t zero
based!

Now, chances are that since out-of-bounds subscripting traps on arrays,
this bug would be caught early on. But maybe not – especially if you ended
up using it on an UnsafeBufferPointer slice (perhaps dropped in for
performance reasons to replace an Array) and now all of a sudden you have a
memory access violation that could go undetected.

On the flip side: many operations on collections that use indices are hard
to get right, especially ones that involve mutating as you go, like
removing elements from a collection based on some criteria, where you have
to work around index invalidation or fencepost errors. For this reason, the
std lib probably ought to have a RangeReplaceableCollection.remove(where:)
method so people don’t have to reinvent it and risk getting it wrong.

*5. Is there a performance trap with the composed equivalent? Or with the
helper?*

The composed example of Dictionary from a sequence+closure, you need to
remember the .lazy part in sequence.lazy.map to avoid creating a temporary
array for no reason. A helper method might lift that burden from the user.
remove(where:) can easily be accidentally quadtratic, or perform needless
element copies when there’s little or nothing to remove.

Counter example: the fact that map is eager and chaining it creates
temporary arrays needlessly is a performance problem caused by introducing
it as a helper method.

*6. Does the helper actually encourage misuse?*

This is a very common pattern when searching GitHub for uses of
enumerated():

for (index, _) in collection.enumerated() {
    mutate collect[index] in-place i.e. collection[index] += 1
}.

(or even they assign the element then don’t use it, for which specific
case we don’t currently emit a warning)

What the user really needs is just:

for index in collection.indices { etc. }

I expect if the standard way to do enumerated was with zip, people
wouldn’t do this as often. In-place map would be even better.

As the data show above, people use `enumerated()` correctly far more often
than they use it incorrectly. In the one place where it's erroneously used,
it was an extension on Collection; it's likely that the only instances that
actually used that extension method were collections with zero-based
indices.

*8. Can a native implementation be made more performant than the
equivalent composition?*

Dictionary.mapValues(transform: (Value)->T) can be implemented internally
much more efficiently than just composing it with map and the key/value
sequence initializer, because the layout of the hash table storage can be
re-used in the new dictionary, even when the Value type is different.

On Jan 31, 2017, at 6:24 AM, Chris Eidhof via swift-evolution < > swift-evolution@swift.org> wrote:

Hey everyone,

I've organized a number of Swift workshops over the last two years. There
are a couple of things that keep coming up, and a couple of mistakes that I
see people making over and over again. One of them is that in almost every
workshop, there's someone who thinks that `enumerated()` returns a list of
(index, element) pairs. This is only true for arrays. It breaks when using
array slices, or any other kind of collection. In our workshops, I
sometimes see people doing something like `x.reversed().enumerated()`,
where `x` is an array, and somehow it produces behavior they don't
understand.

A few ways I think this could be improved:

- Move enumerated to Array
- Change enumerated to return `(Index, Iterator.Element)` (this would mean
we at least need to move it to collection)
- Remove enumerated
- Keep things as is

In any case, just wanted to share my experience (gained from teaching
people).

--
Chris Eidhof
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Chris Lattner) #11

This is great perspective Ben, thank you for taking time to write this up!

It seems that you are leaning towards removing enumerated(). What do you think about the proposal of removing enumerated but adding indexed() to replace it? It would provide the same behavior for common array cases, while providing more useful/defensible functionality for other collections.

-Chris

···

On Jan 31, 2017, at 11:16 AM, Ben Cohen via swift-evolution <swift-evolution@swift.org> wrote:

As we expand (and maybe contract :slight_smile: the standard library, this kind of question comes up a lot, so it is worth setting out some criteria for judging these “helper” methods. Here’s my take on such a list (warning: objectivity and subjectivity blended together in the below).


(Haravikk) #12

I'm in favour of getting rid of enumerated; I think like many people I used it expecting to get actual indices, and it's very easy to think this when working with arrays (as the values will in fact be perfectly valid). In reality the right way to do it is with one of the following:

for eachIndex in myArray.indices { print("\(eachIndex): " + myArray[eachIndex]) }
for eachIndex in myArray.startIndex ..< myArray.endIndex { print("\(eachIndex): " + myArray[eachIndex]) }
var offset = 0; for eachEntry in myArray { print("\(offset): \(eachEntry)"); offset += 1 }

However, I think I'd still support the inclusion of an enumerating and an indexing type that sequences/collections can be wrapped in to perform enumeration/indexing if you still want it.

···

On 31 Jan 2017, at 14:24, Chris Eidhof via swift-evolution <swift-evolution@swift.org> wrote:

Hey everyone,

I've organized a number of Swift workshops over the last two years. There are a couple of things that keep coming up, and a couple of mistakes that I see people making over and over again. One of them is that in almost every workshop, there's someone who thinks that `enumerated()` returns a list of (index, element) pairs. This is only true for arrays. It breaks when using array slices, or any other kind of collection. In our workshops, I sometimes see people doing something like `x.reversed().enumerated()`, where `x` is an array, and somehow it produces behavior they don't understand.

A few ways I think this could be improved:

- Move enumerated to Array
- Change enumerated to return `(Index, Iterator.Element)` (this would mean we at least need to move it to collection)
- Remove enumerated
- Keep things as is

In any case, just wanted to share my experience (gained from teaching people).

--
Chris Eidhof
_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Ole Begemann) #13

To clarify, the discussions I linked to don't all propose to remove or replace `enumerated()`, but they all talk about the potential confusion about what `enumerated()` does and does not do.

···

On 31/01/2017 16:19, Ole Begemann via swift-evolution wrote:

Here are three previous discussion about this topic:

1) December 2015: [Idea] Add an (Index, Element) sequence to
CollectionType
https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20151221/004561.html
and
https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20151228/004626.html

2) April 2016: [Idea] Replace enumerate() with something more explicit
https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20160411/015074.html

3) September 2016: [Proposal draft] Introducing `indexed()` collections
https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20160926/027355.html


(Erica Sadun) #14

https://gist.github.com/erica/2b2d92e6db787d001c689d3e37a7c3f2

This could easily be adapted to incorporate both the removal of enumerated as well as the addition of indexed, and I love the idea of using Ben's list as a template for the justification section, plus adding the feedback from this thread.

-- E

···

On Feb 2, 2017, at 9:46 PM, Chris Lattner via swift-evolution <swift-evolution@swift.org> wrote:

On Jan 31, 2017, at 11:16 AM, Ben Cohen via swift-evolution <swift-evolution@swift.org> wrote:

As we expand (and maybe contract :slight_smile: the standard library, this kind of question comes up a lot, so it is worth setting out some criteria for judging these “helper” methods. Here’s my take on such a list (warning: objectivity and subjectivity blended together in the below).

This is great perspective Ben, thank you for taking time to write this up!

It seems that you are leaning towards removing enumerated(). What do you think about the proposal of removing enumerated but adding indexed() to replace it? It would provide the same behavior for common array cases, while providing more useful/defensible functionality for other collections.

-Chris


(Dave Abrahams) #15

indexed() is trivially composed from other library primitives:

      zip(c.indices, c)

enumerated() only exists in the standard library, and was explicitly
designed as it was, because what it does is not currently easy to
write. As soon as we enable "0...", that changes.

I'm not against having trivial helper functions in principle, but I
don't think the Standard Library or the language is yet at the stage of
maturity where we can see clearly which ones are important enough, and
pay for the API surface area they add. For that reason I feel strongly
that for the time being we should remain very conservative about adding
such things, and only introduce them where there's real compelling
evidence that they are a win. There is no such evidence for indexed()
as far as I can tell.

···

on Thu Feb 02 2017, Chris Lattner <swift-evolution@swift.org> wrote:

On Jan 31, 2017, at 11:16 AM, Ben Cohen via swift-evolution <swift-evolution@swift.org> wrote:

As we expand (and maybe contract :slight_smile: the standard library, this kind
of question comes up a lot, so it is worth setting out some criteria
for judging these “helper” methods. Here’s my take on such a list
(warning: objectivity and subjectivity blended together in the
below).

This is great perspective Ben, thank you for taking time to write this up!

It seems that you are leaning towards removing enumerated(). What do
you think about the proposal of removing enumerated but adding
indexed() to replace it? It would provide the same behavior for
common array cases, while providing more useful/defensible
functionality for other collections.

--
-Dave


(Ben Cohen) #16

I’m actually kind of conflicted.

Replacing enumerated() with indexed() feels replacing one problem for another. Sometimes people want to number things, and might assume indexed() will be zero-based for slices.

Adding indexed() while keeping enumerated() seems too much clutter on the API though. Once we have 0… both can be expressed simply with zip, and in my view is zip(a, 0…), zip(a, a.indices), zip(1…, a) just as clear, maybe clearer in some cases as they will encourage code to show intent more (i.e. are you counting or indexing? even when they are the same, it’s better to say which). Encouraging learning about zip will also help introduce people to better ways of expressing other similar-but-different loops.

The trouble with zip is it isn’t discoverable – another entry that probably belongs on that list of criteria. Unlike enumerated, users aren’t going to stumble over it.

Maybe moving zip to be a method on Sequence rather than a free function would help with this? e.g. something like a.zipped(with: 0…), a.zipped(with: a.indices). The documentation for it could even explicitly mention the counting and index use cases. The main downside is it pushes the order of the lhs/rhs to be self first.

···

On Feb 2, 2017, at 8:46 PM, Chris Lattner <clattner@nondot.org> wrote:

It seems that you are leaning towards removing enumerated().


(Vladimir) #17

Btw, in context of discussion of indices,
should this be fixed soon:

func function<C: Collection>(c: C) {
   for index in c.indices {
     print(c[index])
   }
}
ERROR: cannot subscript a value of type 'C' with an index of type 'C.Indices.Iterator.Element'

?
(have access for Swift 3.0.2 Release only for now, so probably this already fixed in dev version)

···

On 01.02.2017 1:54, Xiaodi Wu via swift-evolution wrote:

On Tue, Jan 31, 2017 at 1:16 PM, Ben Cohen via swift-evolution > <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

    I think whether enumerated() is justified as a method on Sequence is
    one example of a wider question which definitely needs some discussion,
    which is: where should the standard library draw the line in providing
    convenience functions that can easily be composed from other functions
    in the std lib? Here’s another example:

    SE-100
    <https://github.com/apple/swift-evolution/blob/master/proposals/0100-add-sequence-based-init-and-merge-to-dictionary.md> is
    a proposal to add an init to Dictionary from a sequence of key/value
    pairs. It’s a commonly requested feature, and IMO much needed and
    should be added as soon as we move to the appropriate phase in Swift’s
    evolution.

    Another commonly requested Dictionary feature is similar: add a
    Dictionary.init that takes a sequence, and a closure that maps that
    sequence to keys. This is useful, for example, when you have a sequence
    of objects that you frequently need to index into via one property on
    those objects, so you want to build a fast lookup cache using that
    property.

    Now, if we implement SE-100, that second request can be easily
    composed. It would be something like Dictionary(sequence.lazy.map {
    (key: $0.someProperty, value: $0) } )

    Some people look at that line of code and think sure, that’s what I’d
    do and it’s easy enough that the second helper shouldn’t be added as
    it’s superfluous. Others look at it and say that it is unreadable
    clever-code FP nonsense, and we should just add the helper method
    because most programmers wouldn’t be able to read or write that easily.

    As we expand (and maybe contract :slight_smile: the standard library, this kind of
    question comes up a lot, so it is worth setting out some criteria for
    judging these “helper” methods. Here’s my take on such a list (warning:
    objectivity and subjectivity blended together in the below).

    *1. Is it truly a frequent operation?*

    The operation needs to carry its weight. Even once we have ABI
    stability, so the size of the std lib becomes less of a concern as it
    could ship as part of the OS, we still need to keep helper method
    growth under control. APIs bristling with methods like an
    over-decorated Xmas tree are bad for usability. As mentioned in the
    String manifesto, String+Foundation currently has over 200
    methods/properties. Helpers are no good if you can’t find them to use them.

    Someone mentioned that they actually don’t find themselves using
    enumerated() all that often. I suspect enumerated in its current form
    isn’t all that useful. In a quick (and non-scientific) review of search
    results for its use on GitHub, nearly all the examples I see of it are
    either 1) misuse – see more below, or 2) use of it to perform the
    equivalent of in-place map where the index is used to subscript into
    the array and replace it with a transformed element.

So, in general, I agree with your overarching points. But I have to push
back on this part:

Although it's clearly a misuse when one assumes `startIndex == 0` while
working with a generic Collection, it's been said on this list many times
that `0..<array.count` is a perfectly legitimate spelling for the indices
of a concrete `Array<T>`. That is, it's not an unwarranted assumption, but
rather a semantic guarantee that the first index of an array is 0. For the
same reason, using `enumerated()` on an _array_ for indices is not a misuse.

With that in mind, I took the time to do a systematic review of how
`enumerated()` is used. I took the top trending Swift language repositories
over the last 90(?--whatever the longest choice in the dropdown menu was)
days, and searched for uses of this API. Here are the results:

# Hero
Multiple uses: all on arrays; all correct

# iina
Multiple uses: two on arrays, one `string.utf8CString.enumerated()`; all
correct

# Sharaku
No uses

# swift-algorithm-club
Multiple uses: some on arrays and some on `string.characters`; all correct

# Files
No uses
One example in README: correct

# awesome-ios
No uses

# Alamofire
One use: correct

# ODUIThreadGuard
No uses

# Vapor
One use: extension on Collection, *** incorrectly assumes `startIndex == 0` ***

# Charts
Multiple uses: all on arrays; all correct

# Moya
No uses

# LocalizationKit_iOS
No usages

# SwifterSwift
Multiple uses: almost all on arrays, one
`array.reversed().lazy.enumerated()`; all correct

# ios-oss
Multiple uses: all on arrays; all correct

# RxSwift
Multiple uses: all on arrays; all correct

# SwiftyJSON
No uses

# SwiftLint
Multiple uses: all on arrays; all correct

    I think the std lib needs an in-place map, and if enumerated() is
    removed, this one most-common use case should be added at the same time.

    *2. Is the helper more readable? Is the composed equivalent obvious at
    a glance**?*

    When an operation is very common, the big win with a helper method is
    it creates a readable vocabulary. If enumerated() is very common, and
    everyone sees it everywhere, it makes that code easier to read at a glance.

    That said, I think that the alternative – zip(0…, sequence) – is just
    as readable once you are familiar with zip and ranges, two concepts
    that, IMO at least, it is important that every Swift programmer learn
    about early on. I would even go so far as to say that enumerated is
    harmful if it discourages new users from discovering zip.

As others have already chimed in, `zip(0..., sequence)` is not yet
possible; as I mentioned in another thread, I was once a big fan of `0...`
but now think there are some issues that may need ironing our or may not be
easily solvable with it. In either case, I think my survey above shows that
`enumerated()` is indeed quite common. It is familiar to those coming from
at least one other popular language, Python.

    OTOH, an example that I think is strongly justified on readability
    grounds is Sequence.all(match:), a function that checks that every
    element in a sequence matches a predicate. This is by far the most
    common extension for Sequence on GitHub. Yes, you can write this as
    !seq.contains(!predicate) but that far less readable – especially since
    it requires you write it with a closure that !’s the predicate i.e.
    !seq.contains { !predicate($0) }, because we don’t have a ! for
    composing with predicate functions (though maybe we should).

    *3. Does the helper have the flexibility to cover all common cases?*

    This, I think, is where enumerated() really starts to fall down.

    Sometimes you want to start from not-zero, so maybe it should be
    Sequence.enumerated(startingFrom: Int = 0)

Python, which has `enumerate`, has indeed extended that function to allow
this. We could trivially do the same.

    Sometimes you want non-integer indices so maybe we should add indexed().

That has been floated and, like Jacob, I think it's a fine addition.

    Sometimes you want it the other way around (not for a for loop but for
    passing the elements directly into a mapping function) so now you need
    a flip.

Flip the order of the number and the value in the tuple? Wouldn't you just
use `map` for that?

    Sometimes you want to enumeration to run backwards in some way.

    Less of a problem if you’re already accustomed to composing your
    enumeration:

    Enumerate starting from 1: zip(1…, c)
    Enumerate indices: zip(c.indices, c)
    Need the pair to be the other way around: zip(c, 0…)
    Need the enumeration to run the other way:
    zip((0..<c.count).reversed,c) or zip(0…,c).reversed() or
    zip(0…,c.reversed()).

As shown above, people do indeed compose with `enumerated()`.

    Similarly, for the Dictionary helper – what if you want a sequence, and
    a closure to map keys to values, rather than values to keys?

    (zip also needs to retain its collection-ness, which is filed as
    SR-3760, a great starter proposal if anyone wants to take it on :slight_smile:

    *4. Is there a correctness trap with the composed equivalent? Is there
    a correctness trap with the helper?*

    As others noted on the thread, enumerated() used for indices encourages
    a possible correctness error:

    for (idx, element) = someSlice.enumerated() { } // slices aren’t zero
    based!

    Now, chances are that since out-of-bounds subscripting traps on arrays,
    this bug would be caught early on. But maybe not – especially if you
    ended up using it on an UnsafeBufferPointer slice (perhaps dropped in
    for performance reasons to replace an Array) and now all of a sudden
    you have a memory access violation that could go undetected.

    On the flip side: many operations on collections that use indices are
    hard to get right, especially ones that involve mutating as you go,
    like removing elements from a collection based on some criteria, where
    you have to work around index invalidation or fencepost errors. For
    this reason, the std lib probably ought to have a
    RangeReplaceableCollection.remove(where:) method so people don’t have
    to reinvent it and risk getting it wrong.

    *5. Is there a performance trap with the composed equivalent? Or with
    the helper?*

    The composed example of Dictionary from a sequence+closure, you need to
    remember the .lazy part in sequence.lazy.map to avoid creating a
    temporary array for no reason. A helper method might lift that burden
    from the user. remove(where:) can easily be accidentally quadtratic, or
    perform needless element copies when there’s little or nothing to remove.

    Counter example: the fact that map is eager and chaining it creates
    temporary arrays needlessly is a performance problem caused by
    introducing it as a helper method.

    *6. Does the helper actually encourage misuse?*

    This is a very common pattern when searching GitHub for uses of
    enumerated():

    for (index, _) in collection.enumerated() {
        mutate collect[index] in-place i.e. collection[index] += 1
    }.

    (or even they assign the element then don’t use it, for which specific
    case we don’t currently emit a warning)

    What the user really needs is just:

    for index in collection.indices { etc. }

    I expect if the standard way to do enumerated was with zip, people
    wouldn’t do this as often. In-place map would be even better.

As the data show above, people use `enumerated()` correctly far more often
than they use it incorrectly. In the one place where it's erroneously used,
it was an extension on Collection; it's likely that the only instances that
actually used that extension method were collections with zero-based indices.

    *8. Can a native implementation be made more performant than the
    equivalent composition?*

    Dictionary.mapValues(transform: (Value)->T) can be implemented
    internally much more efficiently than just composing it with map and
    the key/value sequence initializer, because the layout of the hash
    table storage can be re-used in the new dictionary, even when the Value
    type is different.

    On Jan 31, 2017, at 6:24 AM, Chris Eidhof via swift-evolution >> <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

    Hey everyone,

    I've organized a number of Swift workshops over the last two years.
    There are a couple of things that keep coming up, and a couple of
    mistakes that I see people making over and over again. One of them is
    that in almost every workshop, there's someone who thinks that
    `enumerated()` returns a list of (index, element) pairs. This is only
    true for arrays. It breaks when using array slices, or any other kind
    of collection. In our workshops, I sometimes see people doing
    something like `x.reversed().enumerated()`, where `x` is an array,
    and somehow it produces behavior they don't understand.

    A few ways I think this could be improved:

    - Move enumerated to Array
    - Change enumerated to return `(Index, Iterator.Element)` (this would
    mean we at least need to move it to collection)
    - Remove enumerated
    - Keep things as is

    In any case, just wanted to share my experience (gained from teaching
    people).

    --
    Chris Eidhof
    _______________________________________________
    swift-evolution mailing list
    swift-evolution@swift.org <mailto:swift-evolution@swift.org>
    https://lists.swift.org/mailman/listinfo/swift-evolution
    <https://lists.swift.org/mailman/listinfo/swift-evolution>

    _______________________________________________
    swift-evolution mailing list
    swift-evolution@swift.org <mailto:swift-evolution@swift.org>
    https://lists.swift.org/mailman/listinfo/swift-evolution
    <https://lists.swift.org/mailman/listinfo/swift-evolution>

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Xiaodi Wu) #18

I totally sympathize with users being confused. It's an interesting idea to
move it to Array only.

The thing is, it does make sense (and wouldn't be confusing) to enumerate a
dictionary or set. Moreover, the behavior is _exactly_ what it says on the
tin: when you enumerate something in real life, there is no sense in which
the number is related to some sort of index. Can we fix this by
documentation? Like, a big blaring "don't use this when you want the index"?

···

On Tue, Jan 31, 2017 at 09:35 Ole Begemann via swift-evolution < swift-evolution@swift.org> wrote:

On 31/01/2017 16:19, Ole Begemann via swift-evolution wrote:
> Here are three previous discussion about this topic:
>
> 1) December 2015: [Idea] Add an (Index, Element) sequence to
> CollectionType
>
https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20151221/004561.html
> and
>
https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20151228/004626.html
>
>
> 2) April 2016: [Idea] Replace enumerate() with something more explicit
>
https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20160411/015074.html
>
>
> 3) September 2016: [Proposal draft] Introducing `indexed()`
collections
>
https://lists.swift.org/pipermail/swift-evolution/Week-of-Mon-20160926/027355.html

To clarify, the discussions I linked to don't all propose to remove or
replace `enumerated()`, but they all talk about the potential confusion
about what `enumerated()` does and does not do.

_______________________________________________
swift-evolution mailing list
swift-evolution@swift.org
https://lists.swift.org/mailman/listinfo/swift-evolution


(Erica Sadun) #19

I believe what people *want* is `indexed` over `enumerated`, and consistently for both array and array slices.

-- E

···

On Feb 3, 2017, at 10:57 AM, Dave Abrahams via swift-evolution <swift-evolution@swift.org> wrote:

on Thu Feb 02 2017, Chris Lattner <swift-evolution@swift.org <mailto:swift-evolution@swift.org>> wrote:

On Jan 31, 2017, at 11:16 AM, Ben Cohen via swift-evolution <swift-evolution@swift.org> wrote:

As we expand (and maybe contract :slight_smile: the standard library, this kind
of question comes up a lot, so it is worth setting out some criteria
for judging these “helper” methods. Here’s my take on such a list
(warning: objectivity and subjectivity blended together in the
below).

This is great perspective Ben, thank you for taking time to write this up!

It seems that you are leaning towards removing enumerated(). What do
you think about the proposal of removing enumerated but adding
indexed() to replace it? It would provide the same behavior for
common array cases, while providing more useful/defensible
functionality for other collections.

indexed() is trivially composed from other library primitives:

     zip(c.indices, c)

enumerated() only exists in the standard library, and was explicitly
designed as it was, because what it does is not currently easy to
write. As soon as we enable "0...", that changes.

I'm not against having trivial helper functions in principle, but I
don't think the Standard Library or the language is yet at the stage of
maturity where we can see clearly which ones are important enough, and
pay for the API surface area they add. For that reason I feel strongly
that for the time being we should remain very conservative about adding
such things, and only introduce them where there's real compelling
evidence that they are a win. There is no such evidence for indexed()
as far as I can tell.


(Dave Abrahams) #20

I don't always make zip a method, but when I do, its argument label is
“to:”

···

on Fri Feb 03 2017, Ben Cohen <swift-evolution@swift.org> wrote:

On Feb 2, 2017, at 8:46 PM, Chris Lattner <clattner@nondot.org> > wrote:

It seems that you are leaning towards removing enumerated().

I’m actually kind of conflicted.

Replacing enumerated() with indexed() feels replacing one problem for
another. Sometimes people want to number things, and might assume
indexed() will be zero-based for slices.

Adding indexed() while keeping enumerated() seems too much clutter on
the API though. Once we have 0… both can be expressed simply with zip,
and in my view is zip(a, 0…), zip(a, a.indices), zip(1…, a) just as
clear, maybe clearer in some cases as they will encourage code to show
intent more (i.e. are you counting or indexing? even when they are the
same, it’s better to say which). Encouraging learning about zip will
also help introduce people to better ways of expressing other
similar-but-different loops.

The trouble with zip is it isn’t discoverable – another entry that
probably belongs on that list of criteria. Unlike enumerated, users
aren’t going to stumble over it.

Maybe moving zip to be a method on Sequence rather than a free
function would help with this? e.g. something like a.zipped(with: 0…),
a.zipped(with: a.indices). The documentation for it could even
explicitly mention the counting and index use cases. The main downside
is it pushes the order of the lhs/rhs to be self first.

--
-Dave