Yeah, agree with that. I can see not wanting the behavior for protocol conformances, that would seem a reasonable exception. Then it'd only be subclasses overriding a function that might need decoration, for everyone else it'd likely be an actual issue.
Improving diagnostics does not require a pitch, but adding some decoration does.
There is no need for a decoration, because we already have a way of spelling an ignored argument, and it's already used for this purpose, so any new diagnostics must account for this and not warn:
In func f(a: Int, b _: Int), b is intentionally ignored, since there is no way to refer to it. [Edit: I see @cukr has already pointed this out.]
Moreover, it's possible to silence a warning for any named argument by writing, for example, _ = a in the body of the function.
Argument labels are distinct from internal parameter names. @cukr is referring to the latter.
No decoration is required either for protocol conformance or subclassing: it is permitted to name your internal parameter name anything you want in either circumstance, and best practice even now should be that unused arguments have their parameters internally named _.
10 Likes
Thanks @xwu for explaining - then it feels this could be a clean diagnostic improvement. Not sure what the proper way to try to get that added would be though.
You can follow the instructions in the main Swift repository for getting started with making edits to the compiler, and then create your first pull request when you're ready! Seems it should be straightforward to build on what's already there for unused variable checking, but I haven't looked into it myself :)
I agree that the func f(a _: Int) is the natural spelling to silence such a warning, but IMO such a dramatic shift in diagnostic behavior probably deserves to go though evolution review. I suspect that there is lots of code that would suddenly hit this warning, which would represent a non-trivial disruption to existing code bases. We should be confident that the tradeoff is justified!
Of course, that's just my opinion—the Core Team is the final arbiter of what can be considered a bug fix vs. what needs to go through evolution review.
4 Likes
I put up an implementation for this a long time ago - [Diagnostics] Emit a diagnostic if a function parameter is unused in the body by theblixguy · Pull Request #28839 · apple/swift · GitHub (although it was behind a flag and not enabled by default). I agree with what @Jumhyn says, I think this change would benefit from going through evolution.
4 Likes
@suyashsrijan that's excellent, thank you. Do you want me to write a pitch for it and try to get it through evolution or do you want to yourself?
Free free to pitch it! The implementation is already there (bit old so probably need to fix conflicts, tidy it up and perhaps adjust how it works depending on what rules are being proposed) so if the pitch gathers support and momentum you can probably leverage that existing implementation (I would be happy to help if needed).
1 Like
Here is a concern I have. It is that when you add documentation to a function you document the parameter name not the label and you can’t document _ at the moment. I have unused variables like this when I am working with generics and I want a type not an instance. I personally prefer referring to the generic type rather than the parameter so all the type references are spelled the same.
Other than that it would certainly need to not want it for protocol conformance.
(Fixed typos thanks to autocorrect)
For documentation purposes or other reasons?
For other reasons. Sometimes it makes sense to have an argument on a protocol function but your implementation might not need it.
Are there situations that aren't covered by giving the argument an external label, while discarding the internal name? E.g.,
protocol Foo {
func someRequirement(importantArg: Int)
}
struct S: Foo {
func someRequirement(importantArg _: Int) {
// This space intentionally left blank -Jumhyn
}
}
1 Like
The only thing I see is if you customize the documentation rather than inheriting it from the protocols definition. Then you cans document the _. But that is the only issue I see.
Also overall I like the idea of the pitch I just want to make sure we get it right if it is pushed forward!
I'm really surprised we didn't add this warning. 
When I was working on a Swift side-project a few weeks back, I hit two places where this would've helped me within the span of 1 week:
- I updated a protocol method to take more arguments, but forgot to use all the arguments in one of the implementations.
- I had a hard-coded value in a method. → I added an accidentally unused parameter with a default value matching the hard-coded value, and forgot to delete the hard-coded value. → I forgot to test the non-default case. → Hit the bug a few days later while investigating another bug.
Spelling-wise, if we used a spelling _parameterName instead of _, that gives the reader a clue as to what the thing is supposed to be. This is what Rust does. (_ is also permitted though.)
5 Likes
Any warning that merely requires rote removal of autocomplete code to silence just seems like overhead. This warning is awful in Obj-C, it would be awful in Swift (by default). I do think it's a good candidate for Swift's first optional warning though.
2 Likes
You can do this instead:
/// Some description.
///
/// - Parameters:
/// - importantArg: Some description.
func someRequirement(importantArg: Int) {
_ = importantArg
}
I did however notice while tinkering that the importantArg _: Int variant breaks the parameter section of any inherited documentation (at least in Xcode). That should probably be fixed.
5 Likes
+1 yes please. Underscored internal argument name to silence warning. Thanks!
Right, this is the one issue with protocol conformances and subclasses that would cause problems here.
But: it's already a problem when a user (legitimately) wants to implement a protocol requirement but prefers alternative internal parameter names than those given by the protocol. For instance, in the Swift standard library, several of the SIMD implementations of binary operators required by protocols were changed to name the operands a and b, instead of lhs and rhs, to standardize with other binary operator implementations that aren't protocol requirements. Any inherited documentation inherited from a protocol is now broken.
This is a tooling bug, and it's the tail wagging the dog to try to "solve" this by contorting the design of a language feature.
This wouldn't address the problem regarding protocol conformances by itself, since _parameterName is still different from parameterName, just as _ is. We need to teach the tooling to understand that implementations can inherit protocol documentation and match up the arguments by position to behave correctly to support _, and if it does that, then the _parameterName workaround wouldn't be necessary.
I see your point about telling the user something about what the _ is, but I think that can be accomplished in the tooling itself by, for example, numbering unnamed parameters in the generated documentation rather than privileging "magic" underscores in this one place. We already number unnamed parameters in closures, so the same syntax in documentation could be intuitive here.
Swift, which started off eschewing the punctuation-heavy syntax of Objective-C as a major attraction to users, has already acquired or expanded the use of a bunch of sigils like \ and $ and @ that prefix names. Used judiciously, sigils can be powerful, but I don't think this problem rises to one that could justify a solution that endows yet another sigil with additional special meaning instead of relying on existing syntax.
2 Likes
This empiric evidence is so key to motivating this: in the draft implementation, the core team indicated that adding diagnostics for stylistic purposes isn't desirable: it is extremely useful to show (as the bug that motivated this thread and your examples do) that there are actual, real-world bugs that are mitigated by this diagnostic. That is very strong evidence to favor making a change.
4 Likes
I did not consider it to be a problem with the pitch. I could find no problems. The bug is just something I stumbled on while toying with it, and which should presumably be fixed regardless of the pitch.
1 Like