parser, checker: notify about unused function parameters

[ttytm]

With changes code like the follow will result in a notification like shown below:

fn foo(unused_var string) {
	println('foo')
}

❯ v run foo.v
foo.v:1:8: notice: unused parameter: `unused_var`
    1 | fn foo(unused_var string) {
      |        ~~~~~~~~~~
    2 |     println('foo')
    3 |     a := 'a'

The change should allow for better detection of dead code. There are many situations where knowing about unused parameters can help resolve unintended behavior or simplify code, such as when removing historical parameter/argument residues after refactorings.

In the standard library, dead parameters are not removed in this PR; they were excluded from the notification. Removing this exclusion could help with the aforementioned benefits when working on the internal code.

Only the current tests have been updated without creating an additional explicit test, as I thought it might suffice to verify the behavior.

Since the change affects a series of test files, a note for the reviewer:
Actual code changes are only in parser/fn.v and checker/checker.v. Selecting those files or reviewing the test update commit (Run autofix for compiler error tests) separately might make things easier.

_{Huly®: V_0.6-21318}

[felipensp]

Good catch. :-)

[felipensp]

Imho the receiver var must be skiped though.

[spytheman]

That should not be the default.
It can be behind a custom flag.

[spytheman]

Revert everything except the check, and add a custom flag for it -check-unused-fn-args or something similar.

[JalonSolov]

Go linters always notify about unused parameters, but allows you to use _ as a placeholder to suppress the warning, as in

fn foo(_ string) {
	println('foo')
}

While you can argue that you shouldn't list a parameter at all if you aren't going to use it, sometimes this is needed if you're using an API from a package, and need to have the parameters listed, but don't use them yourself.

So this sounds like a v vet enhancement, rather than an extra option on V itself.

[ttytm]

Go always notifies about unused parameters,

Rust also notifies about them.

but allows you to use _ as a placeholder to suppress the warning, as in

This would be possible with the changes too - the same as we do for regular variables.

While you can argue that you shouldn't list a parameter at all if you aren't going to use it, sometimes this is needed if you're using an API from a package, and need to have the parameters listed, but don't use them yourself.

Thats the case for public functions in go as well. Also I handled it that way. It would not trigger for public functions.

[ttytm]

So this sounds like a v vet enhancement, rather than an extra option on V itself.

Yep rather a v vet thing instead of an additional flag. But imo the compiler should show it by default to really be useful.

Go throws errors for unused variables and warns about params (allows compilation). We do warn about variables (error in prod). The change would notify about unused params (allow them in prod).

@spytheman what do you think?

[JalonSolov]

Warn regular, error in prod is a good thing. If V is already doing that for variables, it really should be doing the same for parameters, too.

[ttytm]

Warn regular, error in prod is a good thing. If V is already doing that for variables.

I think so too. This wouldn't change here. With the notify for params we would allow prod compilation in the same situations as Go does.

it really should be doing the same for parameters, too.

Turning it into a warning for params would be too strict I think. Lot of things wouldn't compile in prod anymore. That's why I chose the notify call for unused params.

[JalonSolov]

Yeah... I'm not really a fan of Go's way. Probably because I have to check everything in via PR at work, and the PR always runs linters, and the linters refuse my code if there are unused params, even if the code compiled clean locally.

That means I have to take the extra step to run the linters locally if I want a clean PR. :-\

I would rather be told up front when I compile.

[ttytm]

Having a notify by default should help to achieve that.

[spytheman]

It should not be a default for the compiler.
Function/method declarations are API boundaries - they are a contract between the calling code (users), and the implementing code.
What happens inside a function, should not dictate what that API boundary looks like.

[spytheman]

It can be a vet default, but even in that case, it is highly preferable for me, and people that develop like me, to have it as an option, not for it to be something that dictates policy by default for the entire V ecosystem.

[spytheman]

Local variables are LOCAL. They are not API boundaries, and changing/removing/adding more of them, does not change the API, and force cascading changes in unrelated parts of your codebase.

[spytheman]

Rust also notifies about them.

Can you show a specific example program, where it does so?

[spytheman]

Go linters always notify about unused parameters, but allows you to use _ as a placeholder to suppress the warning

If you can use _ as a placeholder, so that the API is not changed, that is more acceptable.

[spytheman]

Although replacing a meaningful parameter name with _ to silence the notice/warning/error, still loses information 🤔 . But then, that information is perhaps not important, given that it is not used.

[spytheman]

Ok, I think I missed the nuance of what it does, and misunderstood it to introduce a very annoying noise, with no way to opt out, for a situation that happens often, if you have to adhere to a fixed API.

[ttytm]

@spytheman

You taking into account that it would trigger only for local functions? If it's a pub fn the unused param name would not trigger the compiler message. Same as it would for a Go func foo and not for params in a func Foo.

Can you show a specific example program, where it does so?

fn foo(s: &str) {
	println!("not s");
}

fn main() {}

❯ cargo run
warning: unused variable: `s`
 --> main.rs:1:8
  |
