Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

correct unused field pattern suggestions #47922

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 49 additions & 14 deletions src/librustc/middle/liveness.rs
Original file line number Diff line number Diff line change
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,
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
4 changes: 3 additions & 1 deletion src/librustc/util/nodemap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,20 @@
#![allow(non_snake_case)]

use hir::def_id::DefId;
use hir::ItemLocalId;
use hir::{HirId, ItemLocalId};
use syntax::ast;

pub use rustc_data_structures::fx::FxHashMap;
pub use rustc_data_structures::fx::FxHashSet;

pub type NodeMap<T> = FxHashMap<ast::NodeId, T>;
pub type DefIdMap<T> = FxHashMap<DefId, T>;
pub type HirIdMap<T> = FxHashMap<HirId, T>;
pub type ItemLocalMap<T> = FxHashMap<ItemLocalId, T>;

pub type NodeSet = FxHashSet<ast::NodeId>;
pub type DefIdSet = FxHashSet<DefId>;
pub type HirIdSet = FxHashSet<HirId>;
pub type ItemLocalSet = FxHashSet<ItemLocalId>;

pub fn NodeMap<T>() -> NodeMap<T> { FxHashMap() }
Expand Down
34 changes: 34 additions & 0 deletions src/test/ui/lint/issue-47390-unused-variable-in-struct-pattern.rs
Original file line number Diff line number Diff line change
@@ -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;
}
}
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This warning is emitted somewhere else. Would you mind checking how hard it'd be to modify that note by checking if it is a field in a struct literal?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@estebank The PR touches this code (on master, the note starts with "to avoid this warning", which I thought was unnecessary). I had started working on making this a structured suggestion, too, but then backed off when I realized that getting the suggestion right would involve more refactoring than I wanted to do now. Notice that the span includes the mut: in the case of an entirely unused variable, that's fine (the mut is also unused, so replacing both the variable and any muts or refs with variable: _ is correct), but in the case of a mere unused assignment (where we mutate the variable, but don't do anything interesting with it), not only is the mut significant, but there are also other appearances of the variable (requiring a multi-span suggestion).


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
Original file line number Diff line number Diff line change
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