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

[Dave Abrahams]

on Wed Jun 22 2016, Erica Sadun <erica-AT-ericasadun.com> wrote:

> On Jun 20, 2016, at 3:25 PM, Dave Abrahams via swift-evolution <swift-evolution at swift.org> wrote:
>> 
>> 
>> Hi All,
>> 
>> 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.
>
> -  /// - `isEquivalent(a, a)` is always `true`. (Reflexivity)
>  -  /// - `isEquivalent(a, b)` implies `isEquivalent(b, a)`. (Symmetry)
>  -  /// - If `isEquivalent(a, b)` and `isEquivalent(b, c)` are both `true`, then
>  -  ///   `isEquivalent(a, c)` is also `true`. (Transitivity)
>  +  /// - `areEquivalent(a, a)` is always `true`. (Reflexivity)
>  +  /// - `areEquivalent(a, b)` implies `areEquivalent(b, a)`. (Symmetry)
>  +  /// - If `areEquivalent(a, b)` and `areEquivalent(b, c)` are both `true`, then
>  +  ///   `areEquivalent(a, c)` is also `true`. (Transitivity)
>
> I like this change!
>
> -    func forEach<S: SequenceType>(_ body: (S.Iterator.Element) -> ())
> +    func forEach<S: SequenceType>(invoke body: (S.Iterator.Element) -> ())
>
> Adding an external label makes sense here. This is a procedural call and
> using it within the parens should have a "code ripple".

I don't think I understand what you mean here.

> That said, would prefer `do` or `perform` over `invoke` or `invoking` as in
> `runRaceTest`, `_forAllPermutationsImpl`, `expectFailure`, etc. 

Sorry, again, I'm a little lost.  Forgive my overly-literal brain but
could you please spell out how those latter 3 names relate to the choice
of `do` or `perform` over `invoke`?   

> This also applies where there's a `body` label instead of an empty
> external label.

We don't have any methods with a `body` label IIUC.

> -public func withInvalidOrderings(_ body: ((Int, Int) -> Bool) -> Void) {
> +public func withInvalidOrderings(invoke body: ((Int, Int) -> Bool) -> Void) {
>
> For any with/external label pair, I'd prefer `with-do` or `with-perform` 
> over `with-invoke`.

OK.  Is there a rationale, or is it just personal taste?

> -  return IteratorSequence(it).reduce(initial, combine: f)
> +  return IteratorSequence(it).reduce(initial, accumulatingBy: f)
>
> For `reduce`, I'd prefer `applying:` or `byApplying:`

Makes sense.

> Similarly in `starts(with:comparingBy:)`, I'd prefer byComparing`,

I don't see how that works.  You're not comparing the closure with
anything.

> min/max, byOrdering

Likewise, you're not ordering the closure.

> -      ).encode(encoding, output: output)
> +      ).encode(encoding, sendingOutputTo: processCodeUnit)
>
> How about `exportingTo`?

“export” is freighted with various connotations that I don't think we
want to make here.  In fact it doesn't mean anything more exotic than
“sending output to.”

> -  tempwords.sort(isOrderedBefore: <)
> +  tempwords.sort(orderingBy: <)
>
> With `sort` and `sorted`, I'd prefer `by:`

When people talk about “sorting by XXX”, XXX is a property of the
elements being sorted.  Therefore IMIO that label should probably be
reserved for a version that takes a unary projection function:

     friends.sort(by: {$0.lastName})

> -  if !expected.elementsEqual(actual, isEquivalent: sameValue) {
> +  if !expected.elementsEqual(actual, comparingBy: sameValue) {
>
> I'm torn on this one. I don't like but I don't have a good solution.
>
> -  /// for which `predicate(x) == true`.
> +  /// for which `isIncluded(x) == true`.
> -      _base: base.makeIterator(), whereElementsSatisfy: _include)
> +      _base: base.makeIterator(), suchThat: _include)
>
> How about simply `include` for both? 

Looking at the effect on the doc comments—which is how we should judge
parameter names—I think `predicate` might read better.

> I get the `is` desire but it's being tossed away in a lot of other
> places in this diff. and `suchThat` feels out of place.

I think I'm pretty strongly sold on “soEach” as a label for closures in
all the filtering components.

> -      || u16.contains({ $0 > 127 || _isspace_clocale($0) }) {
> +    || u16.contains(elementWhere: { $0 > 127 || _isspace_clocale($0) }) {
>
> I assume the challenge here is differentiating contains(element) from contains(closure).
> This feels predicate-y, which is why I put it near the predicates. But I think something
> like `containsElement(where:)` works better.

I understand your motivation for suggesting it.  The downside is that it
severs the strong basename relationship between algorithms that do
effectively the same things, one using a default comparison and the
other using an explicitly-specified one.  I'm truly not sure where we
ought to land on this one.

>  -    let result = try base._withUnsafeMutableBufferPointerIfSupported(body)
> +    let result = try base._withUnsafeMutableBufferPointerIfSupported(invoke: body)
>
> I hate "ifSupported" 

Me too.

> but that's another discussion

Quite.  It's also an internal API so it's not an evolution issue.  The
point of having that change as part of the diff (as with all the other
use sites), is to observe how the changes affect real usage.

> (withSupportedUnsafeMutableBufferPointer,
> withAvailableUnsafeMutableBufferPointer, it's all lipstick)
>
> This is procedural, so `do` or `perform` rather than `invoke`
>
> -      for test in removeFirstTests.filter({ $0.numberToRemove == 1 }) {
> +      for test in removeFirstTests.filter(
> +        suchThat: { $0.numberToRemove == 1 }
>
> The difference between `filter` and `forEach` is that `forEach` is explicitly 
> procedural while `filter` is functional.  I do not like functional chainable
> calls being modified to use explicit external labels in this way. 
>
> I'd prefer no label here.

Can you provide rationale for treating functional methods differently,
or is it just personal taste?

> public func split(
>       maxSplits: Int = Int.max,
>       omittingEmptySubsequences: Bool = true,
> -    isSeparator: @noescape (Base.Iterator.Element) throws -> Bool
> +    separatedWhere isSeparator: @noescape (Base.Iterator.Element) throws -> Bool
>
> I'm torn on this one. It's not the worst ever but something more like where/at/when
> makes more sense to me than
> "separatedWhere/separatedAt/separatedWhen".

The biggest reason to keep “separate” in the name is the connection with
the semantics of the other `split` method, that takes a single element
that is tested for equality.  I think this is very similar to the
`contains(elementWhere` vs `containsElement(where` discussion.  If you
leave “separate” out of the name it fails to imply that those elements
for which the predicate returns true are not present in the result.

>
> +      count: __manager._headerPointer.pointee.count)
>
> For the sake of Zippy the Pinhead, surely there has to be something better than `pointee`.
> Like...`reference`?

It's not a reference; it's a value.  But that, my friend, is an
*entirely* different discussion. Let's try to stick to the scope of the
proposal: names and labels for parameters of function type, OK?

-- 
-Dave