Add warnings for unused function arguments by default

Evolution Discussion
1

I think it would make sense to add a new warning to the Swift language for unused function arguments.

This is analogous to

And created this (trivial, but an example) bug:
https://github.com/apple/swift-nio/pull/1797#issuecomment-816520124

If people think it makes sense I could write up a formal pitch.

I am thinking along the lines of adding the warning by default and then have some decoration (@unused?) for function arguments which are unused.

This can be on purposes in e.g. subclasses or protocol implementers that don't need to consume all arguments, but I would argue a majority of functions do want to consume all arguments and it would make sense to warn if you aren't doing it.

Code that specifically don't need an argument can just decorate it accordingly.

This would eliminate one class of bugs reasonably cheaply.

8 Likes
2

Warning like that would be great!
I think the better solution for silencing the warning would be to use underscore, because it's already used for discarding values in swift

func test(a: Int) {} // warning
func test(a b: Int) {} // warning

func test(_: Int) {} // no warning
func test(a _: Int) {} // no warning

Similar solution is used by rust today

fn test1(arg: i32) {} // warning
fn test2(_: i32) {} // no warning
fn test3(_arg: i32) {} // no warning
8 Likes
3

It could be a bit confusing as its already used for omitting function label names?

Omitting Argument Labels

If you don’t want an argument label for a parameter, write an underscore ( _ ) instead of an explicit argument label for that parameter.
(from The Swift Programming Language: Redirect)

If using e.g. @unused it could look like:

func test(a: Int) {} // warning normal
func test(a b: Int) {} // warning using argument label
func test(_ a: Int) {} // warning suppressed name
// And same three without warnings would be:
func test(@unused a: Int) {} 
func test(@unused a b: Int) {}
func test(@unused _ a: Int) {}

But the first question is whether the community thinks its a good idea, can hash out details later in that case.

2 Likes
4

I like this idea!

I, too, sometimes face cases where arguments that I thought using are not used in the function, and do not behave as expected like the example. This addition would reduce such bugs.

5

I'd be strongly +1 on this, but only if this warning does not show when the function is part of protocol conformance. Adding even more decorations/attributes to the language to call out an argument as genuinely not needed seems like adding more noise for very little gain, especially given that unused arguments are not unsafe in themselves.

2 Likes
6

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.

7

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
8

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.

9

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 :)

10

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
11

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
12

@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?

13

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
14

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)

15

For documentation purposes or other reasons?

16

For other reasons. Sometimes it makes sense to have an argument on a protocol function but your implementation might not need it.

17

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
18

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!

19

I'm really surprised we didn't add this warning. :confused: 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:

  1. I updated a protocol method to take more arguments, but forgot to use all the arguments in one of the implementations.
  2. 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
20

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