Support use of an optional label for the first trailing closure

Evolution Pitches
21

I think the second of these sentences is plausible only under the "can need" assumption of the first, and that we should also discuss making this "can need" scenario go away.

Specifically, imagine a future where the syntax rule is that the trailing closure label may be omitted for a particular function signature if there is only one possible function parameter that it could mean.

Now imagine a rule that prohibits functions from having signatures that differ only in the parameter label of a closure that satisfies the above rule. That is, two functions that have a single only one closure parameter cannot differ only in the label on that parameter.

For example, it wouldn't be possible for a type to have first(where:) and first(without:). Or first(where:) and first(_:). (I'm assuming all of those parameters are closures.)

Of course, this is source breaking, but I would argue that it's source breaking in a good way. There seems to be a lot of consensus that the construct is already confusing and actively harmful.

(Personally, I'd like "necessarily single" trailing closures to have no label, or to have an optional label, and all "potentially or actually multiple" trailing closures to require all labels.)

22

Proposal looks great to me. I'll be one those "unsettled" by the floating labels in the single closure case, but those should be easily removable. I'm not particularly motived by the single closure label examples, as I'm not sure where brings much of anything to functions like first or filter, but the multi label examples are much more motivating.

2 Likes
23

Thanks a lot @xwu for putting that proposal together. That's very well put and detailed, and I have to admit that I think that's a good way forward and probably the best solution to everyone's frustrations with SE-279.

I think we could indeed benefit later to give control to the API author to disallow first label elision at call site with an attribute like you suggested for future improvements but that indeed can wait until later and until we let experience show how this turns out with current design.

My main concern left with all that (SE-0279 + this) is the potential effect on parser performance; I hope that amount of leniency in closure syntax won't worsen compile time too much.

(PS: spotted a typo on your first post: in the very last snippet of that post you used func keyword even for the 2 call sites)

4 Likes
24

I want to add a couple of examples to clarify the behavior of the attribute I am proposing. We'll consider this example:

extension Result {
   func fold<T>(success: (Success) -> T, failure: (Failure) -> T) -> T { ... }
}
// under the proposal, this is allowed:
let wasSuccessful = result.fold success: { _ in true } failure: { _ in false }
// under the proposal, this is also allowed:
let wasSuccessful = result.fold { _ in true } failure: { _ in false }

With the attribute I propose:

extension Result {
   // this attribute enforces the use of labels at all call sites
   @require_trailing_closure_labels
   func fold<T>(success: (Success) -> T, failure: (Failure) -> T) -> T { ... }
}
// this is now required:
let wasSuccessful = result.fold success: { _ in true } failure: { _ in false }
// this now produces a compiler error:
// "`fold` requires trailing closure labels, `success` was elided:" 
let wasSuccessful = result.fold { _ in true } failure: { _ in false }

The require_trailing_closure_labels is a strawman, if you like this idea please suggest something better.

5 Likes
25

I never write first in trailing closure form specifically because I think the label adds crucial clarity at the call site. As others have mentioned, drop(while:) is an even better example of where the label is essential.

8 Likes
26

I have to disagree there. Those labels provide no additional context beyond what you get with first or drop, as they largely serve as conjunctions in the current naming. You'd need the other part of the phrase for them to really be valuable, like first(matching) or drop(allMatching). This is also why I think the labels should always be optional: once you learn what the closures do, the labels are superfluous. Of course, this really only applies to frequently used closures, and so should be left to the discretion of the user. To me, the attribute is only interesting in that it would prevent the automatic completion of the new multi closure syntax for specific APIs which don't support it well. Of course, users could get similar control through improved tooling, so I'm not sure which way I'd want to go there.

2 Likes
27

I think it's less important to be concerned about specific examples from the standard library and just realize that it's possible to write APIs where writing the current trailing closure syntax means you lose clarity. For example, if you have a function call with a single trailing closure argument and non-closure arguments preceding it, there is nowhere that you can place a label to clarify what the purpose of that closure is, and I don't consider the status quo or "don't use trailing closure syntax" to be satisfying.

3 Likes
28

True, which is why the optionality makes sense to me: there's no one size fits all solution here given the sheer flexibility of closure parameters. So personally I don't find many single closure cases particularly motivating, except perhaps in very complex cases (we have one or two such cases in Alamofire). That's why I support the optional nature of this proposal over an alternative which requires them, or renamings that change the base function.

3 Likes
29

Imagine drop(while:) and a hypothetical drop(where:). Leaving aside the fact that the label is necessary for disambiguation, it is also essential for clarity. The fact that the latter does not exist in the standard library does not mean drop(while:) is clear at the point of use without a label. A reader unfamiliar with Swift’s standard library may easily assume the latter behavior.

4 Likes
30

I would argue that they're both poorly labeled, as they both sound like they do the same thing to me. :man_shrugging:

Ultimately, I don' think this proposal should go as far as also adding an attribute to control label visibility.

2 Likes
31

This proposal actually allows callers that use a label to avoid a bunch of parser logic specific to matching unlabeled trailing closures. You can inspect the proof-of-concept implementation to assure yourself that that's the case.

6 Likes
32

@xwu, thank you for putting together this proposal. In addition to being incredibly well-written, it does a fantastic job of laying out the motivations for the optional first label.

I especially appreciated the motivation for the ability to disambiguate the intended matching parameter as this was one of the rough edges of SE-0279 for me (and for others that I've talked with.) Being able to provide the first label will reduce the cognitive load on the programmer by allowing them to declare what they expect rather than stepping through the rules of backwards scanning to deduce what the compiler will choose.

The illustrations laid out in the above section were extremely helpful in visualizing how this form might be preferred over others. Calling it out as a possibility was a good move, but I might even consider increasing its prominence in the actual proposal.

I think the future directions are interesting, though I worry that concise single trailing closure APIs such as DispatchQueue.async { ... } would suffer under mandatory labels.

Again, many thanks for putting this together!

2 Likes
33

Thanks for this, it's extremely well-written and for the most part reflects perfectly the ideas and concerns that were manifested in the SE-279 review and acceptance threads. Here's my two cents (probably just one).

I would accept the proposal as-is, but a small improvement I can see is a stronger story for signaling the elision of the first argument label at call site when the label is not _:. I honestly would go as far as making the compiler emit a warning: if the label for the first closure is not _: then it should be written explicitly, and it will become an error in a future version of Swift. I know that this will cause warnings in many codebases where APIs were used as suggested by the guidelines (including my codebases), but I don't care: we need to fix this, and we might as well start now. I really don't think one should worry about improvements to the language that cause warnings in valid codebases, but maybe it's just me.

Another option would be, as @anandabits suggested, to add an annotation to require the label, that will cause an error if it's elided: it's fine, but we already have a tool for eliding parameter labels, that is, _:, so I would simply leverage that. Also, my future self, while writing new shining APIs that use multiple closures and take advantage of SE-279, will probably not want to add that annotation over and over again.

Anyway, can't wait for this to be accepted and implemented :smiley:

One minor quirk is that, because SE-279 has been already implemented in Swift 5.3, there will be a period of "limbo" when I will probably write APIs ready to be deprecated when Swift 5.4 is released (or something like that) :smiley:

6 Likes
34

To be clear, this is my preference as well. The attribute I propose is in anticipation of the community at large and Core Team in particular not being willing to accept the source churn a warning for all misaligned call sites would cause.

1 Like
35

Maybe (probably) this is a completely horrible idea, but what about combining the first parameter label with the function name?

func count(where: (T) -> Bool) -> Int {
   ...
}

could be called like this:

let result = foos.countWhere { $0 > 5 }

or this

let result = foos.count { $0 > 5 } //assuming there weren't a typechecker problem with count(where:)

but not this

let result = foos.count where: { $0 > 5 }
36

That's essentially what the API naming guideline revision would have authors do in terms of API naming.

As noted in the text, it doesn't address the problem of what to do here:

func foo(bar: Int, baz: Int, boo: () -> Void) { ... }
foo(bar: 42, baz: 42) /* now how do we label 'boo'? */ { ... }

...nor does it solve the issue of what to do when there is more than one possible first trailing closure:

func foo(bar: (() -> Int)? = nil, baz: (() -> Int)? = nil) { ... }
// let's rename to:
func fooBar(_: (() -> Int?) = nil, baz: (() -> Int)? = nil { ... }
fooBar { 42 } // oops, that closure actually goes with 'baz'
3 Likes
37

This has been brought up previously. The consensus is that names like countWhere is anti-swifty.

7 Likes
38

To that point, though, the revised API naming guidelines as a result of SE-0279 would encourage that name.

39

I really like this proposal, it gives flexibility to the user whether to expose the label for the closure or not, and makes sense with the fact that _ arguments are meant to be omitted in the Swift APIs. Also I'm feeling that this won't go through revision due you the firm decision over how labels are treated, which I'd love that they think again about it. The semantics makes sense and the resilience with the APIs as well, so it's a +1 for me.
Great job btw.

40

Really good proposal. It reflects my ideas on the topic quite accurately. I’m in favor of the push towards the new syntax without being too aggressive (warning or even failing to compile when “non-preferred” syntax is used in the current version of Swift). I would, though, like to see the non-preferred syntax slowly phased out in future versions. Overall great work!

-Filip