I do see this as equally applying to both updating the guideline (& affected apis) and update the trailing closure syntax. Perhaps other criteria is also used (like impact on compatibility).
Why wouldn't you be able to do this (let's use @mandatoryLabel as a strawman syntax just for illustration):
// the new function
func drop(@mandatoryLabel while predicate: (Element) throws -> Bool) rethrows -> SubSequence {
drop(_while: predicate)
}
// the original function, renamed
func drop(_while predicate: (Element) throws -> Bool) rethrows -> SubSequence {
}
From the source POV, any call sites with the explicit label (s.drop(while: fn)) would go to the new function, while labelless ones would go to the one renamed _while (s.drop { ... }). Is it an ABI compatibility issue because the original function is being renamed and a new one is being swapped into its place? Could that be resolved some other way, since the new function would just be a transparent forward to the old one?
I sympathize that suggestions like the one above, even if possible, would require more engineering than just renaming and deprecating some functions. But this feels completely in line with Swift's design goals and would be valuable by making the language more consistent and remove existing ambiguities. Swift deemed argument labels to be valuable enough as part of the name of an API to include them in the language design, but trailing closure call sites introduce parsing ambiguities and are inconsistent with how those labels are elided at call sites compared to non-trailing closure labels (by using the underscore at the declaration site); in other words, the language design unconditionally takes this choice away from the API designer.
Overall, it would be good to have a clearer understanding of what's on or off the table here, especially as we approach Swift 6. Is it worth trying to push on bigger efforts like this, or is it more likely that the only realistic options will be "do nothing" or "rename the functions as pitched above"? If we only have those two options, then I suppose I'd be supportive of renaming, because the regression is mostly aesthetic but it does improve clarity at point of use. But I think it would be missing a great opportunity to patch a lot of little holes in the language and improve Swift API design in the long-term.
21 Likes
Please keep this discussion to the question of trailing closure labels only. Re-evaluating naming more widely is not something that is going to be considered.
1 Like
i am against this change. the number of methods it applies to (sort, first, count, drop, all-satisfy, dictionary filter) is small, and the naming has been this way for so long i’ve kind of internalized that there is an implicit “where” or “by” in calls like:
sort
{
$0.key < $1.key
}
so i don’t find the spelling without the preposition that confusing at all. plus, it reads very weird to have a preposition in the function name rather than as an argument label, especially if the return type of the function is anything other than Bool or Void?
dropWhile(_:)
firstWhere(_:)
sortBy(_:)
the reasoning given in this pitch, that the readability is “severely impaired” without the argument label would also apply to methods like filter and allSatisfy but it’s not really clear how adding a preposition to the function name would make it any less awkward or confusing
allSatisfyWhere(_:)
filterWhen(_:)
making code read like english is always a nice to have but i don’t think it comes anywhere near the bar you need to justify source breakage.
6 Likes
It's even smaller. sort, filter, and others, need no amendment.
That people have figured out where the bear trap is and now walk around it is not a good reason not to fill it in.
But it doesn't. filter(isEven) and filter { $0%2==0 } read just fine.* The extra word isn't necessary. But drop is clearly misnamed.
To reiterate: this is not about making things read like English. It is about function names that fail to serve the most important goal in the API guidelines, that of clarity at the point of use. This new guidance is about helping achieve that goal.
* edit: heh, well, except for the other problem with filter, that it's not clear if it's filtering in or out... but again, not what we're discussing here and not on the table to change :)
3 Likes
If you're not going to move all argument labels out, to solve what I showed, then you shouldn't do it for any. Trailing closures are not special. Just go like this:
let array = [1, 2, 3]
array.first(where:) { $0 > 1 }
array.drop(while:) { $0 > 1 }
let max = [1, 2, 3].max // 😖
I know that ideally the language shouldn’t let us be ambiguous but really I don’t think that just letting people write the closure without trailing is such a big deal.
This proposal seems to harm one use case for the other.
2 Likes
I think I would naturally put parenthesis around these for clarity anyway. I think a better linter or code completer would solve this satisfactorily for me. As long as the editor helped guide newcomers I think it is acceptable.
// Pick this if closure is labeled
if array.first(where: {$0 < minValue}) > 0 { ... }
// Pick this if closure is unlabeled
if (array.firstWhere {$0 < minValue}) > 0 { ... }
// Pick this if multi-trailing closure (Forgive lazy example)
if (array.firstWhere {$0 < minValue} completionHandler: { ... }) > 0 { ... }
It's fine for individual developers to choose to put parens there if they wish, but the language arbitrarily requiring a different syntax depending on whether or not the function call is in a conditional statement makes things less clear, especially for newcomers.
Linters also can't solve this problem; a syntax-based linter (e.g., one implemented on top of a parsed syntax tree representation, like swift-format on top of SwiftSyntax) needs the code to parse correctly in the first place in order to diagnose the problem and offer an alternative. But the syntax tree returned by parsing the following code will not represent the user's intent:
if array.first { $0 < minValue } > 0 { something() }
After all, if the parser could do so, it wouldn't be ambiguous. So it would be nearly impossible to auto-correct this or suggest to the user that they should wrap the condition in parentheses.
1 Like
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.