Skip to content

Commit

Permalink
Rollup merge of #67766 - sapir:fix-unused-in-or-pattern-warning, r=ma…
Browse files Browse the repository at this point in the history
…tthewjasper

Fix warning for unused variables in or pattern (issue #67691)

Is this a good way to fix it?

Also, the tests fail, the "fixed" code output says `{ i, j }` instead of `{ i, j: _ }`, how can I fix that?
  • Loading branch information
Dylan-DPC authored Apr 12, 2020
2 parents 3712e11 + 3221638 commit 6947dec
Show file tree
Hide file tree
Showing 7 changed files with 229 additions and 52 deletions.
75 changes: 49 additions & 26 deletions src/librustc_passes/liveness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1492,28 +1492,33 @@ impl<'tcx> Liveness<'_, 'tcx> {
) {
// In an or-pattern, only consider the variable; any later patterns must have the same
// bindings, and we also consider the first pattern to be the "authoritative" set of ids.
// However, we should take the spans of variables with the same name from the later
// 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<String, (LiveNode, Variable, HirId, Vec<Span>)> = <_>::default();
let mut vars: FxIndexMap<String, (LiveNode, Variable, Vec<(HirId, 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);
vars.entry(self.ir.variable_name(var))
.and_modify(|(.., spans)| spans.push(ident.span))
.or_insert_with(|| (ln, var, hir_id, vec![ident.span]));
.and_modify(|(.., hir_ids_and_spans)| hir_ids_and_spans.push(id_and_sp))
.or_insert_with(|| (ln, var, vec![id_and_sp]));
});

for (_, (ln, var, id, spans)) in vars {
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();
on_used_on_entry(spans, id, ln, var);
} else {
self.report_unused(spans, id, ln, var);
self.report_unused(hir_ids_and_spans, ln, var);
}
}
}

