From 59782f4829db1afed5a3d50d03709711b8dd16e0 Mon Sep 17 00:00:00 2001 From: "Zack M. Davis" Date: Fri, 18 May 2018 00:07:31 -0700 Subject: [PATCH] in which the unused shorthand field pattern debacle/saga continues In e4b1a79 (#47922), we corrected erroneous suggestions for unused shorthand field pattern bindings, suggesting `field: _` where the previous suggestion of `_field` wouldn't even have compiled (#47390). Soon, it was revealed that this was insufficient (#50303), and the fix was extended to references, slices, &c. (#50327) But even this proved inadequate, as the erroneous suggestions were still being issued for patterns in local (`let`) bindings (#50804). Here, we yank the shorthand-detection and variable/node registration code into a new common function that can be called while visiting both match arms and `let` bindings. Resolves #50804. --- src/librustc/middle/liveness.rs | 106 +++++++++--------- ...47390-unused-variable-in-struct-pattern.rs | 9 ++ ...0-unused-variable-in-struct-pattern.stderr | 36 +++--- 3 files changed, 80 insertions(+), 71 deletions(-) diff --git a/src/librustc/middle/liveness.rs b/src/librustc/middle/liveness.rs index b39311a74718f..3db8c746713fd 100644 --- a/src/librustc/middle/liveness.rs +++ b/src/librustc/middle/liveness.rs @@ -117,6 +117,7 @@ use std::io::prelude::*; use std::io; use std::rc::Rc; use syntax::ast::{self, NodeId}; +use syntax::ptr::P; use syntax::symbol::keywords; use syntax_pos::Span; @@ -398,72 +399,65 @@ fn visit_fn<'a, 'tcx: 'a>(ir: &mut IrMaps<'a, 'tcx>, lsets.warn_about_unused_args(body, entry_ln); } -fn visit_local<'a, 'tcx>(ir: &mut IrMaps<'a, 'tcx>, local: &'tcx hir::Local) { - local.pat.each_binding(|_, p_id, sp, path1| { - debug!("adding local variable {}", p_id); +fn add_from_pat<'a, 'tcx>(ir: &mut IrMaps<'a, 'tcx>, pat: &P) { + // For struct patterns, take note of which fields used shorthand + // (`x` rather than `x: x`). + // + // FIXME: according to the rust-lang-nursery/rustc-guide book, `NodeId`s are to be + // phased out in favor of `HirId`s; however, we need to match the signature of + // `each_binding`, which uses `NodeIds`. + let mut shorthand_field_ids = NodeSet(); + let mut pats = VecDeque::new(); + pats.push_back(pat); + while let Some(pat) = pats.pop_front() { + use hir::PatKind::*; + match pat.node { + Binding(_, _, _, ref inner_pat) => { + pats.extend(inner_pat.iter()); + } + Struct(_, ref fields, _) => { + for field in fields { + if field.node.is_shorthand { + shorthand_field_ids.insert(field.node.pat.id); + } + } + } + Ref(ref inner_pat, _) | + Box(ref inner_pat) => { + pats.push_back(inner_pat); + } + TupleStruct(_, ref inner_pats, _) | + Tuple(ref inner_pats, _) => { + pats.extend(inner_pats.iter()); + } + Slice(ref pre_pats, ref inner_pat, ref post_pats) => { + pats.extend(pre_pats.iter()); + pats.extend(inner_pat.iter()); + pats.extend(post_pats.iter()); + } + _ => {} + } + } + + pat.each_binding(|_bm, p_id, _sp, path1| { let name = path1.node; - ir.add_live_node_for_node(p_id, VarDefNode(sp)); + ir.add_live_node_for_node(p_id, VarDefNode(path1.span)); ir.add_variable(Local(LocalInfo { id: p_id, name, - is_shorthand: false, + is_shorthand: shorthand_field_ids.contains(&p_id) })); }); +} + +fn visit_local<'a, 'tcx>(ir: &mut IrMaps<'a, 'tcx>, local: &'tcx hir::Local) { + add_from_pat(ir, &local.pat); intravisit::walk_local(ir, local); } fn visit_arm<'a, 'tcx>(ir: &mut IrMaps<'a, 'tcx>, arm: &'tcx hir::Arm) { - for mut pat in &arm.pats { - // For struct patterns, take note of which fields used shorthand - // (`x` rather than `x: x`). - // - // FIXME: according to the rust-lang-nursery/rustc-guide book, `NodeId`s are to be - // phased out in favor of `HirId`s; however, we need to match the signature of - // `each_binding`, which uses `NodeIds`. - let mut shorthand_field_ids = NodeSet(); - let mut pats = VecDeque::new(); - pats.push_back(pat); - while let Some(pat) = pats.pop_front() { - use hir::PatKind::*; - match pat.node { - Binding(_, _, _, ref inner_pat) => { - pats.extend(inner_pat.iter()); - } - Struct(_, ref fields, _) => { - for field in fields { - if field.node.is_shorthand { - shorthand_field_ids.insert(field.node.pat.id); - } - } - } - Ref(ref inner_pat, _) | - Box(ref inner_pat) => { - pats.push_back(inner_pat); - } - TupleStruct(_, ref inner_pats, _) | - Tuple(ref inner_pats, _) => { - pats.extend(inner_pats.iter()); - } - Slice(ref pre_pats, ref inner_pat, ref post_pats) => { - pats.extend(pre_pats.iter()); - pats.extend(inner_pat.iter()); - pats.extend(post_pats.iter()); - } - _ => {} - } - } - - pat.each_binding(|bm, p_id, _sp, path1| { - debug!("adding local variable {} from match with bm {:?}", - p_id, bm); - let name = path1.node; - ir.add_live_node_for_node(p_id, VarDefNode(path1.span)); - ir.add_variable(Local(LocalInfo { - id: p_id, - name: name, - is_shorthand: shorthand_field_ids.contains(&p_id) - })); - }) + for pat in &arm.pats { + add_from_pat(ir, pat); } intravisit::walk_arm(ir, arm); } diff --git a/src/test/ui/lint/issue-47390-unused-variable-in-struct-pattern.rs b/src/test/ui/lint/issue-47390-unused-variable-in-struct-pattern.rs index 100fb6d3533f5..bac3f00ffc79d 100644 --- a/src/test/ui/lint/issue-47390-unused-variable-in-struct-pattern.rs +++ b/src/test/ui/lint/issue-47390-unused-variable-in-struct-pattern.rs @@ -20,6 +20,11 @@ struct SoulHistory { endless_and_singing: bool } +struct LovelyAmbition { + lips: usize, + fire: usize +} + #[derive(Clone, Copy)] enum Large { Suit { case: () } @@ -45,6 +50,10 @@ fn main() { hours_are_suns = false; } + let the_spirit = LovelyAmbition { lips: 1, fire: 2 }; + let LovelyAmbition { lips, fire } = the_spirit; + println!("{}", lips); + let bag = Large::Suit { case: () }; 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 index 992be2c0a2844..a8b0e3e4250ea 100644 --- 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 @@ -1,5 +1,5 @@ warning: unused variable: `i_think_continually` - --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:31:9 + --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:36:9 | LL | let i_think_continually = 2; | ^^^^^^^^^^^^^^^^^^^ help: consider using `_i_think_continually` instead @@ -12,31 +12,31 @@ 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:38:13 + --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:43:13 | LL | let mut mut_unused_var = 1; | ^^^^^^^^^^^^^^ help: consider using `_mut_unused_var` instead warning: unused variable: `var` - --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:40:14 + --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:45:14 | LL | let (mut var, unused_var) = (1, 2); | ^^^ help: consider using `_var` instead warning: unused variable: `unused_var` - --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:40:19 + --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:45:19 | LL | let (mut var, unused_var) = (1, 2); | ^^^^^^^^^^ help: consider using `_unused_var` instead warning: unused variable: `corridors_of_light` - --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:42:26 + --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:47:26 | 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:43:30 + --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:48:30 | LL | mut hours_are_suns, | ^^^^^^^^^^^^^^ @@ -44,7 +44,7 @@ LL | mut hours_are_suns, = note: consider using `_hours_are_suns` instead warning: value assigned to `hours_are_suns` is never read - --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:45:9 + --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:50:9 | LL | hours_are_suns = false; | ^^^^^^^^^^^^^^ @@ -56,44 +56,50 @@ LL | #![warn(unused)] // UI tests pass `-A unused` (#43896) | ^^^^^^ = note: #[warn(unused_assignments)] implied by #[warn(unused)] +warning: unused variable: `fire` + --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:54:32 + | +LL | let LovelyAmbition { lips, fire } = the_spirit; + | ^^^^ help: try ignoring the field: `fire: _` + warning: unused variable: `case` - --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:54:23 + --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:63:23 | LL | Large::Suit { case } => {} | ^^^^ help: try ignoring the field: `case: _` warning: unused variable: `case` - --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:59:24 + --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:68:24 | LL | &Large::Suit { case } => {} | ^^^^ help: try ignoring the field: `case: _` warning: unused variable: `case` - --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:64:27 + --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:73:27 | LL | box Large::Suit { case } => {} | ^^^^ help: try ignoring the field: `case: _` warning: unused variable: `case` - --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:69:24 + --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:78:24 | LL | (Large::Suit { case },) => {} | ^^^^ help: try ignoring the field: `case: _` warning: unused variable: `case` - --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:74:24 + --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:83:24 | LL | [Large::Suit { case }] => {} | ^^^^ help: try ignoring the field: `case: _` warning: unused variable: `case` - --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:79:29 + --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:88:29 | LL | Tuple(Large::Suit { case }, ()) => {} | ^^^^ help: try ignoring the field: `case: _` warning: variable does not need to be mutable - --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:38:9 + --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:43:9 | LL | let mut mut_unused_var = 1; | ----^^^^^^^^^^^^^^ @@ -108,7 +114,7 @@ LL | #![warn(unused)] // UI tests pass `-A unused` (#43896) = note: #[warn(unused_mut)] implied by #[warn(unused)] warning: variable does not need to be mutable - --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:40:10 + --> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:45:10 | LL | let (mut var, unused_var) = (1, 2); | ----^^^