I understand the idealistic reasons for this, but it is a small edge case. The compiler should give a good error as written since it would need a newline before bar for it to be two functions. Then there is a 99.9+% chance it would still give a good error since it is equivalent to a hash table collision. I think this is within the error rate of other errors being good errors.
This bothers me too. In part because both new and old syntax is ambiguous in too many contexts. I would like to see change in part because of this. I'm pragmatic enough to leave a few ambiguous cases that would never be encountered following style guidelines, but there are too many right now requiring some horrible explicit cases in the guidelines. Even if not in this proposal, I'd like to at least see a plan to improve ambiguous cases for both new and existing trailing closures.
1 Like
The projects I'm on are using closer to an Allman syntax style than a K&R variant, so we would have each label start on its own line. All comments are always on their own lines before the related code so that also matches our style if describing a closure.
1 Like
This review is probably going to seem very negative. Sorry for that, but I think the proposal is fundamentally flawed and it wouldn't serve the language to pull punches here. -1 on accepting it.
Problems With The Problem
First of all, I find the description of the problem in the proposal to be incoherent. First, it identifies “the” problem with trailing closure syntax as that it only applies to the last closure (i.e. we don't get to omit enough argument labels), and then it goes on to give an analysis where the actual problem is that a label is omitted that was needed for clarity. Making it possible to sugar closures in more argument positions can't help with that.
Second, the description of the problem is uncompelling:
// Without trailing closure: UIView.animate(withDuration: 0.3, animations: { self.view.alpha = 0 }, completion: { _ in self.view.removeFromSuperview() }) // With trailing closure UIView.animate(withDuration: 0.3, animations: { self.view.alpha = 0 }) { _ in self.view.removeFromSuperview() }In this case, the trailing closure syntax is harder to read : the role of the trailing closure is unclear, the first closure remains nested, and something about the asymmetry is unsettling.
Handling the claims in reverse order,
- “Something about the asymmetry is unsettling” — just the sort of subjective assertion that makes me deeply suspicious that changes are being driven by criteria that leave us with no identifiable guiding principles. If we start making adjustments to the language whenever “something is unsettling,” different examples and peoples' experience will dictate contradictory choices and we'll be adjusting forever.
- “The first closure remains nested” — so what? That's not a problem in and of itself.
- “The role of the trailing closure is unclear” — yes. That's a problem to be solved.
Overall, the readability of the examples is harmed by formatting that hides the structure of the code (and a needless with in the argument label—but never mind that), which makes it hard to evaluate the real issues. Once reformatted:
UIView.animate(
withDuration: 0.3,
animations: { self.view.alpha = 0 }
) { _ in self.view.removeFromSuperview() }
I can see the problem quite clearly; it's about the role of the final closure. Looking back at the proposal's example of successful trailing closure use, we find:
UIView.animate(withDuration: 0.3) {
self.view.alpha = 0
}
What makes it OK that the trailing closure is unlabeled in this case? For me, it's fine because it represents the change that's being animated, in other words, it has a primary role in the call that can be deduced from context—and I see that the proposal identifies “primariness” as the reason trailing closure syntax usually works out. The issue with the “problem” example is that we've put a closure with a secondary (and inconsistent!) role in the primary position at the end of the call. If the example with one closure is good, I suggest this should also be considered good:
UIView.animate(
withDuration: 0.3,
completion: { _ in self.view.removeFromSuperview() }
) {
self.view.alpha = 0
}
And if you're going to argue that the role of the trailing closure isn't clear enough in this example, I'd ask that you explain why it's clear enough in the example with a single trailing closure, and not here.
To sum up, the one example cited as a problem suffers from poor Swift API design, because it wasn't designed for Swift—it was imported from Objective-C. Before we complicate the language with new wrinkles on calling syntax we should have—not just one, not just a few, but many— examples where there are problems that couldn't have been solved with better API design choices.
Problems With the Solution
I have nothing in principle against extending the expressiveness of the language by making it possible to label trailing closures, nor with features that reduce nesting. This proposal, however, moves the de-facto position of the primary closure from the end to the beginning of the parameter list. Existing multiple-closure APIs that were sensibly designed to take advantage of the status quo now have an even terser syntax that puts a label on the trailing closure. So, for example, if the closure order in UIView.animate had been well-chosen, users could write their call as in my last example, or they could write:
UIView.animate(withDuration: 0.3) { _ in
self.view.removeFromSuperview()
} animations: { self.view.alpha = 0 }
It seems to me that the above is no better than the original when it comes to clarity, at least if you buy the idea that the problem is that the secondary closure needs a label.
By adding more options for how to call a given function, we are taking control away from API authors over how their APIs look in use, and transferring it toward users. That's not necessarily wrong; API authors can't control how users will format their code or name their variables, both of which can have a huge impact on how well an API's use site reads; clients have to participate in making use sites read well.
If we're going to take this tack, though, let's start with a simple change that addresses the only real problem raised here: sometimes you need a label for the trailing closure in order to make its role clear, and to use it you have to give up on the de-nesting benefits of trailing closure syntax. Just allow the existing trailing closure syntax to be optionally labelled for clarity.
Problem of Focus
This proposal seems to throw a relatively large change at a very small, syntactic problem, as far as I can tell. IMO there are enough real fundamental problems to solve in Swift (ownership, CoW slice, variadic generics, concurrency, … off the top of my head) that if we're going to “do syntax sugar,” we should keep the changes small and conservative, and we should be rigorous with ourselves about the problems actually being solved, so we can put attention on the fundamentals. This proposal seems to do neither.
43 Likes
I don't think you've gone quite far enough in explaining why the chosen example is poor. It's not a bad API design. It's an API inherited from Objective-C, and it fits in very well with Objective-C syntax. When written out, you can read the closures as naturally following each other in time, as we would expect with an animation which is followed by a completion (handler).
Even in Swift, if one ignores the trailing-closure option, one could write:
UIView.animate(withDuration: 1.0,
animations: { ... },
completion: { ... })
(The fact that Xcode won't format it nicely in most cases is an Xcode problem, not an API or syntax problem.)
I don't find the fact that one can find examples which don't work nicely with trailing-closure syntax a compelling argument in-and-of-itself. The answer isn't to add yet-another-option which will inevitably have its own list of examples which work poorly. The answer is to use the syntax that is already available.
14 Likes
I agree with your concerns expressed in the "Problems with the Solution" section. I think that optionally labelling the first closure should be more thoroughly considered and as some other members of this community suggested, I advocate the deprecation and eventual removal of implicitly dropping the label of the first closure (in future versions of the language, of course). I believe that, because not all APIs have a first closure whose label is implied by the function/method name. There has been recent discussion about Standard Library methods have closures whose purpose is unclear due to the lack of a label (example: Renaming Trailing-closure Functions in the Standard Library).
I, also, understand your concerns regarding the focus of the community on what I assume you'd call unimportant language features, but I firmly disagree.
What I don't support is the fact that in the "Problems With The Problem" section you review the writing of the proposal, not the ideas shared in that proposal. The purpose of this discussion is to provide constructive criticism and express our opinions about the proposed change, not judge members's writing abilities. Providing constructive feedback on the writing is, of course, welcome. Even for that though, I consider this thread unrelated.
3 Likes
You're right; it's a poor API design for Swift as it is currently defined. I'll amend my post to that effect, thanks.
Lest we forget that things like this TextField(_:value:formatter:onEditingChanged:onCommit:) is tricky to design with current syntax.
3 Likes
Please support your claim with specifics. My objections in that section are, as far as I can tell, absolutely with the ideas expressed in the proposal's problem description: they fail to convince me that the proposal's authors understand what problem they're trying to solve, or that any problem they are solving is real. While the ability to make a convincing argument is always going to be tied, at some level, to writing skill, I wouldn't be criticizing coherent, compelling ideas expressed, for example, with poor grammar.
2 Likes
How should Xcode (or any formatter) know when it's preferable to use the trailing closure syntax and where it should be avoided in order to preserve the label? At best it'll know about a limited list of functions hard coded into the tool, or it'll use a heuristic that'll guess wrong half the time.
Whereas if there was a way for function declarations to tell when the label is important, code formatters would have a useful signal to work with. Arguably we already have that with _ labels, but the trailing closure syntax treats both _ and non-_ labels equally so it's not very useful in that regard.
2 Likes
Simple – if the function takes more than one trailing closure, don't use the trailing closure syntax.
It might make sense to invent a more advanced logic, but that's an issue with Xcode anyway, not with Swift
3 Likes
- What is your evaluation of the proposal?
+0.25
If the first closure also had a label, +1
- Is the problem being addressed significant enough to warrant a change to Swift?
Yes. Trailing closures have become an important and popular language feature and making them work for multiple closures will make more APIs feel Swifty.
- Does this proposal fit well with the feel and direction of Swift?
For the most part yes. But to me the special case of having one unlabeled trailing closure feels like a big mistake. It is reminiscent of the way argument labels worked prior to Swift 3, where the first argument to a function automatically had its label suppressed. While it was initially appealing for ObjC interop, it ended up not feeling right for Swift and in Swift 3 the special case for the first argument was removed and now we use argument labels everywhere (unless the author of a function decides that a particular argument works without one).
I think the same thing is bound to happen here (just look at the other feedback in this thread!) so we should probably learn from the past and avoid that mistake entirely. All trailing closures should be labeled from the start. For a function with a single trailing closure we should allow the label to be elided for compatibility with current source, but we should consider dropping that in the future. Perhaps we should allow an individual function to specify that the label may be elided (either by setting it to _ or with some @attribute) but that can be decided later after we have more experience using this syntax.
- If you have used other languages or libraries with a similar feature, how do you feel that this proposal compares to those?
N/A
- How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
I've been following the threads pretty closely and have played around with some use cases.
4 Likes
I’ve been staying out of this review and waiting for things to settle, but that doesn’t appear to be happening so I’ll offer my perspective now.
With regard to existing APIs, I find the proposal’s motivation quite lacking. The examples it gives can be written perfectly clearly with the existing standard syntax, not using trailing closures at all.
Trailing closures are a tool, and it is up to the programmer to decide when that tool is helpful for the job at hand. Sometimes it is better to write out the function call long-hand, with all labels included and all arguments inside the parentheses. That is a perfectly valid thing to do.
A side-by-side comparison of examples from the proposal using standard (non-trailing) syntax, and using the proposed syntax, shows almost no change. The proposed syntax just moves the closing parenthesis and drops a few commas. It is not obviously a win at all, especially not a large one.
• • •
Others in this thread, including @Lantua and @dabrahams, have mentioned that many existing APIs have a “primary” closure. For those functions, it would often make sense to use trailing closure syntax for the primary closure.
However, some of them place the primary closure in a position where it currently cannot be used as a trailing closure. That is a specific and identifiable problem, which could be solved with a @trailable attribute.
• • •
The proposal has not provided compelling examples of new APIs that would benefit from the proposed syntax. If the authors have ideas in mind where they would like to use this proposed feature, that should be prominently placed in the motivation section since it would in fact be their motivation.
The absence of any such examples for new APIs with this feature, is a substantial mark against the proposal. If it does not enable new code to be written in a demonstrably-superior way, then it should be flatly rejected.
Instead, the motivation section spends all its time discussing legacy Obective-C APIs that were not originally designed for Swift at all. In my view it would be far more productive to consider ways in which those APIs could be improved or replaced to become more Swifty.
Moreover, as mentioned above, any such hypothetical new API that purportedly benefits from this proposal, could just as easily be called in the standard way, with all arguments inside the parentheses. With such a straightforward alternative already existing, the bar is quite high for introducing additional complexity, and the proposal at hand effectively just allows a few pieces of punctuation to be shuffled around.
18 Likes
There are three examples in the proposal. UIView.animate, whose signature would not change, and Combine.sink and SwiftUI.Section, whose signatures would change.
1 Like
Objections of this kind, seem to me, to fly in the face of the popularity of trailing closure syntax usage in Swift today. Many style guides, including Google's Swift Style Guide go so far as to mandate it:
If a function has a single closure argument and it is the final argument, then it is always called using trailing closure syntax...
From the proposal:
Trailing closure syntax has proven to be very popular, and it's not hard to guess why. Especially when an API is crafted with trailing closure syntax in mind, the call site is easier to read: it is more concise and less nested, without loss of clarity.
The proposed solution to extend "trailing closure syntax to allow additional labeled closures to follow the initial unlabeled closure" aims to extend "the concision and denesting of trailing closure syntax to function calls with multiple closures arguments".
That is, any new API that benefits from this proposal would be more concise and less nested than if it were "called in the standard way, with all arguments inside the parenthesis."
1 Like
+1. This is a natural extension to the trailing closure mechanism. I think the concern that one of the closures remains unnamed might be valid in the context of some existing APIs, but it will enable and is likely to encourage the design of APIs that are clearer at the point of use going forward. Certainly, SwiftUI and Combine, as mentioned in the examples, would benefit from this extension already. Also, in cases where clarity is an issue, it seems that not using the trailing closure syntax might be appropriate and remains an option.
Yes, absolutely. I'm not sure the animation example illustrates this well, and I think @dabrahams brought up in his comments - the absence of the label on the first closure does not necessarily make its intent clearer. However, it enables API authors to create tailored APIs in which the first closure's intent would be evident from the function name itself. There are already examples of that like Combine's sink.
Yes, I think this is a natural extension of the trailing closure syntax. As far as I can tell there are only 3 choices for extending current trailing closure syntax that potentially fit well with the current feel of the language:
- keep all trailing closures named. This seems like not much of a win since it essentially just moves the closing parenthesis before the first trailing closure.
- keep all but the last closure named. This, in my opinion, suffers from a significant clarity issue at the call site since the last closure will follow the one before. With this approach, the API author will lose the ability to name their function in a way that allows the reader to naturally deduce the meaning of the last closure.
- the proposed approach. This, in my opinion, is the best option of the 3, since it both extrapolates the existing syntax and solves the clarity problem for subsequent closures.
My only real concern here is around the ergonomics of this, especially with regards to code completion, but it sounds like @rintaro already addressed it in his comments.
I studied the proposal itself and I believe I understand it well. I gave the discussion a quick read.
1 Like
I want to add that there are also those that don't have primary closures as well.
2 Likes
I think that this is the best that you can do with the language today. It's the design that Combine.sink eventually settled on:
ipAddressPublisher
.sink { identity in
self.hostnames.insert(identity.hostname!)
}
ipAddressPublisher
.sink(receiveCompletion: { completion in
// handle error
}) { identity in
self.hostnames.insert(identity.hostname!)
}
However, that's just because the lack of a syntax for multiple trailing closures has left API authors in a bind. When it comes to closures, we've been forced to violate an important API guideline:
- Prefer to locate parameters with defaults toward the end of the parameter list. Parameters without defaults are usually more essential to the semantics of a method, and provide a stable initial pattern of use where methods are invoked.
The guideline says methods should have a "a stable initial pattern," but because trailing closures were liable to be unlabeled it was important for clarity to prioritize their stability as the last argument.
This proposal allows us to unify trailing closure syntax with this API guideline
UIView.animate(withDuration: 0.3) {
self.view.alpha = 0
} completion: { _ in
self.view.removeFromSuperview()
}
I disagree that UIView.animate is a poor Swift API design. Rather, because it was imported from Objective-C, it respected a more fundamental API guideline that is not limited to Swift—prefer to locate parameters with defaults toward the end—without contorting itself to trailing closure syntax, before it had been properly extended to multiple arguments.
2 Likes
I think you're reading far too much into guidelines like this and are mistaking a desire for consistency for some argument for the inherent superiority of the existing trailing closure syntax. It's generally good, yes, but it leads to some tricky naming problems, as we've been discussing. Additionally, the simple fact that Xcode defaults to trailing closures with the special syntax (when it works, its inconsistency is so annoying) without the ability for users to disable it naturally leads to a very high level of use among the community.
I also think you're making a logical leap in applying the popularity of trailing closure syntax to the wider issue of dealing with many closure parameters. As a few reviewers have mentioned, the actual problem being solved by this proposal hasn't clearly been stated as more than just a desire to apply some form of trailing closure syntax to multiple closures. I think @Nevin's point was that the problem of multiple closure parameters, without the desire for a new trailing syntax, is already solved, and this new syntax doesn't unlock any sort of new APIs.
11 Likes
I’m not too worried about the order. It is easy to overload and provide a different order. The original order could be deprecated and eventually removed. I’d rather see that we are doing the right thing for the future so if we change the order of everything we don’t need to do it again later.