Renaming Trailing-closure Functions in the Standard Library

For countless times I had to do things like array.first(where: { criteria }), instead of using trailing closures, just so the code could be semantically clear, at the cost of visual cluttering.

No source-breaking changes, then. I like it.

1 Like

My mind changed after reading @allevato's comment.

I want to add another potential benefit of labeling the first closure instead of adding second functions. Once this idea is accepted and implemented, lots of people are bound to modify their existing code to make them more clear semantically. It's relatively, albeit slightly, easier to add a label to an existing trailing closure than changing the function's name. It also probably gives the IDE an easier time indexing the code during the change. And, allowing labels to first closures doesn't inflate the number of standard library functions.

7 Likes

It is worthwhile to address this problem, but I agree with the tenor of the conversation--and particularly @allevato's reply--that we should look at it on a more holistic level.

In your replies, you have scoped this discussion narrowly such that both changes to language syntax and changes to the naming of other APIs are out of the question. I do not believe we can carry on a conversation about exploring meaningful options to the problem when scoped so narrowly.

That said, we do not need to go on a wild goose chase here. I think there is a simple change to the API design guidelines that would speak to this problem without requiring large-scale changes. In a way, I think it's rather implicit to the existing overarching goal for clarity and drop(while:) simply is poorly named by any metric. I would phrase the rule such as this--

Functions may be referenced in certain contexts by their base name only, excluding any argument labels. This includes--but is not limited to--the case of trailing closures. Avoid naming functions in ways where omission of argument labels would cause the use site to be actively misleading. For instance:

[1, 2, 3].printElement(ifNot: { $0 > 2 })
[1, 2, 3].printElement { $0 > 2 }

In such cases, ensure that all critical information required at the use site is available in the base name:

[1, 2, 3].printElementIfNot { $0 > 2 }

This guideline would justify deprecating old names such as drop(while:). On the other hand, I do not think first(where:) rises to the point of being actively misleading when the preposition is omitted. [Indeed, a rule such as the above would mean that users can be rather assured that the standard library is not also vending a sneaky API such as first(whereNot:).]

As for typechecking issues with the proposed count(where:), I don't think we should be conflating the typechecker issue with the readability issue. Humans should have no problems parsing the use site, and so it's harder to justify making demands of users in that case in the form of API renaming. We have had myriad issues with typechecking involving operators that rise to the level of active harm, and the count(where:) example you give would be adequately addressed when the underlying issue is tackled.

16 Likes

I think the points in favor of requiring labels for trailing closures are excellent and spot-on: (optional) parameter arguments are one of the foundational features of the Swift language, that make it particularly pleasant to read, and we should require them for trailing closures too. The one exception I'd make is: if the argument was _:, I'd allow to omit it completely, like the language already does for regular parameters.

array.first { condition } should be written array.first where: { condition }, it's much better. To maintain source compatibility, I'd add a deprecated func first(_ where: (Element) throws -> Bool) rethrows -> Element? to the standard library.

8 Likes

after reading this modified pitch, I just feels that the original first pitch maybe better than re-reviews.
Func() { lbl: {} lbl:{}...} it’s a little bit hard to write but much easier to read and compiler friendly style form.

The harm here is indeed that there's two ways to call it: one with the label and one without. Renaming first(where:) to firstWhere(_:) does solves the problem, but in a way that goes against the naming guidelines. It also forces every call site to be updated, regardless of whether using the trailing closure syntax of not.

I'd rather see a way to make the label mandatory, perhaps like this:

func first(@explicit where: (Element) -> Bool)

One effect this could have is to forbid calls using the trailing closure syntax. We could also add the ability to add a label in the trailing closure syntax in a similar way to labels in the amended SE-0279 currently being reviewed. A warning + a fixit would nudge you in the right direction.

Half the time I'm using first(where:), it is within an if condition where I can't use the trailing closure syntax anyway. Changing the name means I have to rename it everywhere, whereas disallowing unlabelled calls only requires changes to where the trailing closure syntax is actually used.

Note: I'm suggesting @explicit here because it seems most backward compatible. Personally I think it'd fit better in the language to have explicit be the default and @implicit for when the label is allowed to be omitted.

1 Like

