[swift-evolution] Stdlib closure argument labels and parameter names

[Dave Abrahams]

on Mon Jun 20 2016, Brent Royal-Gordon <swift-evolution at swift.org> wrote:

>> A couple of weeks ago we started to notice that we had some poorly-named
>> closure parameters and argument labels in the standard library, so we
>> did a complete audit of the standard library's APIs and came up with a
>> preliminary proposal for changes, which we applied in a branch and you
>> can review in https://github.com/apple/swift/pull/2981.  Let's please
>> carry on further discussion here rather than in the pull request, though.
>
> In general, I like this; `orderingBy` is a particularly nice
> improvement over the old `isOrderedBefore` convention. 

I don't really love the use of “by”, FWIW, but I thought `orderingWith`
was more confusable (ordering A with B might swap A and B, whereas the
parameter is a closure).  It could be argued, though, that I am being
overly concerned with unlikely misinterpretations, at the cost of
“naturalness”—a known weakness of mine ;-).  Anyway, as ever I'm open to
discussion on this.

> A few specific comments about things I don't like:
>
> * In `map` and `flatMap`, I'm not sure how much `transform` buys us
>   over `elementTransform`.

I think you mean the converse.  And I agree that `elementTransform`
is probably not an improvement over `transform`.

> * In general, I'm not a fan of most of the changes away from `where`
> labels. 

The only such changes I can find are in
https://github.com/apple/swift/pull/2981/commits/3418eede88d724ad23731fe8f412f51e03cf5106

Note that part of this change was to make all filter closures
consistent; in the main `filter` API there was no label at all.
However, we felt that there's a real clarity problem with the polarity
of the argument (we talk about “filtering things out” but the closure
indicates which elements to keep).  And we couldn't find a “where”-based
name that began to clarify it.

I will argue that even changing to “suchThat,” as in the PR, does not
sufficiently clarify the closure's polarity, and the only true fix for
filter is to use a different base name (some have suggested “select,”
and I have other ideas), but that is out of scope for this particular
set of changes.  So if the community is happier with a “where” label
here I can live with it.  I do think “suchThat” is marginally clearer.

> Those are a nice, straightforward convention applied broadly across
> the Sequence APIs. (Yes, I criticized `where` as a method name in
> another thread, but I don't think `where` is a problem when there's a
> function base name to give it context.) When they don't work, that's
> usually because of a less-than-ideal base name. I'm not saying that
> *all* base names that aren't compatible with `where` should be
> changed, but rather that if `where` is not enough, that's an API
> smell.
>
> * In particular, `elementWhere` is not a good label for the same
> reason that `removeElement` is not a good name. Session 403 last week
> actually talked about this between roughly minutes 8 and 11. (I'm sure
> you know about its content; you probably saw it before we did.)

Yes I do, and I think you misinterpreted the message in that session.
There's nothing wrong with repeating type information when it's
necessary for clarity or fluency at the use-site.  In the case of
`contains(elementWhere:)`, it's there for fluency:

       customers.contains(where: isSingle)

doesn't read as well as:

       customers.contains(elementWhere: isSingle)

The point is not to imagine that every argument should be preceded by a
noun, and repetition of type information is often the result of trying
to do that.

> * I like `separatedWhere` on `split`, but I think the Equatable
> version needs a similar renaming. 

That's a nice thought; I think it's arguably out-of-scope here, though.

> Perhaps `separatedBy`?  `separatedOn`? The usual opposite of `where`,
> `of`, doesn't work here. (Alternatively, `separatedWhere` could be
> `separatorWhere` instead, but that's not quite as elegant.)

I'd want to consider variations of `separatingAt` or `onSeparator` or
`atSeparator` too... which makes me thing “separatedWhere” might not be
as good as “separatingWhere” for the closure version.

> * I'm very uncomfortable with the amount of weight
> `accumulatingResultBy` adds to `reduce`. `combinedBy` seems perfectly
> cromulent to me. I'm even more concerned by your suggestion in the
> pull request body of
> `accumulating(startingFrom:combiningBy:)`. `reduce` is a subtle and
> slightly confusing operation; adding more words to its call sites will
> not solve that problem. If you want to invent a new name from whole
> cloth, I would probably use something like `combining(with
> initialResult: T, by nextResult: (T, Element) -> T)`. (For that
> matter, while we're working in this area, `sequence(first:next:)`
> could use a similar coat of paint.)

As with `filter(suchThat:`, `reduce(accumulatingResultBy:` is attempting
to solve with an argument label what IMO is a grave weakness in clarity
of the base name.  If you read the documentation for `reduce`, you'll
see that it's all about accumulating a result, and if you consider that
its current signature often leads to O(N^2) behavior and we are thinking
about adding an overload that takes its “accumulator” inout, the
arguments for avoiding the name “accumulate” get progressively weaker.
But as noted earlier, changing base names is out-of-scope for this
proposal.  As with “filter,” I could live with leaving this alone,
though I do believe “accumulatingResultBy:” is a real improvement in
clarity.

> * I agree with the comment on GitHub that `invoke` should be
> `execute`. 

Why?  Rationales help.

> If you see a distinction between the two cases on the number of
> arguments, I would then suggest `passTo` as the label on these
> methods: `views.forEach(passTo: addSubview)`,
> `withUnsafeBufferPointer(&bytes, passTo: Data.init(buffer:))`.

Those are intriguing ideas, but that direction tends to suggest this
would be better:

  views.passEach(to: addSubview)
  passUnsafeBufferPointer(to: Data.init(buffer:))

...until you pass a trailing closure:

  views.passEach { addSubView($0) }
  passUnsafeBufferPointer { Data.init(buffer:$0) }

(note: withUnsafeBufferPointer takes only one argument, a closure).

>
> * It's a little odd that you're using `comparingBy` for `Equatable`
> and `orderingBy` for `Comparable`. Did you judge `equatingBy` to be
> too awkward? 

Yes, and because it's not “equating,” which would mean using equality
(==) it's “testing equivalence” with respect to the predicate.

> Perhaps the real problem is that `Equatable` ought to be `Comparable`
> and `Comparable` ought to be `Orderable`? 

I don't think so, personally, but regardless I consider such a change
out-of-scope for this proposal.

> Or maybe `comparingBy` should just be something more general, like
> `matchingBy`? That would make perfectly sensible but slightly odd use
> cases like this one read better:
>
> 	let isAnIdiot = luggageCombination.starts(with: [1, 2, 3, 4,
> 5], matchingBy: <=) // Matches [1,2,3,4,5], but also [1,1,1,1,1],
> [1,2,3,2,1], etc.

That would not be legal, as <= is not an equivalence relation.  You
could think about redefining the meaning of `starts(with:` to not
require an equivalence relation, but that's something I'm not confident
*I* know how to do meaningfully, and regardless is again out-of-scope.

> Very soon (hopefully), I will be posting an early draft of a proposal
> renaming the various first/last/prefix/suffix/etc. APIs. I believe the
> only place it touches on your proposal is in
> `starts(with:isEquivalent:)`, but I think your changes to the second
> parameter label can be easily incorporated into what I'm doing.

Great!

-- 
Dave