Auto merge of #57899 - davidtwco:issue-56685, r=estebank

Unused variable suggestions apply on all patterns. Fixes #56685. This PR extends existing suggestions to prefix unused variable bindings in match arms with an underscore so that it applies to all patterns in a match arm. r? @estebank cc @alexcrichton (since you filed the issue)
rust-lang · Jan 28, 2019 · a21bd75 · a21bd75
2 parents cc4d1e5 + 5f021e0
commit a21bd75
Show file tree

Hide file tree

Showing 17 changed files with 179 additions and 59 deletions.
diff --git a/src/librustc/middle/liveness.rs b/src/librustc/middle/liveness.rs
@@ -105,7 +105,7 @@ use lint;
 use errors::Applicability;
 use util::nodemap::{NodeMap, HirIdMap, HirIdSet};
 
-use std::collections::VecDeque;
+use std::collections::{BTreeMap, VecDeque};
 use std::{fmt, u32};
 use std::io::prelude::*;
 use std::io;
@@ -1446,7 +1446,7 @@ fn check_local<'a, 'tcx>(this: &mut Liveness<'a, 'tcx>, local: &'tcx hir::Local)
         None => {
             this.pat_bindings(&local.pat, |this, ln, var, sp, id| {
                 let span = local.pat.simple_ident().map_or(sp, |ident| ident.span);
-                this.warn_about_unused(span, id, ln, var);
+                this.warn_about_unused(vec![span], id, ln, var);
             })
         }
     }
@@ -1455,12 +1455,29 @@ fn check_local<'a, 'tcx>(this: &mut Liveness<'a, 'tcx>, local: &'tcx hir::Local)
 }
 
 fn check_arm<'a, 'tcx>(this: &mut Liveness<'a, 'tcx>, arm: &'tcx hir::Arm) {
-    // only consider the first pattern; any later patterns must have
-    // the same bindings, and we also consider the first pattern to be
-    // the "authoritative" set of ids
-    this.arm_pats_bindings(arm.pats.first().map(|p| &**p), |this, ln, var, sp, id| {
-        this.warn_about_unused(sp, id, ln, var);
-    });
+    // Only consider the variable from the first pattern; 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
+    // patterns so the suggestions to prefix with underscores will apply to those too.
+    let mut vars: BTreeMap<String, (LiveNode, Variable, HirId, Vec<Span>)> = Default::default();
+
+    for pat in &arm.pats {
+        this.arm_pats_bindings(Some(&*pat), |this, ln, var, sp, id| {
+            let name = this.ir.variable_name(var);
+            vars.entry(name)
+                .and_modify(|(.., spans)| {
+                    spans.push(sp);
+                })
+                .or_insert_with(|| {
+                    (ln, var, id, vec![sp])
+                });
+        });
+    }
+
+    for (_, (ln, var, id, spans)) in vars {
+        this.warn_about_unused(spans, id, ln, var);
+    }
+
     intravisit::walk_arm(this, arm);
 }
 