Could folks elaborate on why they think renaming drop(while:) to dropWhile(_:) would violate (especially the spirit of) the API naming guidelines?

It seems like a natural extension to this rule (which already includes a well-founded exception):

Adding a new language feature like @explicit strikes me as overkill compared to moving the argument label into the base name.

I don't view it as an extension, but a reversal: the rule says that when the first argument forms part of a prepositional phrase, the preposition goes in the label except for symmetry when the preposition applies across multiple labels. Carving out another exception for trailing closures wouldn't be driven by API naming consistency or symmetry, but solely to workaround a limitation of the language.

I'd really like to move on from focusing so much on the API naming of a specific example drop(while:) because (as @xwu said in this post above) it forces the scope of the discussion to be so narrow that it's hard to discuss this problem meaningfully. drop(while:) isn't the problem; it's a symptom of a larger problem, and that larger problem has other consequences that I and other posters have stated above but which have mostly been glossed over in replies:

  • trailing closure syntax can't be used in a conditional statement without awkward workarounds
  • an API cannot be designed even with SE-0279 that reads well if it uses multiple trailing closures and those closures have "equal weight"; e.g., Binding(...) get: {} set: {}

If these are problems that the core team doesn't want to solve right now, then that's totally fine; but it would be helpful to know what's on the table and what's not because that affects whether I support the existing proposal as written (it's an improvement over the current state of things) or if I try to advocate to something that goes further.

To be fair, the only reason a new language feature would have to be considered here is because the original design of trailing closures made the choice to unconditionally elide the label, which is inconsistent with how other argument labels are used. That's another reason why I'm asking for a holistic look at this problem (which could possibly include a new Swift 6 language mode that allows some source breaks? is that on the table?) and not just focusing on a couple cherry-picked APIs.

10 Likes

I like this idea. However, for the sake of backward compatibility, would it be better to have @implicit to be the default?

If "implicit" is the default, we need no @implicit attribute because it'd have no effect. That's why I'm saying @explicit is the most backward compatible option: it implies "implicit" is the default.

Thanks for the clarification. I misunderstood earlier.

I'll agree with you in part: @explicit is a patchy solution. If it was just me, I'd simply ban the trailing closure syntax when the last argument has a label. This actually makes the language simpler:

  • API designers don't have to ask themselves if the function is readable both when the parameter label is present and when it is omitted. If they say it has a label, it always has a label.
  • Call sites cannot create ambiguities by omitting labels. Function calls are spelled as the author of the function intended, always with the same label everywhere.

Forcing this everywhere would be a major headache however**. So @explicit is just an opt-in system to this world with less ambiguities. It should be used sparingly when the ambiguity actually causes problems.

Unlike a rename, there is also no ABI impact in adding the attribute to a function. It does not break source compatibility either if the effect is only a warning + a fixit at the call site. So whenever you discover a similar problem with other functions, it's an easy fix.

And if SE-0279 is accepted, this problem is bound to come up more often. Better be prepared.

** The problem if we were to make labels explicit as the default is that it'd create so many warnings to fix that nobody would want to put up with them. So not really source breaking because everything would still compile fine, but it'd be quite annoying to deal with. It also changes what people are used to.

7 Likes

If you think the current SE-0279 proposal can work on its own but you’d prefer it to go further in certain ways, that’s very interesting feedback. Likewise if you think it’s not a good idea unless it does go further. It’s not an up/down vote; you don’t have to try to distill your conclusions down to a single bit.

3 Likes

This is a topic I'm biased against. I always believed (and still do) that trailing closures should've been only an opt-in feature, for making some specific functions (e.g. control flow-like ones) read more naturally. Instead it's present everywhere, and since it is people tend to use it, and then the issues occurring due to it arise.
I believe that function labels are meant to have semantic meaning, and I see allowing for that to be removed as counter-productive.

The main question for me is: why do we have trailing closures at all? What reads different between foo { ... } and foo({ ... }) (apart from the lack of ())? Imho this distinction should have some semantic difference, but the fact that it can only be known by the designer of the function while the trailing closure is allowed everywhere kills this option.

Trailing closure syntax can also be interpreted as one of the various "simplification" sugars available (like omitting the signature), but imho it's not the same: it actively moves the closure from inside to outside the "domain" of the function call. This kind of movement will necessarily cause a difference in perception to the reader, even though by now we're trained to read it as equivalent due to it not being a selective thing.

Personally I believe it could still be made opt-in, with something like @trailing in the parameter, as long as a compiler option for ignoring it gets made (e.g. always_assume_trailing). In this case I wouldn't have strong thoughts about having to write or not the associated label anyway, as it would be in control of the designer of the function. Though I find the option of having label-prefixed trailing closures interesting (in the context where I find trailing closures appropriate).

But said all that, I understand that people seem to like trailing closures in general, and I mostly gave up on my interpretation of it :sweat_smile: though I always liked putting semantic information in the argument labels and this renaming seems to go against it :(

10 Likes

This thread is not the place to provide feedback on SE-0279. I appreciate the point that it’s hard to respond to Ben without interrogating the language design at all, but still, please try to recognize when your post is primarily a response to SE-0279 and say those points in the review thread. You can then build on that in here (by linking to it) to make a more direct point about renaming APIs.

Thanks for the answer, but I don't really agree: SE-0279 is primarily for multiple trailing closures, even though of course it has implications on the case of a single trailing closure. But I posted in this thread completely ignoring the multiple trailing closures case, and only considering the single one.
I realise that the proposal is for renaming of functions, so the trailing closure feature itself might seem OT, but since it's really "renaming because of trailing closures" I believe that a longer form of "I wouldn't want a rename because I think it's trailing closure's that it's at fault" is correctly posted here.

If you still think that it is fundamentally in the wrong place I can move it :+1:

(Edit: I appreciate that this whole discussion is getting hard to moderate)

3 Likes

My post wasn't meant exclusively as a response to you, sorry; it was something I'd meant to post as follow-up to an earlier post.

2 Likes

I find this mostly to be a straw man argument.

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

IMO A is better than B and A is better than C. Stick with A. You want to force me to use C because you say it's better than B. Just don't prevent me from using A.

C looks like Objective-C, which we all want to leave in our rear-view. If some people want to use B rather than A that's fine with me. Just not in my projects. This is a job for the linter.

I don't see this as a matter of safety or clarity. There are plenty of ways people can shoot themselves in the foot.

I personally use this design style in plenty of my own method names and I like it.

func myObject(at indexPath: IndexPath) -> MyObject?
func name(for offset: Int) -> String

and similar. If you change the API Guidelines then this good style will be replaced by this new (IMO Bad) style in general.

-1

19 Likes

I definitely understand the rationale for wanting to trailing closure read more naturally, but I don't support this proposal as it stands.

For one, I don't like the way it changes these functions when not using trailing closures.

arr.firstWhere({ $0 > 5 })

As has been mentioned previously, the Swift API guidelines strongly recommend moving the prepositional part ('where') into the first argument label. Not doing so makes this read awkwardly alongside more idiomatic Swift code. In making one use case better (trailing closures) at the expense of other use cases, I think the benefits of this change are largely negated.

Outside of simple style nits, the methods you're proposing to change are used quite heavily in most Swift codebases, so any changes to them have wide reaching effects. Is the effort involved in renaming all of these methods in every single Swift codebase going forward worth a slight increase in readability in certain use cases?


I think one under-discussed part of the issue here also has to do with the way Xcode aggressively inserts trailing closures wherever possible. I frequently run into cases where Xcode autocompletes some code to use a trailing closure, and then I have to manually revert that change for the sake of readability. For example if I'm in the middle of writing this Rx code:

If I then tab over to the argument placeholder for onError and press return, it expands to this:

Screen Shot 2020-04-21 at 2.56.02 pm

In this case Xcode has removed the argument label and made my code less readable in doing so.

I think a more pragmatic solution here might be to introduce annotations which can provide hints to text editors about when to use trailing closures, and where a label provides valuable context and shouldn't be removed.

7 Likes

-1

It makes the call site uglier when you aren’t using trailing closures. Better would be to adopt a trailing closure syntax that optionally keeps the argument label intact, which also allows for it to read the same way whether or not trailing closures are used but keeps the function name looking nice in the non-trailing closure scenario. (And has the bonus of not renaming functions and breaking source compatibility.)

6 Likes