Skip to content

also point to value in "value assigned is never read" lint #49197

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.

Sign up for GitHub

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

Closed
wants to merge 2 commits into from
Closed

also point to value in "value assigned is never read" lint #49197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
579004f
in which the liveness lint tests find a new home
zackmdavis Mar 20, 2018
60e2af0
also point to value in "value assigned is never read" lint
zackmdavis Mar 20, 2018
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
46 changes: 25 additions & 21 deletions src/librustc/middle/liveness.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ use std::io;
use std::rc::Rc;
use syntax::ast::{self, NodeId};
use syntax::symbol::keywords;
use syntax_pos::Span;
use syntax_pos::{Span, MultiSpan};

use hir::Expr;
use hir;
Expand Down Expand Up @@ -1363,8 +1363,8 @@ impl<'a, 'tcx> Visitor<'tcx> for Liveness<'a, 'tcx> {

fn check_local<'a, 'tcx>(this: &mut Liveness<'a, 'tcx>, local: &'tcx hir::Local) {
match local.init {
Some(_) => {
this.warn_about_unused_or_dead_vars_in_pat(&local.pat);
Some(ref init_expr) => {
this.warn_about_unused_or_dead_vars_in_pat(&local.pat, init_expr.span);
},
None => {
this.pat_bindings(&local.pat, |this, ln, var, sp, id| {
Expand All @@ -1388,19 +1388,21 @@ fn check_arm<'a, 'tcx>(this: &mut Liveness<'a, 'tcx>, arm: &'tcx hir::Arm) {

fn check_expr<'a, 'tcx>(this: &mut Liveness<'a, 'tcx>, expr: &'tcx Expr) {
match expr.node {
hir::ExprAssign(ref l, _) => {
this.check_place(&l);
hir::ExprAssign(ref l, ref r) => {
let msp = MultiSpan::from_spans(vec![l.span, r.span]);
this.check_place(&l, msp);

intravisit::walk_expr(this, expr);
}

hir::ExprAssignOp(_, ref l, _) => {
if !this.tables.is_method_call(expr) {
this.check_place(&l);
intravisit::walk_expr(this, expr);
}

intravisit::walk_expr(this, expr);
}
hir::ExprAssignOp(_, ref l, ref r) => {
if !this.tables.is_method_call(expr) {
let msp = MultiSpan::from_spans(vec![l.span, r.span]);
this.check_place(&l, msp);
}

intravisit::walk_expr(this, expr);
}

hir::ExprInlineAsm(ref ia, ref outputs, ref inputs) => {
for input in inputs {
Expand All @@ -1410,7 +1412,7 @@ fn check_expr<'a, 'tcx>(this: &mut Liveness<'a, 'tcx>, expr: &'tcx Expr) {
// Output operands must be places
for (o, output) in ia.outputs.iter().zip(outputs) {
if !o.is_indirect {
this.check_place(output);
this.check_place(output, MultiSpan::from_span(output.span));
}
this.visit_expr(output);
}
Expand All @@ -1435,7 +1437,7 @@ fn check_expr<'a, 'tcx>(this: &mut Liveness<'a, 'tcx>, expr: &'tcx Expr) {
}

impl<'a, 'tcx> Liveness<'a, 'tcx> {
fn check_place(&mut self, expr: &'tcx Expr) {
fn check_place(&mut self, expr: &'tcx Expr, msp: MultiSpan) {
match expr.node {
hir::ExprPath(hir::QPath::Resolved(_, ref path)) => {
if let Def::Local(nid) = path.def {
Expand All @@ -1445,7 +1447,7 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> {
// as being used.
let ln = self.live_node(expr.id, expr.span);
let var = self.variable(nid, expr.span);
self.warn_about_dead_assign(expr.span, expr.id, ln, var);
self.warn_about_dead_assign(msp, expr.id, ln, var);
}
}
_ => {
Expand Down Expand Up @@ -1482,10 +1484,11 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> {
}
}

fn warn_about_unused_or_dead_vars_in_pat(&mut self, pat: &hir::Pat) {
fn warn_about_unused_or_dead_vars_in_pat(&mut self, pat: &hir::Pat, assign_span: Span) {
self.pat_bindings(pat, |this, ln, var, sp, id| {
if !this.warn_about_unused(sp, id, ln, var) {
this.warn_about_dead_assign(sp, id, ln, var);
let msp = MultiSpan::from_spans(vec![sp, assign_span]);
this.warn_about_dead_assign(msp, id, ln, var);
}
})
}
Expand Down Expand Up @@ -1538,16 +1541,17 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> {
}

fn warn_about_dead_assign(&self,
sp: Span,
msp: MultiSpan,
id: NodeId,
ln: LiveNode,
var: Variable) {
if self.live_on_exit(ln, var).is_none() {
self.report_dead_assign(id, sp, var, false);
self.report_dead_assign(id, msp, var, false);
}
}

fn report_dead_assign(&self, id: NodeId, sp: Span, var: Variable, is_argument: bool) {
fn report_dead_assign<S: Into<MultiSpan>>(&self, id: NodeId, sp: S,
var: Variable, is_argument: bool) {
if let Some(name) = self.should_warn(var) {
if is_argument {
self.ir.tcx.lint_node(lint::builtin::UNUSED_ASSIGNMENTS, id, sp,
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/lint/issue-47390-unused-variable-in-struct-pattern.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ warning: value assigned to `hours_are_suns` is never read
--> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:32:9
|
LL | hours_are_suns = false;
| ^^^^^^^^^^^^^^
| ^^^^^^^^^^^^^^ ^^^^^
|
note: lint level defined here
--> $DIR/issue-47390-unused-variable-in-struct-pattern.rs:13:9
Expand Down
12 changes: 7 additions & 5 deletions src/test/compile-fail/liveness-dead.rs → src/test/ui/lint/liveness-dead.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,33 +8,35 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// must-compile-successfully

#![allow(dead_code)]
#![deny(unused_assignments)]
#![warn(unused_assignments)]

fn f1(x: &mut isize) {
*x = 1; // no error
}

fn f2() {
let mut x: isize = 3; //~ ERROR: value assigned to `x` is never read
let (mut x, y) = (3, 4); //~ WARN: value assigned to `x` is never read
x = 4;
x.clone();
}

fn f3() {
let mut x: isize = 3;
x.clone();
x = 4; //~ ERROR: value assigned to `x` is never read
x = 4; //~ WARN: value assigned to `x` is never read
}

fn f4(mut x: i32) { //~ ERROR: value passed to `x` is never read
fn f4(mut x: i32) { //~ WARN: value passed to `x` is never read
x = 4;
x.clone();
}

fn f5(mut x: i32) {
x.clone();
x = 4; //~ ERROR: value assigned to `x` is never read
x = 4; //~ WARN: value assigned to `x` is never read
}

fn main() {}
30 changes: 30 additions & 0 deletions src/test/ui/lint/liveness-dead.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
warning: value assigned to `x` is never read
--> $DIR/liveness-dead.rs:21:10
|
LL | let (mut x, y) = (3, 4); //~ WARN: value assigned to `x` is never read
| ^^^^^ ^^^^^^
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. The second span isn't correct, as it should be pointing only to 3, not the entire tuple

  2. It doesn't seem to be as useful in this case, I feel that pointing to the reassignment of x in line 22 would be better instead:

warning: value assigned to `x` is never read
  --> $DIR/liveness-dead.rs:21:10
   |
LL |     let (mut x, y) = (3, 4);
   |          ^^^^^        - this value is never read...
LL |     x = 4;
   |     - ...because its binding is reassigned here before being used
  1. I would prefer if the second span where a secondary span (blue), instead of both spans being primary spans (red).

Copy link
Contributor

Choose a reason for hiding this comment

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

It's going to be fairly hard (and not always possible) to highlight the value in a case like that. Consider let (x, y) = pair; But I do like the second label! Right now I don't think we have that information though. (Grr, we should rewrite liveness in terms of MIR)

Copy link
Member Author

Choose a reason for hiding this comment

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

The language value from here is never read rather than this value... may make up for our inability to get a variable-specific span on the right hand side. (Ordinarily I'd prefer to avoid "here" language for the same reason it should be avoided in hyperlinks, but "value from assignment" feels less clear?)

|
note: lint level defined here
--> $DIR/liveness-dead.rs:14:9
|
LL | #![warn(unused_assignments)]
| ^^^^^^^^^^^^^^^^^^

warning: value assigned to `x` is never read
--> $DIR/liveness-dead.rs:29:5
|
LL | x = 4; //~ WARN: value assigned to `x` is never read
| ^ ^

warning: value passed to `x` is never read
--> $DIR/liveness-dead.rs:32:7
|
LL | fn f4(mut x: i32) { //~ WARN: value passed to `x` is never read
| ^^^^^

warning: value assigned to `x` is never read
--> $DIR/liveness-dead.rs:39:5
|
LL | x = 4; //~ WARN: value assigned to `x` is never read
| ^ ^

34 changes: 17 additions & 17 deletions src/test/compile-fail/liveness-unused.rs → src/test/ui/lint/liveness-unused.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,44 +8,44 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![warn(unused)]
#![deny(unused_variables)]
#![deny(unused_assignments)]
// must-compile-successfully

#![warn(unused_variables, unused_assignments, unreachable_code)]
#![allow(dead_code, non_camel_case_types, trivial_numeric_casts)]

use std::ops::AddAssign;

fn f1(x: isize) {
//~^ ERROR unused variable: `x`
//~^ WARN unused variable: `x`
}

fn f1b(x: &mut isize) {
//~^ ERROR unused variable: `x`
//~^ WARN unused variable: `x`
}

#[allow(unused_variables)]
fn f1c(x: isize) {}

fn f1d() {
let x: isize;
//~^ ERROR unused variable: `x`
//~^ WARN unused variable: `x`
}

fn f2() {
let x = 3;
//~^ ERROR unused variable: `x`
//~^ WARN unused variable: `x`
}

fn f3() {
let mut x = 3;
//~^ ERROR variable `x` is assigned to, but never used
//~^ WARN variable `x` is assigned to, but never used
x += 4;
//~^ ERROR value assigned to `x` is never read
//~^ WARN value assigned to `x` is never read
}

fn f3b() {
let mut z = 3;
//~^ ERROR variable `z` is assigned to, but never used
//~^ WARN variable `z` is assigned to, but never used
loop {
z += 4;
}
Expand All @@ -67,7 +67,7 @@ fn f3d() {
fn f4() {
match Some(3) {
Some(i) => {
//~^ ERROR unused variable: `i`
//~^ WARN unused variable: `i`
}
None => {}
}
Expand All @@ -87,19 +87,19 @@ fn f4b() -> isize {

fn f5a() {
for x in 1..10 { }
//~^ ERROR unused variable: `x`
//~^ WARN unused variable: `x`
}

fn f5b() {
for (x, _) in [1, 2, 3].iter().enumerate() { }
//~^ ERROR unused variable: `x`
//~^ WARN unused variable: `x`
}

fn f5c() {
for (_, x) in [1, 2, 3].iter().enumerate() {
//~^ ERROR unused variable: `x`
//~^ WARN unused variable: `x`
continue;
drop(*x as i32); //~ WARNING unreachable statement
drop(*x as i32); //~ WARN unreachable statement
}
}

Expand All @@ -120,10 +120,10 @@ fn f6() {
// ensure an error shows up for x even if lhs of an overloaded add assign

let x;
//~^ ERROR variable `x` is assigned to, but never used
//~^ WARN variable `x` is assigned to, but never used

*({
x = 0; //~ ERROR value assigned to `x` is never read
x = 0; //~ WARN value assigned to `x` is never read
&mut v
}) += 1;
}
Expand Down
Loading