cargo fix renames unused local variables from foo to _foo

[MaikKlein]

For example:

let device = &self.ctx.device;
let _device = &self.ctx.device;

I don't like that refactor because I might end up with local variables that do nothing. I think cargo fix should just ignore unused local variables all together. If cargo fix just renames local variables to hide the warnings from the compiler, then the compiler lint is a bit pointless.

Renaming unused function parameters seems fine.

[frewsxcv]

@MaikKlein To get a better understanding of your thought process, why do you feel cargo fix should treat local variables and function parameters differently here?

[MaikKlein]

For example

let foo = Foo::new();
...
foo.do_something();

Now at one point you remove foo.do_something();, and then cargo fix will rename let foo = Foo::new(); to let _foo = Foo::new();

Now you have you might never find this in your code base. You really wanted to remove it, or maybe it was a bug in the first place that you are not using it anymore.

Just renaming it for the sake of removing the warning seems wrong.

Maybe it should also not rename parameters for free standing functions. I think that there are some cases in functions were you just want to rely on type inference and then the parameter should always be _foo, but I think that decision should come from the programmer.

It seems reasonable to rename parameters for trait impls because you can't just change the function signature, and you might not always want to use the parameter.

impl Foo for Bar {
    fn foo(&self, _baz: Baz){}
}

[zackmdavis]

I think I'm persuaded? Here's where the lint is issued:

rust/src/librustc/middle/liveness.rs

Lines 1546 to 1565 in f789b6b

	if is_assigned {
	self.ir.tcx
	.lint_hir_note(lint::builtin::UNUSED_VARIABLES, hir_id, sp,
	&format!("variable `{}` is assigned to, but never used",
	name),
	&suggest_underscore_msg);
	} else if name != "self" {
	let msg = format!("unused variable: `{}`", name);
	let mut err = self.ir.tcx
	.struct_span_lint_hir(lint::builtin::UNUSED_VARIABLES, hir_id, sp, &msg);
	if self.ir.variable_is_shorthand(var) {
	err.span_suggestion_with_applicability(sp, "try ignoring the field",
	format!("{}: _", name),
	Applicability::MachineApplicable);
	} else {
	err.span_suggestion_short_with_applicability(
	sp, &suggest_underscore_msg,
	format!("_{}", name),
	Applicability::MachineApplicable,
	);

We just need to change Applicability::MachineApplicable to Applicability::MaybeIncorrect to tell cargo fix not to auto-apply the suggestion. We have access to the HIR here, so setting the applicability conditionally based on whether it's a let binding or a method argument should be doable, although potentially slightly tricky.

[killercup]

I think you are right, this is misleading in your use case. Just wondering: At which points in your workflow are you running cargo fix?

I also think this is one of the cases where we need to differentiate between "cargo fix should fix it" and "get a lightbulb with auto-fix suggestions in your editor". Just because it might be auto-fixable doesn't mean we should automatically fix it. This is a discussion for a higher level issue, though.

[MaikKlein]

I only ran cargo fix once for a project where I had a few warnings after I did some refactoring.

[KarelPeeters]

Is anyone against changing this suggestion to simply be MaybeIncorrect instead of MachineApplicable?

Prepending an underscore to variable names is saying to the compiler "yes, I intentionally won't use this value", which should very much be a manual action.

I'm hesitant to use cargo fix, since it might silently hide a bunch of leftover variables from an in-process refactoring. I mostly want it to remove unnecessary parenthesis and fix my imports, this is overreaching a bit.