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

[Dave Abrahams]

on Wed Jun 22 2016, David Hart <david-AT-hartbit.com> wrote:

> I did not pay a lot of attention to this renaming because I thought
> the trailing closure syntax would shield me from labels. But I forgot
> that in if/for/while statements, I’m going to be forced to use
> them. In those cases, I’d prefer more succinct labels.
>
> I added some comments inline where I have issues with the new labels:
>
>> On 23 Jun 2016, at 04:02, Erica Sadun via swift-evolution <swift-evolution at swift.org> wrote:
>> 
>> On Jun 20, 2016, at 3:25 PM, Dave Abrahams via swift-evolution
>> <swift-evolution at swift.org <mailto: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 <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".
>> 
>> That said, would prefer `do` or `perform` over `invoke` or `invoking` as in
>> `runRaceTest`, `_forAllPermutationsImpl`, `expectFailure`, etc. This also applies
>> where there's a `body` label instead of an empty external label.
>> 
>> -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`.
>> 
>> -  return IteratorSequence(it).reduce(initial, combine: f)
>> +  return IteratorSequence(it).reduce(initial, accumulatingBy: f)
>> 
>> For `reduce`, I'd prefer `applying:` or `byApplying:`
>> 
>> Similarly in `starts(with:comparingBy:)`, I'd prefer byComparing`,
>> min/max, byOrdering
>> 
>> -      ).encode(encoding, output: output)
>> +      ).encode(encoding, sendingOutputTo: processCodeUnit)
>> 
>> How about `exportingTo`?
>
> I know this label does not need to be necessarily short, but I’d
> prefer Erica’s suggestion, or even `to`. encode(a, to: b) reads well
> to me.

Makes sense.

>> -  tempwords.sort(isOrderedBefore: <)
>> +  tempwords.sort(orderingBy: <)
>> 
>> With `sort` and `sorted`, I'd prefer `by:`
>
> I also agree that `by` is more than enough. `sort` already implies an
> `ordering`.

See my reply to Erica.

>> -  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.
>
> This one has a noticeable chance of appearing in conditional clauses
> so I’d like to go for something short. How about `by`?

You know, it's not bad.  Maybe it's just the fever talking but I am
starting to like these short labels.

>> -  /// 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? 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.
>> 
>> -      || 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.
>
> Agreed. Again, same argument. As this is a boolean function, this has
> a higher chance of appearing in conditional clauses so should be
> short. `where` works well for me.

  if friends.contains(where: {$0.firstName == "ted"}) { ... }

?  `contains(where:` doesn't seem very fluent to me.

>>  -    let result = try base._withUnsafeMutableBufferPointerIfSupported(body)
>> +    let result = try base._withUnsafeMutableBufferPointerIfSupported(invoke: body)
>> 
>> I hate "ifSupported" but that's another discussion (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.
>
> Quick aside. Eric, if you prefer no label here, why did you not also
> prefer no labels for contains and elementsEqual? They also have very
> functional. But if we must have a label, I’d vote for `where`.

IMO `where` fails to correct one of the great weaknesses of the basename
`filter`, which is the strong implication that the predicate's meaning
is the inverse of what it actually is.  That's why I favor `soEach`.

>> 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”.
>
> I care less about this one, but `where` also sounds better.

As I noted in my reply to Erica, I think `where` without some reference
to the separator is potentially misleading.

Perhaps

    contiguousNonPrimes = (0..<100).split(whereSeparator: isPrime)

would be better for use sites than what's in the patch, though.

-- 
-Dave