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

Highlight identifier span instead of whole pattern span in unused lint #82655

Merged
merged 3 commits into from
Mar 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 66 additions & 43 deletions compiler/rustc_passes/src/liveness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1494,12 +1494,13 @@ impl<'tcx> Liveness<'_, 'tcx> {
// bindings, and we also consider the first pattern to be the "authoritative" set of ids.
// However, we should take the ids and spans of variables with the same name from the later
// patterns so the suggestions to prefix with underscores will apply to those too.
let mut vars: FxIndexMap<Symbol, (LiveNode, Variable, Vec<(HirId, Span)>)> = <_>::default();
let mut vars: FxIndexMap<Symbol, (LiveNode, Variable, Vec<(HirId, Span, Span)>)> =
<_>::default();

pat.each_binding(|_, hir_id, pat_sp, ident| {
let ln = entry_ln.unwrap_or_else(|| self.live_node(hir_id, pat_sp));
let var = self.variable(hir_id, ident.span);
let id_and_sp = (hir_id, pat_sp);
let id_and_sp = (hir_id, pat_sp, ident.span);
vars.entry(self.ir.variable_name(var))
.and_modify(|(.., hir_ids_and_spans)| hir_ids_and_spans.push(id_and_sp))
.or_insert_with(|| (ln, var, vec![id_and_sp]));
Expand All @@ -1508,15 +1509,21 @@ impl<'tcx> Liveness<'_, 'tcx> {
for (_, (ln, var, hir_ids_and_spans)) in vars {
if self.used_on_entry(ln, var) {
let id = hir_ids_and_spans[0].0;
let spans = hir_ids_and_spans.into_iter().map(|(_, sp)| sp).collect();
let spans =
hir_ids_and_spans.into_iter().map(|(_, _, ident_span)| ident_span).collect();
on_used_on_entry(spans, id, ln, var);
} else {
self.report_unused(hir_ids_and_spans, ln, var);
}
}
}

fn report_unused(&self, hir_ids_and_spans: Vec<(HirId, Span)>, ln: LiveNode, var: Variable) {
fn report_unused(
&self,
hir_ids_and_spans: Vec<(HirId, Span, Span)>,
ln: LiveNode,
var: Variable,
) {
let first_hir_id = hir_ids_and_spans[0].0;

if let Some(name) = self.should_warn(var).filter(|name| name != "self") {
Expand All @@ -1530,62 +1537,78 @@ impl<'tcx> Liveness<'_, 'tcx> {
self.ir.tcx.struct_span_lint_hir(
lint::builtin::UNUSED_VARIABLES,
first_hir_id,
hir_ids_and_spans.into_iter().map(|(_, sp)| sp).collect::<Vec<_>>(),
hir_ids_and_spans
.into_iter()
.map(|(_, _, ident_span)| ident_span)
.collect::<Vec<_>>(),
|lint| {
lint.build(&format!("variable `{}` is assigned to, but never used", name))
.note(&format!("consider using `_{}` instead", name))
.emit();
},
)
} else {
self.ir.tcx.struct_span_lint_hir(
lint::builtin::UNUSED_VARIABLES,
first_hir_id,
hir_ids_and_spans.iter().map(|(_, sp)| *sp).collect::<Vec<_>>(),
|lint| {
let mut err = lint.build(&format!("unused variable: `{}`", name));

let (shorthands, non_shorthands): (Vec<_>, Vec<_>) =
hir_ids_and_spans.into_iter().partition(|(hir_id, span)| {
let var = self.variable(*hir_id, *span);
self.ir.variable_is_shorthand(var)
});

let mut shorthands = shorthands
.into_iter()
.map(|(_, span)| (span, format!("{}: _", name)))
.collect::<Vec<_>>();

// If we have both shorthand and non-shorthand, prefer the "try ignoring
// the field" message, and suggest `_` for the non-shorthands. If we only
// have non-shorthand, then prefix with an underscore instead.
if !shorthands.is_empty() {
shorthands.extend(
non_shorthands
.into_iter()
.map(|(_, span)| (span, "_".to_string()))
.collect::<Vec<_>>(),
);
let (shorthands, non_shorthands): (Vec<_>, Vec<_>) =
hir_ids_and_spans.iter().copied().partition(|(hir_id, _, ident_span)| {
let var = self.variable(*hir_id, *ident_span);
self.ir.variable_is_shorthand(var)
});

// If we have both shorthand and non-shorthand, prefer the "try ignoring
// the field" message, and suggest `_` for the non-shorthands. If we only
// have non-shorthand, then prefix with an underscore instead.
if !shorthands.is_empty() {
let shorthands = shorthands
.into_iter()
.map(|(_, pat_span, _)| (pat_span, format!("{}: _", name)))
.chain(
non_shorthands
.into_iter()
.map(|(_, pat_span, _)| (pat_span, "_".to_string())),
)
.collect::<Vec<_>>();

self.ir.tcx.struct_span_lint_hir(
lint::builtin::UNUSED_VARIABLES,
first_hir_id,
hir_ids_and_spans
.iter()
.map(|(_, pat_span, _)| *pat_span)
.collect::<Vec<_>>(),
|lint| {
let mut err = lint.build(&format!("unused variable: `{}`", name));
err.multipart_suggestion(
"try ignoring the field",
shorthands,
Applicability::MachineApplicable,
);
} else {
err.emit()
},
);
} else {
let non_shorthands = non_shorthands
.into_iter()
.map(|(_, _, ident_span)| (ident_span, format!("_{}", name)))
.collect::<Vec<_>>();

self.ir.tcx.struct_span_lint_hir(
lint::builtin::UNUSED_VARIABLES,
first_hir_id,
hir_ids_and_spans
.iter()
.map(|(_, _, ident_span)| *ident_span)
.collect::<Vec<_>>(),
|lint| {
let mut err = lint.build(&format!("unused variable: `{}`", name));
err.multipart_suggestion(
"if this is intentional, prefix it with an underscore",
non_shorthands
.into_iter()
.map(|(_, span)| (span, format!("_{}", name)))
.collect::<Vec<_>>(),
non_shorthands,
Applicability::MachineApplicable,
);
}

err.emit()
},
);
err.emit()
},
);
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,16 +12,16 @@ LL | #![warn(unused)] // UI tests pass `-A unused` (#43896)
= note: `#[warn(unused_variables)]` implied by `#[warn(unused)]`

warning: unused variable: `mut_unused_var`
--> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:33:9
--> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:33:13
|
LL | let mut mut_unused_var = 1;
| ^^^^^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_mut_unused_var`
| ^^^^^^^^^^^^^^ help: if this is intentional, prefix it with an underscore: `_mut_unused_var`

warning: unused variable: `var`
--> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:37:10
--> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:37:14
|
LL | let (mut var, unused_var) = (1, 2);
| ^^^^^^^ help: if this is intentional, prefix it with an underscore: `_var`
| ^^^ help: if this is intentional, prefix it with an underscore: `_var`

warning: unused variable: `unused_var`
--> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:37:19
Expand All @@ -36,10 +36,10 @@ LL | if let SoulHistory { corridors_of_light,
| ^^^^^^^^^^^^^^^^^^ help: try ignoring the field: `corridors_of_light: _`

warning: variable `hours_are_suns` is assigned to, but never used
--> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:46:26
--> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:46:30
|
LL | mut hours_are_suns,
| ^^^^^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^
|
= note: consider using `_hours_are_suns` instead

Expand Down
12 changes: 12 additions & 0 deletions src/test/ui/lint/issue-81314-unused-span-ident.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// run-rustfix
// Regression test for #81314: Unused variable lint should
// span only the identifier and not the rest of the pattern

#![deny(unused)]

fn main() {
let [_rest @ ..] = [1, 2, 3]; //~ ERROR unused variable
}

pub fn foo([_rest @ ..]: &[i32]) { //~ ERROR unused variable
}
12 changes: 12 additions & 0 deletions src/test/ui/lint/issue-81314-unused-span-ident.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// run-rustfix
// Regression test for #81314: Unused variable lint should
// span only the identifier and not the rest of the pattern

#![deny(unused)]

fn main() {
let [rest @ ..] = [1, 2, 3]; //~ ERROR unused variable
}

pub fn foo([rest @ ..]: &[i32]) { //~ ERROR unused variable
}
21 changes: 21 additions & 0 deletions src/test/ui/lint/issue-81314-unused-span-ident.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
error: unused variable: `rest`
--> $DIR/issue-81314-unused-span-ident.rs:8:10
|
LL | let [rest @ ..] = [1, 2, 3];
| ^^^^ help: if this is intentional, prefix it with an underscore: `_rest`
|
note: the lint level is defined here
--> $DIR/issue-81314-unused-span-ident.rs:5:9
|
LL | #![deny(unused)]
| ^^^^^^
= note: `#[deny(unused_variables)]` implied by `#[deny(unused)]`

error: unused variable: `rest`
--> $DIR/issue-81314-unused-span-ident.rs:11:13
|
LL | pub fn foo([rest @ ..]: &[i32]) {
| ^^^^ help: if this is intentional, prefix it with an underscore: `_rest`

error: aborting due to 2 previous errors

4 changes: 2 additions & 2 deletions src/test/ui/liveness/liveness-consts.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
warning: variable `a` is assigned to, but never used
--> $DIR/liveness-consts.rs:7:9
--> $DIR/liveness-consts.rs:7:13
|
LL | let mut a = 0;
| ^^^^^
| ^
|
note: the lint level is defined here
--> $DIR/liveness-consts.rs:2:9
Expand Down
8 changes: 4 additions & 4 deletions src/test/ui/liveness/liveness-dead.stderr
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
error: value assigned to `x` is never read
--> $DIR/liveness-dead.rs:9:9
--> $DIR/liveness-dead.rs:9:13
|
LL | let mut x: isize = 3;
| ^^^^^
| ^
|
note: the lint level is defined here
--> $DIR/liveness-dead.rs:2:9
Expand All @@ -20,10 +20,10 @@ LL | x = 4;
= help: maybe it is overwritten before being read?

error: value passed to `x` is never read
--> $DIR/liveness-dead.rs:20:7
--> $DIR/liveness-dead.rs:20:11
|
LL | fn f4(mut x: i32) {
| ^^^^^
| ^
|
= help: maybe it is overwritten before being read?

Expand Down
8 changes: 4 additions & 4 deletions src/test/ui/liveness/liveness-unused.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,10 @@ LL | let x = 3;
| ^ help: if this is intentional, prefix it with an underscore: `_x`

error: variable `x` is assigned to, but never used
--> $DIR/liveness-unused.rs:30:9
--> $DIR/liveness-unused.rs:30:13
|
LL | let mut x = 3;
| ^^^^^
| ^
|
= note: consider using `_x` instead

Expand All @@ -65,10 +65,10 @@ LL | #![deny(unused_assignments)]
= help: maybe it is overwritten before being read?

error: variable `z` is assigned to, but never used
--> $DIR/liveness-unused.rs:37:9
--> $DIR/liveness-unused.rs:37:13
|
LL | let mut z = 3;
| ^^^^^
| ^
|
= note: consider using `_z` instead

Expand Down