Name and label changes for closure parameters (SE-0118)

[dabrahams]

Motivation

The names and argument labels of the standard library's closure parameters have been chosen haphazardly, resulting in documentation comments that are inconsistent and often read poorly, and in code completion/documentation that is less helpful than it might be. Because of trailing closure syntax, the choice of argument labels for closures has less impact on use-sites than it might otherwise, but there are many contexts in which labels still do appear, and poorly-chosen labels still hurt readability.

Overview Of Usage Changes

Note: this summary does not illustrate parameter name changes that have also been made pervasively and have an effect on usability of documentation and code completion. Please see the diffs for examples of those.

Before	After
s.withUtf8Buffer(invoke: processBytes)	s.withUtf8Buffer(processBytes)
lines.split(   isSeparator: isAllWhitespace)	lines.split(   whereSeparator: isAllWhitespace)
words.sort(isOrderedBefore: >)	words.sort(by: >)
words.sorted(isOrderedBefore: >)	words.sorted(by: >)
smallest = shapes.min(   isOrderedBefore: haveIncreasingSize)	smallest = shapes.min(   by: haveIncreasingSize)
largest = shapes.max(   isOrderedBefore: haveIncreasingSize)	largest = shapes.max(   by: haveIncreasingSize)
if a.lexicographicallyPrecedes(   b,   isOrderedBefore: haveIncreasingWeight)	if a.lexicographicallyPrecedes(   b, by: haveIncreasingWeight)
ManagedBuffer<Header,Element>.create(   minimumCapacity: 10,   initialValue: makeHeader)	ManagedBuffer<Header,Element>.create(   minimumCapacity: 10,   makingValueWith: makeHeader)
if roots.contains(isPrime) {	if roots.contains(where: isPrime) {
if expected.elementsEqual(   actual, isEquivalent: haveSameValue)	if expected.elementsEqual(   actual, by: haveSameValue)
if names.starts(   with: myFamily,   isEquivalent: areSameExceptForCase) {	if names.starts(   with: myFamily,   by: areSameExceptForCase) {
let sum = measurements.reduce(0, combine: +)	let sum = measurements.reduce(0, +)
UTF8.encode(   scalar, sendingOutputTo: accumulateByte)	UTF8.encode(   scalar, into: accumulateByte)

Future Extensions

The process of making these changes revealed other, very compelling, naming improvements that could be made, e.g. xs.filter(suchThat: isPrime) becoming xs.where(isPrime), which, in addition to being much less awkward, fully clarifies the polarity of the argument. These are considered out of scope for this proposal, but should be considered soon afterward. Once we have started down the path of diverging from terms of art for functional algorithms,

• I think we should consider whether to rename reduce(_ initialResult, accumulatingResultBy nextPartialResult:) to something like accumulating(startingFrom initialAccumulator:combiningBy nextAccumulator) just based on the description in the documentation and on the argument labels.
• The argument against changing other names to be more consistent with API guidelines is weakened. We should consider mapping(_ transformation), flatMapping(_ transformation), and flattened().
• Perhaps contains(where: isPrime) should become containsAny(where: isPrime). It certainly reads better when trailing closure syntax is used.

What's in this pull request?

Resolved bug number: (SR-)

---

Before merging this pull request to apple/swift repository:

• Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
All supported platforms	@swift-ci Please smoke test and merge
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
All supported platforms	@swift-ci Please test and merge
OS X platform	@swift-ci Please test OS X platform
OS X platform	@swift-ci Please benchmark
Linux platform	@swift-ci Please test Linux platform

Lint Testing

Language	Comment
Python	@swift-ci Please Python lint

Note: Only members of the Apple organization can trigger swift-ci.

[karwa]

filter(suchThat isIncluded: => where(isIncluded:

for test in removeFirstTests.where({ $0.numberToRemove == 1 })
Could easily be mistaken for a for _ in _ where _ (filtered for-in) loop. Both situations have the same meaning, so I'm not sure if that's necessarily a problem.

first(where:) is also a bit close, but this time the functionality is very different:

for x in y.first(where:{ _ })! { ... }

could be seen by some as dangerous close to:

for x in y.first! where _ { ... }

[xwu]

Glad to see that this has the core team's attention. I was so confused by the name sorted(isOrderedBefore:) that I filed a bug, assuming that it was unintentional because it didn't line up with the documentation.

Could I make one suggestion? There are several instances where it seems you're fishing for a synonym or a closely related concept in order to supply another verb. By this I mean: "sort, ordering by," or "split, separating where." While it's true that you sort by ordering, or split by separating, perhaps these could be more terse? I include among this "reduce, accumulating result by"--although "reduce" is not self-explanatory, if we're going to regard it as a term of art then there is no other way one may reduce.

Edit: it's internal, but inIncreasingOrder should probably be areInIncreasingOrder :)

[dabrahams]

on Mon Jun 13 2016, Xiaodi Wu <notifications-AT-github.com> wrote:

Could I make one suggestion? There are several instances where it
seems you're fishing for a synonym or a closely related concept in
order to supply another verb. By this I mean: "sort, ordering by," or
"split, separating where." While it's true that you sort by ordering,
or split by separating, perhaps these could be more terse?

Specific suggestions are welcome. We tried really hard to find good names.

I include among this "reduce, accumulating result by"--although
"reduce" is not self-explanatory, if we're going to regard it as a
term of art then there is no other way one may reduce.

Please see the “Further Changes to Consider” section in the pull request text.

Edit: I don’t know why I chose not to use areInIncreasingOrder, as I know I considered it. But in reviewing the results I think I agree with your suggestion.

Dave

[dabrahams]

on Fri Jun 10 2016, karwa <notifications-AT-github.com> wrote:

"for test in removeFirstTests.where({ $0.numberToRemove == 1 })"

Could easily be mistaken for a "for _ in _ where _" (filtered for-in)
loop. Both situations have the same meaning, so I'm not sure if that's
necessarily a problem.

No, I don't think it is a problem, but we might want to warn and offer a
fixit for this usage, as the “for _ in _ where _” form is likely to be
much more efficient.

first(where:) is also a bit close, but this time the functionality is very different:

for x in y.first(where:{ _ })! { ... }

could be seen by some as dangerous close to:

for x in y.first! where _ { ... }

Could be, but this time it's just a matter of seeing the wrong analogy.
The latter is the same as:

for x in y.first!.where(_) { ... }

Dave

[karwa]

For verb-functions which take a single closure (such as sort), maybe a more verbose, obvious return type would be better than another verb as an argument label? Something like this, perhaps:

enum SortRelationship {
    Case CorrectlySorted
    Case Unsorted
}

func sort<A>(_: (A, A)->SortRelationship)

Then you'd call it like:

myArray.sort { 
    let lhsDot = $0.hasPrefix(".")
    let rhsDot = $1.hasPrefix(".")
    If lhsDot && !rhsDot { return .CorrectlySorted }
    If lhs < rhs { return .CorrectlySorted }
    Return .Unsorted
}

[dabrahams]

on Mon Jun 13 2016, Jordan Rose <notifications-AT-github.com> wrote:

I'm not happy with a lot of these names. Is this going to go through
swift-evolution?

Of course. But please accompany your unhappiness with constructive
suggestions ;-). If you do it here, maybe I can avoid putting forth a
proposal you'll be unhappy with.

[dabrahams]

on Mon Jun 13 2016, karwa <notifications-AT-github.com> wrote:

For verb-functions which take a single closure (such as sort), I would
prefer a more verbose, obvious return type over an argument
label.

Changing the default types used ordering comparisons is out-of-scope for
this soon-to-be-proposal. We have plans to address that, but it is a
separate issue. Some of these names can become simpler when we do that.

Dave

[xwu]

On Mon, Jun 13, 2016 at 7:08 PM, Dave Abrahams notifications@github.com
wrote:

Specific suggestions are welcome. We tried really hard to find good
names.

Yes, and I think they are an improvement in terms of readability. TBH,
though, that it's so difficult to find something apt might be a sign that
the external label is not really necessary. Take, for example,
sort(orderingBy:).

• Sort: what I'm about to do is sort
• Ordering: I'll sort by means of ordering elements--kind of redundant, as
  I mentioned
• By: I'll order by invoking the predicate that follows--sure, but also not
  really in question

My initial suggestion would have been sort(by:), which isn't terrible
IMO. I tried to see if a synonym would be provide more help there, but it
only gets more silly--e.g. sort(invoking:) or sort(thusly:). These end
up devolving into really general terms hinting that what follows is a
predicate, which is actually already confirmed by the type. There is no
real possibility of sort(butNotBy:), so in the end nothing in that label
can be really said to clarify the parameter in question. Was there a
specific rationale for putting a label in to begin with?

Please see the “Further Changes to Consider” section in the [pull

request text](#2981 (comment)).

Maybe reducing would be a more conservative (and thus palatable) change,
or since an unfold-like method is on the horizon, folding?

[jrose-apple]

Ha, sorry, I deleted my message after rereading the title and description. My biggest concern was that I really don't think changing the names of filter, reduce, etc is at all in the same scope as changing the label for the closure argument to each of these. The base name is currently a term of art; the closure argument's name isn't, especially given that using a trailing closure here (and thus not needing the label) is probably the common case.

I don't mean that changing those names is out of the question, but that either you should make that the focus of the proposal (and think about all of the major functional operations together), or you should do the argument labels ahead of time.

---

Here's some initial thoughts on specific changes:

• forEach(invoke:) is inconsistent with Dispatch, which settled on execute. Personally, I like just plain do, now that we allow keywords as labels.
• filter(suchThat:) isn't pretty, I agree. filter(for:) is okay. I guess filter(where:) is worrisome because it could mean to filter matching items out.
• contains(elementWhere:) feels like it's indicative of a problem in the API guidelines. We certainly don't use contains(element:), but contains(where:) would sort of be ambiguous (though it technically repeats type information from the generic parameter). Somehow first(where:) sounds fine, though.
• elementsEqual(comparingBy:) is rubbing me the wrong way because of the "by"; anything you pass here is going to be named like a predicate (haveSameTypes, CGRect.intersects, etc.). "with" seems a little better even if it's semantically empty.
• reduce(_:accumulatingResultBy:) also seems weird, and needlessly long. I do see how it's grammatical (Set.union, .appending). The hard case is where the result type is different from the element type, which makes me less happy with using "combiningBy" in the case where the base name gets to change too.
• encode(_:sendingOutputTo:) doesn't feel to me like a metaphor we use, i.e. we don't "send" things "to" functions in Swift. I feel like a terse functional API would use then and Objective-C might just use outputHandler or block (i.e. give up on making it phrasal).

That's all very negative, but really I'm glad that you've decided this is worth thinking about, and that the argument names will all be consistent and descriptive after this, and that I liked all the other names.

[dabrahams]

,it's so difficult to find something apt might be a sign that
the external label is not really necessary. Take, for example,
sort(orderingBy:).

A label is needed because otherwise the argument reads as a direct object.

My initial suggestion would have been sort(by:), which isn't terrible
IMO.

IMO that label should be reserved for usage like

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

Maybe reducing would be a more conservative (and thus palatable) change,
or since an unfold-like method is on the horizon, folding?

I don't see how the first argument can read correctly as a direct object in this case either.

[xwu]

On Mon, Jun 13, 2016 at 10:09 PM, Dave Abrahams notifications@github.com
wrote:

,it's so difficult to find something apt might be a sign that
the external label is not really necessary. Take, for example,
sort(orderingBy:).

A label is needed because otherwise the argument reads as a direct object.

Interesting. I've always read unlabeled predicates as adverbs.
Instinctively, they answer the question "how?" For example, sort(<)
always read to me like "sort ascending." There really isn't a good English
word that you can insert between "sort" and "ascending" to describe their
relationship--"sort in such a way as to cause the elements of the receiver
to be monotonically ascending" is quite a mouthful.

My initial suggestion would have been sort(by:), which isn't terrible
IMO.

IMO that label should be reserved for usage like

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

Edit: The same could be said for sort(orderingBy:). In colloquial usage, both "sort by" and "order by" is typically followed by a noun.

Maybe reducing would be a more conservative (and thus palatable)
change,
or since an unfold-like method is on the horizon, folding?

I don't see how the first argument can read correctly as a direct object
in this case either.

Yeah I'm a little stumped here for the moment.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#2981 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAA2gE2oz3Kg-ROFmU5kiz1SnNx5Pkwmks5qLhtqgaJpZM4IzGvm
.

[dabrahams]

on Mon Jun 13 2016, Jordan Rose <notifications-AT-github.com> wrote:

My biggest concern was that I really don't think changing
the names of filter, reduce, etc is at all in the same scope as
changing the label for the closure argument to each of these.

Now that evolution has taken up
http://news.gmane.org/find-root.php?message_id=CAKA%3djdbxiC%5fq0Fe%3dXc%3dmSWu7OtGxMgAQyb0QMA6%5fOaiiLB3foA%40mail.gmail.com
as a separate topic, I have pulled those kinds of questions out of the
proposal. We'll handle them separately.

Here's some initial thoughts on specific changes:

• forEach(invoke:) is inconsistent with Dispatch, which settled on
  execute. Personally, I like just plain do, now that we allow
  keywords as labels.

I think we considered do and had reasons to like invoke. @moiseev,
do you remember this?

• filter(suchThat:) isn't pretty, I agree. filter(for:) is okay.

xs.filter(for: isPrime)? Am I supposed to think of the verb “filter”
like the verb “hunt?”

I guess filter(where:) is worrisome because it could mean to filter
matching items out.

Exactly. The purpose of suchThat is to clarify the polarity, to the
best of our ability. The fact that it only barely succeeds is the
reason we should have a look at renaming filter.

• contains(elementWhere:) feels like it's indicative of a problem in
  the API guidelines. We certainly don't use contains(element:), but
  contains(where:) would sort of be ambiguous (though it technically
  repeats type information from the generic parameter).
  Somehow first(where:) sounds fine, though.

I don't understand what your problem is here. The word “element” is
just there because it's needed to make the call read grammatically.
There's no law against repeating type information; it's just something
you're not supposed to do without a reason.

• elementsEqual(comparingBy:) is rubbing me the wrong way because of
  the "by"; anything you pass here is going to be named like a predicate
  (haveSameTypes, CGRect.intersects, etc.). "with" seems a little
  better even if it's semantically empty.

It's worse because it's ambiguous, with the likely interpretation
“against,” as in “compare A with B” = “compare A against B”. I don't
love “by,” but haven't been able to come up with anything better.

• reduce(_:accumulatingResultBy:) also seems weird, and needlessly
  long. I do see how it's grammatical (Set.union, .appending). The
  hard case is where the result type is different from the element type,
  which makes me less happy with using "combiningBy" in the case where
  the base name gets to change too.

The fact that this one is too long is an argument for changing “reduce”
to “accumulate,” which is out-of-scope here.

• encode(_:sendingOutputTo:) doesn't feel to me like a metaphor we
  use, i.e. we don't "send" things "to" functions in Swift. I feel like
  a terse functional API would use then

I don't see how that makes any sense at all. The closure isn't
a continuation.

Dave

[moiseev]

• forEach(invoke:) is inconsistent with Dispatch, which settled on
  execute. Personally, I like just plain do, now that we allow
  keywords as labels.

I think we considered do and had reasons to like invoke. @moiseev,
do you remember this?

I remember it was mentioned during our discussions, but don't remember specific arguments against it, apart from, maybe, that because do is a keyword that means something a little different, it might lead to confusion. do the keyword does not have anything to do with iteration.

What I remember clearly is that eval or execute do not really communicate the impure nature of the closure as good as invoke. But to be fair, do is not at all bad in this respect.

[dabrahams]

@swift-ci Please test

[dabrahams]

@swift-ci Please test and merge

[dabrahams]

@swift-ci Please test and merge

[dabrahams]

@swift-ci Please test and merge

[dabrahams]

@swift-ci Please test and merge

[dabrahams]

The failure is due to a race condition, so I’m merging.

[dabrahams]

Merged in as #3589

[atrick]

Dave, what about these failures?

TestFoundation/TestNSString.swift:1094:56: error: incorrect argument label in call (have 'with:isEquivalent:', expected 'with:by:')
let expectHasPrefix = lhsNFDGraphemeClusters.starts(
^
TestFoundation/TestNSString.swift:1097:54: error: incorrect argument label in call (have 'with:isEquivalent:', expected 'with:by:')
lhsNFDGraphemeClusters.lazy.reversed().starts(
^

Are you running build-toolchain on linux before merging?