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

src/librustc_passes/liveness.rs

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

src/test/ui/lint/issue-47390-unused-variable-in-struct-pattern.stderr

@@ -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
@@ -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
 

src/test/ui/lint/issue-54180-unused-ref-field.stderr

@@ -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
@@ -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
 

src/test/ui/lint/issue-67691-unused-field-in-or-pattern.rs

@@ -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));
+}

src/test/ui/lint/issue-67691-unused-field-in-or-pattern.stderr

@@ -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
+

src/test/ui/liveness/liveness-dead.stderr

@@ -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
@@ -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?