1 | fn foo(s: &str) {
  |        ^ help: if this is intentional, prefix it with an underscore: `_s`
  |
  = note: `#[warn(unused_variables)]` on by default

warning: function `foo` is never used
 --> main.rs:1:4
  |
1 | fn foo(s: &str) {
  |    ^^^
  |
  = note: `#[warn(dead_code)]` on by default

[ttytm]

Btw regarding the pub visibility modifier: rust would warn also only warn about a localy unused struct:

// rust
struct Foo {} // struct `Foo` is never constructed `#[warn(dead_code)]` on by default [dead_code]
pub struct Foo {} // no warning

[spytheman]

The diff though is still too big - most of the .out files are now polluted with that unrelated notice :-| .

[spytheman]

Go's defaults for warnings/errors are far from optimal (from my perspective) - especially the unused local/import one is super annoying while prototyping.

[spytheman]

You taking into account that it would trigger only for local functions? If it's a pub fn the unused param name would not trigger the compiler message.

Yes, I am now, but that is exactly what I missed on first reading, and why I closed the PR initially.

[ttytm]

The diff though is still too big - most of the .out files are now polluted with that unrelated notice :-| .

It would go down quite a bit if applying @felipensp s suggestion. Skipping it for the receiver variables.

[spytheman]

I still think that it should be behind a flag initially though.
It will help to smooth out the transition period, as well as help with adjusting the behavior if needed, without causing diff churn all the time that happens.

[JalonSolov]

Having that notice in all the .out files is just a sign that we've been missing this for too long.

[ttytm]

I still think that it should be behind a flag initially though.
It will help to smooth out the transition period, as well as help with adjusting the behavior if needed, without causing diff churn all the time that happens.

It will be hard to decide when the transition period ends when getting the info is opt-in. I think it's as @JalonSolov says in his last comment, getting the info is likely just overdue.

Maybe then an as an on-by-default but adding a temporary opt-out vflag / env variable for a year? Which would allow to silence it from one central place without code changes.

I'll see how much the diff is reduced by applying the receiver var change, that could help with the decision.

[spytheman]

No, I want a command line option, and for it to be off by default initially.

[spytheman]

Having that notice in all the .out files is just a sign that we've been missing this for too long.

No, it is not. The people that wrote those small examples were not careless, and did not miss/introduce bugs. The code examples in the .vv files are such, to introduce structure/setup an AST in such a way, as to trigger checker/parser errors.

It is entirely unrelated to this change, except that it shows, that it is not a default to be introduced without a grace period, in which it can be fine tuned and in which only people that know about it and actively work on it, will use it.

I am sure that a lot of other codebases will have similar examples, and they are not errors there either - I do not want to break their CI jobs or workflows either.

go2v, c2v, v-analyzer all come to mind as examples of such projects.

[spytheman]

It will be hard to decide when the transition period ends when getting the info is opt-in.

I am thinking of a much shorter period, like a week or so - it is enough time to test it without breaking stuff too much, and to fine tune it, and to fix official V projects like v-analyzer.

After that, the flag will be removed, and it will be a default.

[spytheman]

(that is because unlike -Wimpure-v, or the ?!Type split, which can force people to refactor/move code a lot, the notices here, will be mostly local in scope, and easy to fix with local changes)

[JalonSolov]

I never said anything about people being careless writing tests. I meant that if we had this sooner, we wouldn't "pollute" all those files now, with the extra message.

[medvednikov]

I think @JalonSolov is right.

We can have an option not do this, and enable it for certain tests until the tests are updated (param name replaced with _).

[medvednikov]

But it should be on by default.

[ttytm]

With the linked PR there are about 20 notifications for the vlib internals when running v -check-unused-fn-args self. So it's not too much of changes for the vlib.
For tests and examples there are a bit more of course, but having applied Felipe's suggestion reduced the diff quite a bit. It's feasible to update them.

---

What the compiler currently doesn't disallow for params is prefixing them with _. So it actually already works with the linked PR to use _param_name to disable the notification for an unused param_name. This is how it would work in Rust too (visible in the compiler warning in the comment above #22875 (comment)). Personally I like that. What are your thoughts on that @medvednikov @spytheman @JalonSolov ?

[JalonSolov]

We already use _ alone to mean an "ignored" variable. The same should be used for unused parameters. There's no need for yet another convention just to handle this.

[ttytm]

We already use _ alone to mean an "ignored" variable. The same should be used for unused parameters. There's no need for yet another convention just to handle this.

It could be reduced to this by checking the whole param name. Currently only the first char was checked since prefixing a variable with _ is prohibited while it is allowed for a param.

Giving into consideration: What I like about the prefix is that it's a simple way to keep semantics intact in a functions like below as it can be helpful to have something more meaningful than just _.

v/vlib/os/process_nix.c.v

Lines 153 to 155 in be4aec8

	fn (mut p Process) win_read_string(idx int, maxbytes int) (string, int) {
	return '', 0
	}

Examples for the above:

fn (mut p Process) win_read_string(_idx int, _maxbytes int) (string, int) {
	return '', 0
}

An alternative to silence the notification when the prefix is not possible would then be:

fn (mut p Process) win_read_string(idx int, maxbytes int) (string, int) {
	_, _ := idx, maxbytes
	return '', 0
}

// With below I would forget what the params were intended to be.
fn (mut p Process) win_read_string(_ int, _ int) (string, int) {
	return '', 0
}

[JalonSolov]

How does having a name for a parameter that's never used make it "more meaningful"?

It doesn't matter at all what they were intended to be if they're never used in the code... not using them implies they were always meant to be ignored anyway, so why bother naming them?

If you are doing this to match an "outside" API (for example, a callback for a function in another module), you can always check the callback definition in the other module.

However, if it won't slow things down too much, then it really doesn't matter if you allow _param_name... although that still has to be processed by the parser, at least (even if just to skip to the end), which means it will be at least a bit slower than _ by itself.