Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
gfaster opened this issue Oct 27, 2023 · 4 comments · Fixed by #117390
Closed

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

gfaster opened this issue Oct 27, 2023 · 4 comments · Fixed by #117390
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-bug Category: This is a bug. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@gfaster
Copy link

gfaster commented Oct 27, 2023

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)
@gfaster gfaster added the C-bug Category: This is a bug. label Oct 27, 2023
@Muscraft
Copy link
Member

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 ehuss transferred this issue from rust-lang/cargo Oct 27, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 27, 2023
@ehuss ehuss added the D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. label Oct 27, 2023
@ehuss
Copy link
Contributor

ehuss commented Oct 27, 2023

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

@chenyukang chenyukang self-assigned this Oct 27, 2023
@saethlin saethlin removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Oct 28, 2023
@chenyukang chenyukang removed their assignment Oct 28, 2023
@chenyukang
Copy link
Member

chenyukang commented Oct 29, 2023

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

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 estebank added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` labels Oct 30, 2023
@estebank
Copy link
Contributor

estebank commented Oct 30, 2023

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

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.

@chenyukang chenyukang self-assigned this Oct 30, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 30, 2023
…d-macro, r=estebank

Fix unused variables lint issue for args in macro

Fixes rust-lang#117284
r? `@estebank`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 30, 2023
…d-macro, r=estebank

Fix unused variables lint issue for args in macro

Fixes rust-lang#117284
r? ``@estebank``
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Oct 30, 2023
…d-macro, r=estebank

Fix unused variables lint issue for args in macro

Fixes rust-lang#117284
r? ```@estebank```
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Oct 30, 2023
…d-macro, r=estebank

Fix unused variables lint issue for args in macro

Fixes rust-lang#117284
r? ````@estebank````
@bors bors closed this as completed in 82f34fd Oct 30, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Oct 30, 2023
Rollup merge of rust-lang#117390 - chenyukang:yukang-fix-117284-unused-macro, r=estebank

Fix unused variables lint issue for args in macro

Fixes rust-lang#117284
r? ````@estebank````
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-bug Category: This is a bug. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants