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

[Dave Abrahams]

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

> On Jun 25, 2016, at 4:25 PM, Dave Abrahams <dabrahams at apple.com> wrote:
>> 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:
>>> 
>>> -    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.
>
> When using a procedural "trailable" closure inside parentheses, the intention
> to do so should be clear:
>
> p(x, perform: {...})
> p(x, do: {...})
>
> vs
>
> p(x) {
>    ...
> }
>
> Anyone reading this code can immediately identify that an otherwise trailing
> closure has been pulled inside the signature because the call has become 
> significantly more  complex. The point-of-coding decision ripples through
> to point-of-reading/point-of-maintenance.
>
>>> 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`?   
>
> I read through the pull request. I grouped related modifications
> together, although not exhaustively. They fall under the same umbrella, 
> choosing `do` or `perform` compared to `invoke` or `invoking`.
>
> -    _forAllPermutationsImpl(index + 1, size, &perm, &visited, body)
> +    _forAllPermutationsImpl(index + 1, size, &perm, &visited, invoke: body)
>
> -public func runRaceTest(trials: Int, threads: Int? = nil, body: () -> ()) {
> +public func runRaceTest(
> +  trials: Int, threads: Int? = nil, invoking body: () -> ()
> +) {
>
> -public func expectFailure(${TRACE}, body: () -> Void) {
> +public func expectFailure(${TRACE}, invoking body: () -> Void) {

OK, I understand your process now.  I still don't understand your
rationale for saying `do` or `perform` is better than `invoke`.  Or is
it just personal taste?

>> 
>>> 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.
>
> You did, as in the examples just above, which you recommend a rename to `invoke` or
> `invoking`.

Ah, thanks.

>>> -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?
>
> Strong personal taste, backed by tech writing. Simpler words. 
> From the Corpus of Contemporary American English:
> do: 18
> perform: 955
> invoke: does not appear on list.

I'm unable to reproduce those numbers using http://corpus.byu.edu/coca/.
What exactly did you do to get them?

Also, I'm not sure commonality of use is a good rationale.  I bet
“function” doesn't appear as often as “task” either, but the argument
here is a function and we invoke functions.  Sometimes the appropriate
word is just less common.

>
>> 
>>> -  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.
>
> I get your point but I think it's unnecessarily fussy. 

I plead guilty to sometimes having unnecessarily fussy tendencies, but
in this case I believe strongly that “byComparing” would be actively
misleading and harmful.  Even if there's only one possible sane
interpretation, if readers have to scratch their heads and do an
exhaustive search through possible interpretations of what something
might mean because the one directly implied by the grammar is nonsense,
that's bad.

> Alternatives are slim on the ground. `usingComparison:` is too long,
> as is `byComparingWith:` (which still reads better but you will point
> out can be mistaken by some pedant to mean that the sequence is being
> compared to the closure), and you won't allow for `comparing:`.  I'm
> not in love with `comparingWith:` but it reads slightly better to me
> than `comparingBy:`.

There is definitely something awkward about “xxxBy:” for these uses, and
I'm open to using “xxxWith:” instead, even though as you say it's
confusable.

  if x.starts(with: y, comparingBy: { $0.name == $1.name }) {
    ...
  }

  if x.starts(with: y, comparingWith: { $0.name == $1.name }) {
    ...
  }

At some point I start to wonder if giving up fluency is best:

  if x.starts(with: y, equality: { $0.name == $1.name }) {
    ...
  }

>>> min/max, byOrdering
>> 
>> Likewise, you're not ordering the closure.
>
> Same reasoning.
>
>> 
>>> -      ).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.”
>
> For a language that treasures concision and clarity, this may be clear
> but it's notably inconcise. (Yes, that is a new word.)

I agree, but if you want concision I'd rather stick with something that
doesn't imply anything unintended, such as “to” or “into.”

>>> -  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})
>
> But they're sorting now, and that feature isn't in Swift.

That doesn't mean we should claim the syntax for another purpose.
I think we probably do want that feature eventually.

> However, `sorted` falls under another umbrella, which is potential
> chaining. In such case, I would prefer there be no internal label at all
> to allow cleaner functional flow.

IMO whether a method has a non-Void return value (and thus is chainable)
should not be the determining feature on whether we use argument labels.

We *could* decide to drop all labels for trailing closures (on the
grounds that they often won't appear at the call site) and say the
policy is that if there's something you want to communicate about the
closure's role, you simply do the best you can with the parameter name
and let the clarity at the call site fend for itself.  That, at least,
would be a reasonably principled approach.

>>> -  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.
>
> I actually meant to move this up to the other discussion of `byComparing` and
> forgot to. So please assume you disagree with me on this one too.
>
>>> 
>>> -  /// 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 like `predicate`. I endorse `predicate`. Does this mean the rule of "must
> read like a sentence" can be overturned for things like "comparison" and
> "order"? If so, woo!

I don't know what you mean.  There is no such rule.  There are
guidelines about reading like a phrase at the use site, but the
*parameter name* has no effect on the use site.

>>> 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.
>
> To quote greatness:, "I personally find `soEach` repulsive".
> (cite:
> http://article.gmane.org/gmane.comp.lang.swift.evolution/16998/match=repulsive
> <http://article.gmane.org/gmane.comp.lang.swift.evolution/16998/match=repulsive>)

Sorry to hear that.  Let's change “filter” to “where” so this can become
a non-issue.

>> 
>>> -      || 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.
>
> May I recommend `predicate:` then since it looks like that's actually
> a possibility?

I never suggested predicate as an argument label.

>>> -    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.
>
> Woody Allen: "The heart wants what it wants"
> Me: "The spleen vents what it vents"

SOL (snort out loud)

>> 
>>> (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?
>
> Functional programming flow. I follow Kevin Ballard's rule of parens around
> functional elements and naked braces for trailing closures that do not return
> values. This ensures the compiler is never confused at places like:
>
> for x in foo when y.f{...} {
>     ...
> }

? It's never confused there; it tells you it's illegal.

> and instantly identifies to the reader when there's a non-returning scope, as
> in forEach or GCD dispatch.

Why is it useful to identify when there's a scope that doesn't return a
value?  Isn't that already clear from whether the value is assigned
somewhere?  

-- 
-Dave