fn report_unused(&self, spans: Vec<Span>, hir_id: HirId, ln: LiveNode, var: Variable) {
fn report_unused(&self, hir_ids_and_spans: Vec<(HirId, 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") {
// annoying: for parameters in funcs like `fn(x: i32)
// {ret}`, there is only one node, so asking about
Expand All @@ -1524,8 +1529,8 @@ impl<'tcx> Liveness<'_, 'tcx> {
if is_assigned {
self.ir.tcx.struct_span_lint_hir(
lint::builtin::UNUSED_VARIABLES,
hir_id,
spans,
first_hir_id,
hir_ids_and_spans.into_iter().map(|(_, sp)| sp).collect::<Vec<_>>(),
|lint| {
lint.build(&format!("variable `{}` is assigned to, but never used", name))
.note(&format!("consider using `_{}` instead", name))
Expand All @@ -1535,31 +1540,49 @@ impl<'tcx> Liveness<'_, 'tcx> {
} else {
self.ir.tcx.struct_span_lint_hir(
lint::builtin::UNUSED_VARIABLES,
hir_id,
spans.clone(),
first_hir_id,
hir_ids_and_spans.iter().map(|(_, sp)| *sp).collect::<Vec<_>>(),
|lint| {
let mut err = lint.build(&format!("unused variable: `{}`", name));
if self.ir.variable_is_shorthand(var) {
if let Node::Binding(pat) = self.ir.tcx.hir().get(hir_id) {
// Handle `ref` and `ref mut`.
let spans = spans
.iter()
.map(|_span| (pat.span, format!("{}: _", name)))
.collect();

err.multipart_suggestion(
"try ignoring the field",
spans,
Applicability::MachineApplicable,
);
}

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<_>>(),
);

err.multipart_suggestion(
"try ignoring the field",
shorthands,
Applicability::MachineApplicable,
);
} else {
err.multipart_suggestion(
"if this is intentional, prefix it with an underscore",
spans.iter().map(|span| (*span, format!("_{}", name))).collect(),
non_shorthands
.into_iter()
.map(|(_, span)| (span, format!("_{}", name)))
.collect::<Vec<_>>(),
Applicability::MachineApplicable,
);
}

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:13
--> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:33:9
|
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:14
--> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:37:10
|
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:30
--> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:46:26
|
LL | mut hours_are_suns,
| ^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^^^^^
|
= note: consider using `_hours_are_suns` instead

Expand Down
18 changes: 6 additions & 12 deletions src/test/ui/lint/issue-54180-unused-ref-field.stderr
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
error: unused variable: `field`
--> $DIR/issue-54180-unused-ref-field.rs:20:26
--> $DIR/issue-54180-unused-ref-field.rs:20:22
|
LL | E::Variant { ref field } => (),
| ----^^^^^
| |
| help: try ignoring the field: `field: _`
| ^^^^^^^^^ help: try ignoring the field: `field: _`
|
note: the lint level is defined here
--> $DIR/issue-54180-unused-ref-field.rs:3:9
Expand All @@ -20,20 +18,16 @@ LL | let _: i32 = points.iter().map(|Point { x, y }| y).sum();
| ^ help: try ignoring the field: `x: _`

error: unused variable: `f1`
--> $DIR/issue-54180-unused-ref-field.rs:26:17
--> $DIR/issue-54180-unused-ref-field.rs:26:13
|
LL | let S { ref f1 } = s;
| ----^^
| |
| help: try ignoring the field: `f1: _`
| ^^^^^^ help: try ignoring the field: `f1: _`

error: unused variable: `x`
--> $DIR/issue-54180-unused-ref-field.rs:32:28
--> $DIR/issue-54180-unused-ref-field.rs:32:20
|
LL | Point { y, ref mut x } => y,
| --------^
| |
| help: try ignoring the field: `x: _`
| ^^^^^^^^^ help: try ignoring the field: `x: _`

error: aborting due to 4 previous errors

86 changes: 86 additions & 0 deletions src/test/ui/lint/issue-67691-unused-field-in-or-pattern.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
// FIXME: should be run-rustfix, but rustfix doesn't currently support multipart suggestions, see
// #53934

#![feature(or_patterns)]
#![deny(unused)]

pub enum MyEnum {
A { i: i32, j: i32 },
B { i: i32, j: i32 },
}

pub enum MixedEnum {
A { i: i32 },
B(i32),
}

pub fn no_ref(x: MyEnum) {
use MyEnum::*;

match x {
A { i, j } | B { i, j } => { //~ ERROR unused variable
println!("{}", i);
}
}
}

pub fn with_ref(x: MyEnum) {
use MyEnum::*;

match x {
A { i, ref j } | B { i, ref j } => { //~ ERROR unused variable
println!("{}", i);
}
}
}

pub fn inner_no_ref(x: Option<MyEnum>) {
use MyEnum::*;

match x {
Some(A { i, j } | B { i, j }) => { //~ ERROR unused variable
println!("{}", i);
}

_ => {}
}
}

pub fn inner_with_ref(x: Option<MyEnum>) {
use MyEnum::*;

match x {
Some(A { i, ref j } | B { i, ref j }) => { //~ ERROR unused variable
println!("{}", i);
}

_ => {}
}
}

pub fn mixed_no_ref(x: MixedEnum) {
match x {
MixedEnum::A { i } | MixedEnum::B(i) => { //~ ERROR unused variable
println!("match");
}
}
}

pub fn mixed_with_ref(x: MixedEnum) {
match x {
MixedEnum::A { ref i } | MixedEnum::B(ref i) => { //~ ERROR unused variable
println!("match");
}
}
}

pub fn main() {
no_ref(MyEnum::A { i: 1, j: 2 });
with_ref(MyEnum::A { i: 1, j: 2 });

inner_no_ref(Some(MyEnum::A { i: 1, j: 2 }));
inner_with_ref(Some(MyEnum::A { i: 1, j: 2 }));

mixed_no_ref(MixedEnum::B(5));
mixed_with_ref(MixedEnum::B(5));
}
74 changes: 74 additions & 0 deletions src/test/ui/lint/issue-67691-unused-field-in-or-pattern.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
error: unused variable: `j`
--> $DIR/issue-67691-unused-field-in-or-pattern.rs:21:16
|
LL | A { i, j } | B { i, j } => {
| ^ ^
|
note: the lint level is defined here
--> $DIR/issue-67691-unused-field-in-or-pattern.rs:5:9
|
LL | #![deny(unused)]
| ^^^^^^
= note: `#[deny(unused_variables)]` implied by `#[deny(unused)]`
help: try ignoring the field
|
LL | A { i, j: _ } | B { i, j: _ } => {
| ^^^^ ^^^^

error: unused variable: `j`
--> $DIR/issue-67691-unused-field-in-or-pattern.rs:31:16
|
LL | A { i, ref j } | B { i, ref j } => {
| ^^^^^ ^^^^^
|
help: try ignoring the field
|
LL | A { i, j: _ } | B { i, j: _ } => {
| ^^^^ ^^^^

error: unused variable: `j`
--> $DIR/issue-67691-unused-field-in-or-pattern.rs:41:21
|
LL | Some(A { i, j } | B { i, j }) => {
| ^ ^
|
help: try ignoring the field
|
LL | Some(A { i, j: _ } | B { i, j: _ }) => {
| ^^^^ ^^^^

error: unused variable: `j`
--> $DIR/issue-67691-unused-field-in-or-pattern.rs:53:21
|
LL | Some(A { i, ref j } | B { i, ref j }) => {
| ^^^^^ ^^^^^
|
help: try ignoring the field
|
LL | Some(A { i, j: _ } | B { i, j: _ }) => {
| ^^^^ ^^^^

error: unused variable: `i`
--> $DIR/issue-67691-unused-field-in-or-pattern.rs:63:24
|
LL | MixedEnum::A { i } | MixedEnum::B(i) => {
| ^ ^
|
help: try ignoring the field
|
LL | MixedEnum::A { i: _ } | MixedEnum::B(_) => {
| ^^^^ ^

error: unused variable: `i`
--> $DIR/issue-67691-unused-field-in-or-pattern.rs:71:24
|
LL | MixedEnum::A { ref i } | MixedEnum::B(ref i) => {
| ^^^^^ ^^^^^
|
help: try ignoring the field
|
LL | MixedEnum::A { i: _ } | MixedEnum::B(_) => {
| ^^^^ ^

error: aborting due to 6 previous errors

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:13
--> $DIR/liveness-dead.rs:9:9
|
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:11
--> $DIR/liveness-dead.rs:20:7
|
LL | fn f4(mut x: i32) {
| ^
| ^^^^^
|
= help: maybe it is overwritten before being read?

Expand Down
Loading

0 comments on commit 6947dec

Please sign in to comment.