Skip to content

Commit

Permalink
concerning well-formed suggestions for unused shorthand field patterns
Browse files Browse the repository at this point in the history
Previously, unused variables would get a note that the warning could be
silenced by prefixing the variable with an underscore, but that doesn't
work for field shorthand patterns, which the liveness analysis didn't
know about.

The "to avoid this warning" verbiage seemed unnecessary.

Resolves #47390.
  • Loading branch information
zackmdavis committed Feb 1, 2018
1 parent 8f9d915 commit e4b1a79
Show file tree
Hide file tree
Showing 4 changed files with 124 additions and 16 deletions.
63 changes: 49 additions & 14 deletions src/librustc/middle/liveness.rs
Expand Up @@ -109,7 +109,7 @@ use self::VarKind::*;
use hir::def::*;
use ty::{self, TyCtxt};
use lint;
use util::nodemap::NodeMap;
use util::nodemap::{NodeMap, NodeSet};

use std::{fmt, usize};
use std::io::prelude::*;
Expand Down Expand Up @@ -244,7 +244,8 @@ struct CaptureInfo {
#[derive(Copy, Clone, Debug)]
struct LocalInfo {
id: NodeId,
name: ast::Name
name: ast::Name,
is_shorthand: bool,
}

#[derive(Copy, Clone, Debug)]
Expand Down Expand Up @@ -333,6 +334,13 @@ impl<'a, 'tcx> IrMaps<'a, 'tcx> {
}
}

fn variable_is_shorthand(&self, var: Variable) -> bool {
match self.var_kinds[var.get()] {
Local(LocalInfo { is_shorthand, .. }) => is_shorthand,
Arg(..) | CleanExit => false
}
}

fn set_captures(&mut self, node_id: NodeId, cs: Vec<CaptureInfo>) {
self.capture_info_map.insert(node_id, Rc::new(cs));
}
Expand Down Expand Up @@ -384,23 +392,41 @@ fn visit_local<'a, 'tcx>(ir: &mut IrMaps<'a, 'tcx>, local: &'tcx hir::Local) {
let name = path1.node;
ir.add_live_node_for_node(p_id, VarDefNode(sp));
ir.add_variable(Local(LocalInfo {
id: p_id,
name,
id: p_id,
name,
is_shorthand: false,
}));
});
intravisit::walk_local(ir, local);
}

fn visit_arm<'a, 'tcx>(ir: &mut IrMaps<'a, 'tcx>, arm: &'tcx hir::Arm) {
for 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 and
// librustc/README.md, `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();
if let hir::PatKind::Struct(_, ref fields, _) = pat.node {
for field in fields {
if field.node.is_shorthand {
shorthand_field_ids.insert(field.node.pat.id);
}
}
}

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(sp));
ir.add_variable(Local(LocalInfo {
id: p_id,
name,
name: name,

This comment has been minimized.

Copy link
@zackmdavis

zackmdavis Feb 7, 2018

Author Member

Oops, this is slightly ironic for a commit specifically about struct field shorthand ... 😞

is_shorthand: shorthand_field_ids.contains(&p_id)
}));
})
}
Expand Down Expand Up @@ -1483,17 +1509,26 @@ 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_node_note(lint::builtin::UNUSED_VARIABLES, id, sp,
&format!("variable `{}` is assigned to, but never used",
name),
&format!("to avoid this warning, consider using `_{}` instead",
name));
self.ir.tcx
.lint_node_note(lint::builtin::UNUSED_VARIABLES, id, sp,
&format!("variable `{}` is assigned to, but never used",
name),
&suggest_underscore_msg);
} else if name != "self" {
self.ir.tcx.lint_node_note(lint::builtin::UNUSED_VARIABLES, id, sp,
&format!("unused variable: `{}`", name),
&format!("to avoid this warning, consider using `_{}` instead",
name));
let msg = format!("unused variable: `{}`", name);
let mut err = self.ir.tcx
.struct_span_lint_node(lint::builtin::UNUSED_VARIABLES, id, sp, &msg);
if self.ir.variable_is_shorthand(var) {
err.span_suggestion(sp, "try ignoring the field",
format!("{}: _", name));
} else {
err.span_suggestion_short(sp, &suggest_underscore_msg,
format!("_{}", name));
}
err.emit()
}
}
true
Expand Down
34 changes: 34 additions & 0 deletions src/test/ui/lint/issue-47390-unused-variable-in-struct-pattern.rs
@@ -0,0 +1,34 @@
// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// must-compile-successfully

#![warn(unused)] // UI tests pass `-A unused` (#43896)

struct SoulHistory {
corridors_of_light: usize,
hours_are_suns: bool,
endless_and_singing: bool
}

fn main() {
let i_think_continually = 2;
let who_from_the_womb_remembered = SoulHistory {
corridors_of_light: 5,
hours_are_suns: true,
endless_and_singing: true
};

if let SoulHistory { corridors_of_light,
mut hours_are_suns,
endless_and_singing: true } = who_from_the_womb_remembered {
hours_are_suns = false;
}
}
@@ -0,0 +1,40 @@
warning: unused variable: `i_think_continually`
--> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:22:9
|
22 | let i_think_continually = 2;
| ^^^^^^^^^^^^^^^^^^^ help: consider using `_i_think_continually` instead
|
note: lint level defined here
--> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:13:9
|
13 | #![warn(unused)] // UI tests pass `-A unused` (#43896)
| ^^^^^^
= note: #[warn(unused_variables)] implied by #[warn(unused)]

warning: unused variable: `corridors_of_light`
--> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:29:26
|
29 | 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:30:26
|
30 | 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:32:9
|
32 | hours_are_suns = false;
| ^^^^^^^^^^^^^^
|
note: lint level defined here
--> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:13:9
|
13 | #![warn(unused)] // UI tests pass `-A unused` (#43896)
| ^^^^^^
= note: #[warn(unused_assignments)] implied by #[warn(unused)]

3 changes: 1 addition & 2 deletions src/test/ui/span/issue-24690.stderr
Expand Up @@ -2,15 +2,14 @@ warning: unused variable: `theOtherTwo`
--> $DIR/issue-24690.rs:23:9
|
23 | let theOtherTwo = 2; //~ WARN should have a snake case name
| ^^^^^^^^^^^
| ^^^^^^^^^^^ help: consider using `_theOtherTwo` instead
|
note: lint level defined here
--> $DIR/issue-24690.rs:18:9
|
18 | #![warn(unused)]
| ^^^^^^
= note: #[warn(unused_variables)] implied by #[warn(unused)]
= note: to avoid this warning, consider using `_theOtherTwo` instead

warning: variable `theTwo` should have a snake case name such as `the_two`
--> $DIR/issue-24690.rs:22:9
Expand Down

0 comments on commit e4b1a79

Please sign in to comment.