@@ -1551,7 +1568,7 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> {
                 let var = self.variable(hir_id, sp);
                 // Ignore unused self.
                 if ident.name != keywords::SelfLower.name() {
-                    if !self.warn_about_unused(sp, hir_id, entry_ln, var) {
+                    if !self.warn_about_unused(vec![sp], hir_id, entry_ln, var) {
                         if self.live_on_entry(entry_ln, var).is_none() {
                             self.report_dead_assign(hir_id, sp, var, true);
                         }
@@ -1563,14 +1580,14 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> {
 
     fn warn_about_unused_or_dead_vars_in_pat(&mut self, pat: &hir::Pat) {
         self.pat_bindings(pat, |this, ln, var, sp, id| {
-            if !this.warn_about_unused(sp, id, ln, var) {
+            if !this.warn_about_unused(vec![sp], id, ln, var) {
                 this.warn_about_dead_assign(sp, id, ln, var);
             }
         })
     }
 
     fn warn_about_unused(&self,
-                         sp: Span,
+                         spans: Vec<Span>,
                          hir_id: HirId,
                          ln: LiveNode,
                          var: Variable)
@@ -1587,33 +1604,36 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> {
                     self.assigned_on_exit(ln, var).is_some()
                 };
 
-                let suggest_underscore_msg = format!("consider using `_{}` instead", name);
-
                 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);
+                    self.ir.tcx.lint_hir_note(
+                        lint::builtin::UNUSED_VARIABLES,
+                        hir_id,
+                        spans.clone(),
+                        &format!("variable `{}` is assigned to, but never used", name),
+                        &format!("consider using `_{}` instead", name),
+                    );
                 } 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);
+                    let mut err = self.ir.tcx.struct_span_lint_hir(
+                        lint::builtin::UNUSED_VARIABLES,
+                        hir_id,
+                        spans.clone(),
+                        &format!("unused variable: `{}`", name),
+                    );
+
                     if self.ir.variable_is_shorthand(var) {
-                        err.span_suggestion(
-                            sp,
+                        err.multipart_suggestion(
                             "try ignoring the field",
-                            format!("{}: _", name),
-                            Applicability::MachineApplicable,
+                            spans.iter().map(|span| (*span, format!("{}: _", name))).collect(),
+                            Applicability::MachineApplicable
                         );
                     } else {
-                        err.span_suggestion_short(
-                            sp,
-                            &suggest_underscore_msg,
-                            format!("_{}", name),
+                        err.multipart_suggestion(
+                            "consider prefixing with an underscore",
+                            spans.iter().map(|span| (*span, format!("_{}", name))).collect(),
                             Applicability::MachineApplicable,
                         );
                     }
+
                     err.emit()
                 }
             }
@@ -1623,11 +1643,7 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> {
         }
     }
 
-    fn warn_about_dead_assign(&self,
-                              sp: Span,
-                              hir_id: HirId,
-                              ln: LiveNode,
-                              var: Variable) {
+    fn warn_about_dead_assign(&self, sp: Span, hir_id: HirId, ln: LiveNode, var: Variable) {
         if self.live_on_exit(ln, var).is_none() {
             self.report_dead_assign(hir_id, sp, var, false);
         }

diff --git a/src/test/ui/issues/issue-17999.stderr b/src/test/ui/issues/issue-17999.stderr
@@ -2,7 +2,7 @@ error: unused variable: `x`
   --> $DIR/issue-17999.rs:5:13
    |
 LL |         let x = (); //~ ERROR: unused variable: `x`
-   |             ^ help: consider using `_x` instead
+   |             ^ help: consider prefixing with an underscore: `_x`
    |
 note: lint level defined here
   --> $DIR/issue-17999.rs:1:9
@@ -14,7 +14,7 @@ error: unused variable: `a`
   --> $DIR/issue-17999.rs:7:13
    |
 LL |             a => {} //~ ERROR: unused variable: `a`
-   |             ^ help: consider using `_a` instead
+   |             ^ help: consider prefixing with an underscore: `_a`
 
 error: aborting due to 2 previous errors
 
diff --git a/src/test/ui/issues/issue-22599.stderr b/src/test/ui/issues/issue-22599.stderr
@@ -2,7 +2,7 @@ error: unused variable: `a`
   --> $DIR/issue-22599.rs:8:19
    |
 LL |     v = match 0 { a => 0 }; //~ ERROR: unused variable: `a`
-   |                   ^ help: consider using `_a` instead
+   |                   ^ help: consider prefixing with an underscore: `_a`
    |
 note: lint level defined here
   --> $DIR/issue-22599.rs:1:9

diff --git a/src/test/ui/issues/issue-56685.rs b/src/test/ui/issues/issue-56685.rs
@@ -0,0 +1,44 @@
+#![allow(dead_code)]
+#![deny(unused_variables)]
+
+// This test aims to check that unused variable suggestions update bindings in all
+// match arms.
+
+fn main() {
+    enum E {
+        A(i32,),
+        B(i32,),
+    }
+
+    match E::A(1) {
+        E::A(x) | E::B(x) => {}
+        //~^ ERROR unused variable: `x`
+    }
+
+    enum F {
+        A(i32, i32,),
+        B(i32, i32,),
+        C(i32, i32,),
+    }
+
+    let _ = match F::A(1, 2) {
+        F::A(x, y) | F::B(x, y) => { y },
+        //~^ ERROR unused variable: `x`
+        F::C(a, b) => { 3 }
+        //~^ ERROR unused variable: `a`
+        //~^^ ERROR unused variable: `b`
+    };
+
+    let _ = if let F::A(x, y) | F::B(x, y) = F::A(1, 2) {
+    //~^ ERROR unused variable: `x`
+        y
+    } else {
+        3
+    };
+
+    while let F::A(x, y) | F::B(x, y) = F::A(1, 2) {
+    //~^ ERROR unused variable: `x`
+        let _ = y;
+        break;
+    }
+}
diff --git a/src/test/ui/issues/issue-56685.stderr b/src/test/ui/issues/issue-56685.stderr
@@ -0,0 +1,60 @@
+error: unused variable: `x`
+  --> $DIR/issue-56685.rs:14:14
+   |
+LL |         E::A(x) | E::B(x) => {}
+   |              ^         ^
+   |
+note: lint level defined here
+  --> $DIR/issue-56685.rs:2:9
+   |
+LL | #![deny(unused_variables)]
+   |         ^^^^^^^^^^^^^^^^
+help: consider prefixing with an underscore
+   |
+LL |         E::A(_x) | E::B(_x) => {}
+   |              ^^         ^^
+
+error: unused variable: `x`
+  --> $DIR/issue-56685.rs:25:14
+   |
+LL |         F::A(x, y) | F::B(x, y) => { y },
+   |              ^            ^
+help: consider prefixing with an underscore
+   |
+LL |         F::A(_x, y) | F::B(_x, y) => { y },
+   |              ^^            ^^
+
+error: unused variable: `a`
+  --> $DIR/issue-56685.rs:27:14
+   |
+LL |         F::C(a, b) => { 3 }
+   |              ^ help: consider prefixing with an underscore: `_a`
+
+error: unused variable: `b`
+  --> $DIR/issue-56685.rs:27:17
+   |
+LL |         F::C(a, b) => { 3 }
+   |                 ^ help: consider prefixing with an underscore: `_b`
+
+error: unused variable: `x`
+  --> $DIR/issue-56685.rs:32:25
+   |
+LL |     let _ = if let F::A(x, y) | F::B(x, y) = F::A(1, 2) {
+   |                         ^            ^
+help: consider prefixing with an underscore
+   |
+LL |     let _ = if let F::A(_x, y) | F::B(_x, y) = F::A(1, 2) {
+   |                         ^^            ^^
+
+error: unused variable: `x`
+  --> $DIR/issue-56685.rs:39:20
+   |
+LL |     while let F::A(x, y) | F::B(x, y) = F::A(1, 2) {
+   |                    ^            ^
+help: consider prefixing with an underscore
+   |
+LL |     while let F::A(_x, y) | F::B(_x, y) = F::A(1, 2) {
+   |                    ^^            ^^
+
+error: aborting due to 6 previous errors
+
diff --git a/src/test/ui/lint/issue-47390-unused-variable-in-struct-pattern.stderr b/src/test/ui/lint/issue-47390-unused-variable-in-struct-pattern.stderr
@@ -2,7 +2,7 @@ warning: unused variable: `i_think_continually`
   --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:26:9
    |
 LL |     let i_think_continually = 2;
-   |         ^^^^^^^^^^^^^^^^^^^ help: consider using `_i_think_continually` instead
+   |         ^^^^^^^^^^^^^^^^^^^ help: consider prefixing with an underscore: `_i_think_continually`
    |
 note: lint level defined here
   --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:5:9
@@ -15,19 +15,19 @@ warning: unused variable: `mut_unused_var`
   --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:33:13
    |
 LL |     let mut mut_unused_var = 1;
-   |             ^^^^^^^^^^^^^^ help: consider using `_mut_unused_var` instead
+   |             ^^^^^^^^^^^^^^ help: consider prefixing with an underscore: `_mut_unused_var`
 
 warning: unused variable: `var`
   --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:35:14
    |
 LL |     let (mut var, unused_var) = (1, 2);
-   |              ^^^ help: consider using `_var` instead
+   |              ^^^ help: consider prefixing with an underscore: `_var`
 
 warning: unused variable: `unused_var`
   --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:35:19
    |
 LL |     let (mut var, unused_var) = (1, 2);
-   |                   ^^^^^^^^^^ help: consider using `_unused_var` instead
+   |                   ^^^^^^^^^^ help: consider prefixing with an underscore: `_unused_var`
 
 warning: unused variable: `corridors_of_light`
   --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:37:26

diff --git a/src/test/ui/lint/lint-removed-allow.stderr b/src/test/ui/lint/lint-removed-allow.stderr
@@ -2,7 +2,7 @@ error: unused variable: `unused`
   --> $DIR/lint-removed-allow.rs:8:17
    |
 LL | fn main() { let unused = (); } //~ ERROR unused
-   |                 ^^^^^^ help: consider using `_unused` instead
+   |                 ^^^^^^ help: consider prefixing with an underscore: `_unused`
    |
 note: lint level defined here
   --> $DIR/lint-removed-allow.rs:7:8

diff --git a/src/test/ui/lint/lint-removed-cmdline.stderr b/src/test/ui/lint/lint-removed-cmdline.stderr
@@ -6,7 +6,7 @@ error: unused variable: `unused`
   --> $DIR/lint-removed-cmdline.rs:12:17
    |
 LL | fn main() { let unused = (); }
-   |                 ^^^^^^ help: consider using `_unused` instead
+   |                 ^^^^^^ help: consider prefixing with an underscore: `_unused`
    |
 note: lint level defined here
   --> $DIR/lint-removed-cmdline.rs:11:8

diff --git a/src/test/ui/lint/lint-removed.stderr b/src/test/ui/lint/lint-removed.stderr
@@ -10,7 +10,7 @@ error: unused variable: `unused`
   --> $DIR/lint-removed.rs:8:17
    |
 LL | fn main() { let unused = (); } //~ ERROR unused
-   |                 ^^^^^^ help: consider using `_unused` instead
+   |                 ^^^^^^ help: consider prefixing with an underscore: `_unused`
    |
 note: lint level defined here
   --> $DIR/lint-removed.rs:7:8

diff --git a/src/test/ui/lint/lint-renamed-allow.stderr b/src/test/ui/lint/lint-renamed-allow.stderr
@@ -2,7 +2,7 @@ error: unused variable: `unused`
   --> $DIR/lint-renamed-allow.rs:8:17
    |
 LL | fn main() { let unused = (); } //~ ERROR unused
-   |                 ^^^^^^ help: consider using `_unused` instead
+   |                 ^^^^^^ help: consider prefixing with an underscore: `_unused`
    |
 note: lint level defined here
   --> $DIR/lint-renamed-allow.rs:7:8

diff --git a/src/test/ui/lint/lint-renamed-cmdline.stderr b/src/test/ui/lint/lint-renamed-cmdline.stderr
@@ -6,7 +6,7 @@ error: unused variable: `unused`
   --> $DIR/lint-renamed-cmdline.rs:8:17
    |
 LL | fn main() { let unused = (); }
-   |                 ^^^^^^ help: consider using `_unused` instead
+   |                 ^^^^^^ help: consider prefixing with an underscore: `_unused`
    |
 note: lint level defined here
   --> $DIR/lint-renamed-cmdline.rs:7:8

diff --git a/src/test/ui/lint/lint-renamed.stderr b/src/test/ui/lint/lint-renamed.stderr
@@ -10,7 +10,7 @@ error: unused variable: `unused`
   --> $DIR/lint-renamed.rs:4:17
    |
 LL | fn main() { let unused = (); } //~ ERROR unused
-   |                 ^^^^^^ help: consider using `_unused` instead
+   |                 ^^^^^^ help: consider prefixing with an underscore: `_unused`
    |
 note: lint level defined here
   --> $DIR/lint-renamed.rs:3:8

diff --git a/src/test/ui/lint/lint-uppercase-variables.stderr b/src/test/ui/lint/lint-uppercase-variables.stderr
@@ -8,7 +8,7 @@ warning: unused variable: `Foo`
   --> $DIR/lint-uppercase-variables.rs:22:9
    |
 LL |         Foo => {}
-   |         ^^^ help: consider using `_Foo` instead
+   |         ^^^ help: consider prefixing with an underscore: `_Foo`
    |
 note: lint level defined here
   --> $DIR/lint-uppercase-variables.rs:1:9