Cargo fix will rename unused idents in macros even when used elsewhere

[gfaster]

Problem

Consider the following src/main.rs file in a cargo package:

macro_rules! make_var {
    ($struct:ident, $var:ident) => {
        let $var = $struct.$var;
    };
}

#[allow(unused)]
struct MyStruct {
    var: i32,
}

fn main() {
    let s = MyStruct { var: 42 };
    make_var!(s, var);
}

Cargo will warn that var is not used. Upon running cargo fix, cargo will change the second parameter from var to _var, making the compilation fail. Even worse, if MyStruct contained a field _var then cargo fix would change the variable without producing any warning.

Steps

1. Initialize a new cargo binary project with the above snippet in main.rs
2. Run cargo fix (presumably with --allow-dirty)

Possible Solution(s)

No response

Notes

No response

Version

cargo 1.73.0 (9c4383fb5 2023-08-26)

[Muscraft]

cargo fix just applies any MachineApplicable suggestion produced by rustc. In this case, the suggestion from rustc is to replace var with _var.

I went ahead and ran cargo expand to see if it could help me understand what is going on better:

#[allow(unused)]
struct MyStruct {
    var: i32,
}
fn main() {
    let s = MyStruct { var: 42 };
    let var = s.var;
}

(I cleaned up the expanded code slightly, but it is the same)

Looking at the expanded code, it makes me think that rustc is seeing var in make_var!(s, var) as the source of the problem since it expands to let var = s.var; and let var is unused in this case. The normal suggestion is to add an underscore to let <var name> so it would not be "unused". So rustc suggests to change the ident being passed to the macro call as it does not understand that the warning is really coming from within the macro, not the call itself. It also could be suggesting this as let _$var is not valid rust (macro?) syntax. If you change your example slightly, rustc correctly suggests the problem comes from within the macro call.

macro_rules! make_var {
    ($struct:ident, $var:ident) => {
        let var2 = $struct.$var;
    };
}

#[allow(unused)]
struct MyStruct {
    var: i32,
}

fn main() {
    let s = MyStruct { var: 42 };
    make_var!(s, var);
}

warning: unused variable: `var2`
  --> src/main.rs:3:13
   |
3  |         let var2 = $struct.$var;
   |             ^^^^ help: if this is intentional, prefix it with an underscore: `_var2`
...
14 |     make_var!(s, var);
   |     ----------------- in this macro invocation
   |
   = note: `#[warn(unused_variables)]` on by default
   = note: this warning originates in the macro `make_var` (in Nightly builds, run with -Z macro-backtrace for more info)

To me, this seems like a problem with rustc, but I do not know enough about the compiler internals to be 100% certain.

[ehuss]

Transferred to rust-lang/rust, since the suggestions are generated by the compiler.

[chenyukang]

@Muscraft
Yes, this issue comes from rustc.
The lint is emitted from here

rust/compiler/rustc_passes/src/liveness.rs

Line 1660 in 160fd3c

self.ir.tcx.emit_spanned_lint(

Maybe we only point out that there is an unused variable coming from var, but without the suggestion? I'm not sure what is a proper lint for this test case.
@estebank any thoughts on it?

[estebank]

The quick and straightforward way to fix this would be to only emit the suggestion in this lint if the span has the root context, but then a subset of valid suggestions wouldn't be emitted. I had an older PR that I'm having trouble finding that would automatically do that for every diagnostic, but IIRC it had some negative perf impact when not implementing it very simplistic.

In this

rust/compiler/rustc_passes/src/liveness.rs

Lines 1660 to 1675 in 160fd3c

	self.ir.tcx.emit_spanned_lint(
	lint::builtin::UNUSED_VARIABLES,
	first_hir_id,
	hir_ids_and_spans
	.iter()
	.map(|(_, _, ident_span)| *ident_span)
	.collect::<Vec<_>>(),
	errors::UnusedVariableTryPrefix {
	label: if !suggestions.is_empty() { Some(pat.span) } else { None },
	sugg: errors::UnusedVariableTryPrefixSugg {
	spans: non_shorthands,
	name,
	},
	string_interp: suggestions,
	},
	);

we can probably instead check that all of the spans involved have the same .ctxt(). If they don't, don't suggest changes.