Renaming Trailing-closure Functions in the Standard Library

Background

SE-0279, which is currently in second review, includes the following:

It is in the proposal as part of the explanation for the label handling with multiple trailing closures, but the guidance applies just as much to existing trailing closures. Using argument labels for "trailable" closures is at best semi-pointless and sometimes actively harmful. This becomes clear when you look at some standard library examples:

// this reads well
array.first(where: { criteria })
// but this is not at all clear
array.first { criteria }

// this reads... eh, ok, still a little ambiguous
array.drop(while: { criteria })
// but is this filtering all of them out? dropping after first match?
array.drop { criteria }

The simple solution of applying the beneficial argument label to the base name instead would make these considerably more readable regardless of whether the trailing closure is applied:

array.firstWhere({ criteria })
array.firstWhere { criteria }

array.dropWhile({ criteria })
array.dropWhile { criteria }

With trailing closures as they stand, it turns out it is never a good idea to make your trailing closure label meaningful because it will be left off 90% of the time, and having a label that appears only when parentheses happen to wrap the closure isn't a good practice.

Additionally, we have the case of SE-0220 which introduced Sequence.count(where: (Element)->Bool), which had to be backed out because the basename was ambiguous with Collection.count, leading to performance problems in the type checker:

// SE-0220 caused expressions like these to take longer to type check
queue.count + (task?.queue.count ?? 0)

In this example, the typechecker must consider whether queue.count was referring to Collection.count or the function ((Int) -> Bool) -> Int. Eventually it can figure it out based on the integer literal, but this massively expanded the search space to explore, lengthening compile times. Re-introducing count(where:) as countWhere(_:) would both improve readability at the call site and sidestep the typechecker issue.

Proposal

A review of the standard library should be undertaken on all high-order functions to determine whether their argument label is important, and if so, recommend adding a second method with the argument label hoisted into the basename. This would be source breaking, but justified under the "active harm" exception, since the readability of methods such as drop(while:) is currently severely impaired by dropping the argument label. The original method should be deprecated over time (probably when we next introduce a new language variant).

Note, not all argument labels are necessary for readability, and would have been better left out altogether. These should be left alone rather than changed for consistency, since the bar for source breakage is high.

31 Likes

I approve of this particular pitch, and I also approve of the Core Team's willingness to consider such changes when confronted with new information.

9 Likes

+1. I think this would be a significant improvement regardless of the outcome of SE-0279.

(Belated edit: this assumes that labels on first trailing closures are off the table. A far better solution in terms of language coherency would be to support labelling the first closure, and deprecate dropping the label if it isn’t _.)

3 Likes

This is good. Methods should be named what you would store them as.

let array = [1, 2, 3]

let firstWhere = array.first(where:)
let first = try firstWhere { $0 > 2 }

let dropWhile = array.drop(while:)
let after2 = try dropWhile { $0 <= 2 }
5 Likes

This feels like an unfortunate workaround for the fact that the first trailing closure doesn't permit being labeled. Why not change that, instead of changing the name of the function (and thus breaking the consistency of the Swift API naming guidelines as well)?

In the proposal review thread, @John_McCall wrote:

This isn't hypothetical; this thread and these standard library functions are the lived experience.

Another reason I would prefer a labeled closure vs. moving the label into the basename is something I raised in the initial proposal review: the syntactic ambiguity when using a trailing closure in a conditional statement. Today, we can't do the following, because the parser can't look ahead far enough to know whether this is a function with a trailing closure or a property access followed by the conditional body. This forces us to use an unnatural syntax for these methods, only when they occur in conditionals:

// error
if array.first { $0 < minValue } > 0 { ... }

// works, but unnatural
if array.first(where: { $0 < minValue }) > 0 { ... }
if (array.first { $0 < minValue }) > 0 { ... }

The proposed remedy won't fix this:

// still an error
if array.firstWhere { $0 < minValue } > 0 { ... }

