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 :) 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 :)
*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