However, if we had a way to require the label on the trailing closure, the parsing ambiguity would be eliminated; the presence of the colon following the label would unambiguously indicate that the subsequent { begins a trailing closure and not the conditional body. Furthermore, it lets us use the same syntax for any occurrence of this function call, regardless of surrounding syntactic context:

// good, reads well, consistent
if array.first where: { $0 < minValue } > 0 { ... }

That probably won't resolve the type checker performance issue for count, though. I don't have a great answer to that; one that comes to mind would be to require any function with a required trailing function label to be referenced using its full decl-name and not permit it to be referenced by its base name alone:

let x = queue.count  // excludes count(where:)
let x = queue.count(where:)

I'll be the first to admit that's a bit awkward, though. But I'd love to see a more holistic solution to this problem.

62 Likes

I second what @allevato said. :point_up:

Personally array.first(where: closure) reads and looks way better than array.fristWhere(closure). It doesn't look very swifty to me in this particular case and feels more like a regression or workaround. If the multiple trailing closure proposal gets accepted then we might want to tackle the disambiguation issue where we can explicitly use the label of the first trailing parameter. This would let us to keep array.first(where: closure) and additionally allow to write array.first where: { ... } where appropriate.

24 Likes
// this reads well 
array.first(where: { criteria })
// but this is not at all clear 
array.first { criteria }

I agree, second one isn't good. Thankfully we can write it the first way!
There is a solution in place already, so why do we have to change the stdlib?

I don't see how firstWhere(...) is more readable than first(where: ...).

I don't understand why this. Some labels are useful enough to include it in your code. Some labels are not. That's why I include some of the labels in my code, and don't include some of them. For example I when I call first(where:) function, I always write the label down.

Current situation:

  +----------------------------------+ 
  |Is the label meaningful enough    | 
  |to include it in your source code?| 
  +----------------------------------+ 
    |                              |   
    |                              |   
   Yes                             |   
    |                             No   
    v                              |   
 Include it in your source code    |   
                                   v  
                                Skip it

What you are suggesting:

  +----------------------------------+ 
  |Is the label meaningful enough    | 
  |to include it in your source code?| 
  +----------------------------------+ 
           |           |               
           |           |               
          Yes         No               
           |           |               
           V           V               
    Include it in your source code  

Would that be better for people that think that these labels are useful? I don't think so, they should be writing them already. Would that be better for people that think that these labels should be skipped? I don't think so, you would be forcing them to go against what they want.

I feel like Require parameter names when referencing to functions is a better solution for that. Renaming specific functions one by one doesn't solve the root of the problem.

21 Likes

IMO making argument labels optional at the call site, and potentially needing some mechanism to relay from the API to the IDE or linter "hey, no, my argument label matters", isn't a good strategy. OTOH making argument labels mandatory at the call site for trailing closures seems completely unacceptable to many people.

The language is as it is and the standard library and naming guidelines should reflect this, rather than modifying the language to match the current guidelines.

1 Like

I just want to point out that this isn't new information. Nearly all of the APIs considered in this thread so far, if they went through evolution, had feedback given about their trailing closure form not being as readable as the non-trailing closure form. To suddenly pretend that we've only realized this now seems very weird to me. This has always been the trailing closure tradeoff: succinct closure usage that loses whatever label was applied to that closure.

10 Likes

That solution is not tenable. The language has trailing closures for a reason, and expecting people to choose to forego trailing closures and add parentheses whenever a label looks kinda maybe important is not an acceptable solution.

2 Likes

Here are two relevant points found in the Swift API Design Guidelines:

When the first argument forms part of a prepositional phrase, give it an argument label. The argument label should normally begin at the preposition, e.g. x.removeBoxes(havingLength: 12).

An exception arises when the first two arguments represent parts of a single abstraction.

a.move(toX: b, y: c)
a.fade(fromRed: b, green: c, blue: d)

In such cases, begin the argument label after the preposition, to keep the abstraction clear.

a.moveTo(x: b, y: c)
a.fadeFrom(red: b, green: c, blue: d)

Otherwise, if the first argument forms part of a grammatical phrase, omit its label, appending any preceding words to the base name, e.g. x.addSubview(y)

This guideline implies that if the first argument doesn’t form part of a grammatical phrase, it should have a label.

 view.dismiss(animated: false)
 let text = words.split(maxSplits: 12)
 let studentsByName = students.sorted(isOrderedBefore: Student.namePrecedes)

Note that it’s important that the phrase convey the correct meaning. The following would be grammatical but would express the wrong thing.

view.dismiss(false)   Don't dismiss? Dismiss a Bool?
words.split(12)       Split the number 12?

Note also that arguments with default values can be omitted, and in that case do not form part of a grammatical phrase, so they should always have labels.

I'm not a native speaker, but to me the examples of the where-predicate above and a lot of other problematic labels when omitted look like such "prepositional phrases".

I believe that splitting the method name across method name and argument labels is the correct choice in these scenarios, they read very well at point of use. It is unfortunate that this leads to some ambiguities for the type checker and issues with trailing closures that omit these labels, but I don't think those are worth changing the API Design Guidelines that have been established over many years.

18 Likes
@controlFlow
func foo(_ callback: Callback) { ... }

if foo { ... } { ... }

The only other alternative I can think of to the “nesting trailing closure in if condition” issue is adding an attribute to functions with trailing closures that indicates any overrides must have at least one closure at the end of the argument list. If this is implemented they could be used in if statements. However, I may be missing something obvious or there may be performance issues with this.

To me, this feels way too elaborate, compared to the "the Russians used a pencil" solution we already have.

1 Like

I am inclined to agree, but most likely these functions with trailing closures are written by library developers. There are already many attributes required for library developers to properly annotate on their functions.

I think first(where:) reads just fine as-is in trailing-closure form.

var numbers: [Int] = ...

let x = numbers.first{ $0 < 10 }

I do not want to add “where” into calls like that.

11 Likes

I'd argue that if the user chooses to write the label, it matters, so a linter shouldn't remove it. An open question would be whether auto-complete would insert it or not, and that could be conditioned on whether the API says it's required. Yes, that would require a new special attribute or tagging mechanism of some sort, so I agree it's not ideal.

The horse is already out of the barn, unfortunately. Like you said, I don't think we can require labels for all trailing closures at this point (nor would it be appropriate for all APIs), but I also think that names like firstWhere and countWhere are a net loss in terms of readability that we're going to be stuck with. It also continues to permit strange syntactic ambiguities that occur in extremely frequent uses of these stdlib functions to continue on. The fact that a call to many Sequence/Collection algorithms in Swift has to be written differently when it occurs in a conditional statement is a real wart, and this also has real consequences for tooling: for example, swift-format has a rule that removes unnecessary parentheses around {if|guard|while} (condition), but we have to take special care to not do this if condition is a function call with a trailing closure.

I don't think there's going to be a perfect strategy here, but I see changes like this as chipping away at what was previously some really nice consistency and beauty of Swift APIs and language design to try to workaround mistakes previously made in the language, and it's unfortunate. That's not a criticism—sometimes design choices don't work out or aren't realized to be non-ideal until they've gone through lived experience, but I do hope we can achieve something better here than just renaming the functions.

25 Likes

An extension that allows a label wouldn't suffice to make drop(while:) a good API name; you'd also need a way to force the use of a label, which would then break compatibility with all the existing uses of this API. I'm sympathetic to the idea of allowing API authors more control over call sites, but that's a lot of change for something that seems to be more easily solved.

I'm not convinced that count(where:) and first(where:) would need renaming if we had a satisfactory solution to the type-checking performance issue.

4 Likes

This is a fine point to debate, but under this guidance you would be debating whether it fits into the "shouldn't have a label" bucket instead of the "should have been in the base name" bucket. Either where: is necessary or it isn't. It shouldn't be present or not based purely on whether you make the argument trailing or not.

5 Likes

The criteria I laid out at the start was active harm (unlike SE-0132 which was more "these names are better").

drop(while:) is clearly broken. min(by:) otoh is fine from this perspective. I suspect we're talking about just a couple of methods here that actually deserve renaming.

2 Likes

Most of those are named wrong. A function with a noun name is named wrong.

Programmers in general don't know this, because it's only very evident when working with stored functions.

extension Array {
  func methodScope() {
    // A good noun-name, suitable for a property. 😻
    let first = self.first

    // Good verb-names.
    let contains = self.contains
    let drop = self.drop // `dropWhile` is better though.
    let split = self.split(maxSplits:omittingEmptySubsequences:whereSeparator:)
    typealias GetBoolFromComparing = (Self, (Element, Element) -> Bool) throws -> Bool
    let starts = self.starts as GetBoolFromComparing
    let elementsEqual = self.elementsEqual as GetBoolFromComparing

    // Incorrect names.
    // Either add a `get` prefix
    // or a suffix that is the argument label
    // to make them correct.
    let getFirst = first(where:)
    let getFirstIndex = firstIndex
    let getLastIndex = lastIndex
    let getSorted = sorted
    let getPrefix = prefix(while:)
    let getMin = min
    let getMax = max
  }
}

get is more versatile, because it extends to any number of arguments. But this pitch is about the special case of single arguments, where suffixes are clearer.

Moving argument labels out is a hack, because we can't give variables argument labels yet (see below). When that happens (which may require a new language?), we can reevaluate the noun rule. Until then, add get or move the labels out!

let array = [1, 2, 3]
let first(where:) = array.first
var three = first(where:) { $0 > 2 }
three = array.first(where:) { $0 > 2 }
three = first( where: { $0 > 2 } ) 
three = array.first( where: { $0 > 2 } )
three = first { $0 > 2 }
three = array.first { $0 > 2 }