diff --git a/src/librustc_passes/liveness.rs b/src/librustc_passes/liveness.rs index 00dc8a41350b3..55978afc59437 100644 --- a/src/librustc_passes/liveness.rs +++ b/src/librustc_passes/liveness.rs @@ -76,13 +76,10 @@ //! is not just used to generate a new value. For example, `x += 1` is //! a read but not a use. This is used to generate better warnings. //! -//! ## Special Variables +//! ## Special nodes and variables //! -//! We generate various special variables for various, well, special purposes. -//! These are described in the `specials` struct: -//! -//! - `exit_ln`: a live node that is generated to represent every 'exit' from -//! the function, whether it be by explicit return, panic, or other means. +//! We generate various special nodes for various, well, special purposes. +//! These are described in the `Specials` struct. use self::LiveNodeKind::*; use self::VarKind::*; @@ -131,6 +128,7 @@ enum LiveNodeKind { UpvarNode(Span), ExprNode(Span), VarDefNode(Span), + ClosureNode, ExitNode, } @@ -140,6 +138,7 @@ fn live_node_kind_to_string(lnk: LiveNodeKind, tcx: TyCtxt<'_>) -> String { UpvarNode(s) => format!("Upvar node [{}]", sm.span_to_string(s)), ExprNode(s) => format!("Expr node [{}]", sm.span_to_string(s)), VarDefNode(s) => format!("Var def node [{}]", sm.span_to_string(s)), + ClosureNode => "Closure node".to_owned(), ExitNode => "Exit node".to_owned(), } } @@ -396,10 +395,12 @@ fn visit_fn<'tcx>( // compute liveness let mut lsets = Liveness::new(&mut fn_maps, def_id); - let entry_ln = lsets.compute(&body.value); + let entry_ln = lsets.compute(fk, &body, sp, id); + lsets.log_liveness(entry_ln, id); // check for various error conditions lsets.visit_body(body); + lsets.warn_about_unused_upvars(entry_ln); lsets.warn_about_unused_args(body, entry_ln); } @@ -634,6 +635,12 @@ impl RWUTable { #[derive(Copy, Clone)] struct Specials { + /// A live node representing a point of execution before closure entry & + /// after closure exit. Used to calculate liveness of captured variables + /// through calls to the same closure. Used for Fn & FnMut closures only. + closure_ln: LiveNode, + /// A live node representing every 'exit' from the function, whether it be + /// by explicit return, panic, or other means. exit_ln: LiveNode, } @@ -658,11 +665,8 @@ struct Liveness<'a, 'tcx> { impl<'a, 'tcx> Liveness<'a, 'tcx> { fn new(ir: &'a mut IrMaps<'tcx>, def_id: LocalDefId) -> Liveness<'a, 'tcx> { - // Special nodes and variables: - // - exit_ln represents the end of the fn, either by return or panic - // - implicit_ret_var is a pseudo-variable that represents - // an implicit return let specials = Specials { + closure_ln: ir.add_live_node(ClosureNode), exit_ln: ir.add_live_node(ExitNode), }; @@ -789,6 +793,20 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> { String::from_utf8(wr).unwrap() } + fn log_liveness(&self, entry_ln: LiveNode, hir_id: hir::HirId) { + // hack to skip the loop unless debug! is enabled: + debug!( + "^^ liveness computation results for body {} (entry={:?})", + { + for ln_idx in 0..self.ir.num_live_nodes { + debug!("{:?}", self.ln_str(LiveNode(ln_idx as u32))); + } + hir_id + }, + entry_ln + ); + } + fn init_empty(&mut self, ln: LiveNode, succ_ln: LiveNode) { self.successors[ln.get()] = succ_ln; @@ -889,33 +907,87 @@ impl<'a, 'tcx> Liveness<'a, 'tcx> { self.rwu_table.assign_unpacked(idx, rwu); } - fn compute(&mut self, body: &hir::Expr<'_>) -> LiveNode { - debug!("compute: using id for body, {:?}", body); + fn compute( + &mut self, + fk: FnKind<'_>, + body: &hir::Body<'_>, + span: Span, + id: hir::HirId, + ) -> LiveNode { + debug!("compute: using id for body, {:?}", body.value); - let s = self.s; + // # Liveness of captured variables + // + // When computing the liveness for captured variables we take into + // account how variable is captured (ByRef vs ByValue) and what is the + // closure kind (Generator / FnOnce vs Fn / FnMut). + // + // Variables captured by reference are assumed to be used on the exit + // from the closure. + // + // In FnOnce closures, variables captured by value are known to be dead + // on exit since it is impossible to call the closure again. + // + // In Fn / FnMut closures, variables captured by value are live on exit + // if they are live on the entry to the closure, since only the closure + // itself can access them on subsequent calls. if let Some(upvars) = self.ir.tcx.upvars_mentioned(self.ir.body_owner) { + // Mark upvars captured by reference as used after closure exits. for (&var_hir_id, upvar) in upvars.iter().rev() { - let var = self.variable(var_hir_id, upvar.span); - self.acc(s.exit_ln, var, ACC_READ | ACC_USE); + let upvar_id = ty::UpvarId { + var_path: ty::UpvarPath { hir_id: var_hir_id }, + closure_expr_id: self.ir.body_owner.expect_local(), + }; + match self.tables.upvar_capture(upvar_id) { + ty::UpvarCapture::ByRef(_) => { + let var = self.variable(var_hir_id, upvar.span); + self.acc(self.s.exit_ln, var, ACC_READ | ACC_USE); + } + ty::UpvarCapture::ByValue => {} + } } } - let entry_ln = self.propagate_through_expr(body, s.exit_ln); + let succ = self.propagate_through_expr(&body.value, self.s.exit_ln); - // hack to skip the loop unless debug! is enabled: - debug!( - "^^ liveness computation results for body {} (entry={:?})", - { - for ln_idx in 0..self.ir.num_live_nodes { - debug!("{:?}", self.ln_str(LiveNode(ln_idx as u32))); - } - body.hir_id + match fk { + FnKind::Method(..) | FnKind::ItemFn(..) => return succ, + FnKind::Closure(..) => {} + } + + let ty = self.tables.node_type(id); + match ty.kind { + ty::Closure(_def_id, substs) => match substs.as_closure().kind() { + ty::ClosureKind::Fn => {} + ty::ClosureKind::FnMut => {} + ty::ClosureKind::FnOnce => return succ, }, - entry_ln - ); + ty::Generator(..) => return succ, + _ => { + span_bug!(span, "type of closure expr {:?} is not a closure {:?}", id, ty,); + } + }; - entry_ln + // Propagate through calls to the closure. + let mut first_merge = true; + loop { + self.init_from_succ(self.s.closure_ln, succ); + for param in body.params { + param.pat.each_binding(|_bm, hir_id, _x, ident| { + let var = self.variable(hir_id, ident.span); + self.define(self.s.closure_ln, var); + }) + } + + if !self.merge_from_succ(self.s.exit_ln, self.s.closure_ln, first_merge) { + break; + } + first_merge = false; + assert_eq!(succ, self.propagate_through_expr(&body.value, self.s.exit_ln)); + } + + succ } fn propagate_through_block(&mut self, blk: &hir::Block<'_>, succ: LiveNode) -> LiveNode { @@ -1533,11 +1605,60 @@ impl<'tcx> Liveness<'_, 'tcx> { if name.is_empty() || name.as_bytes()[0] == b'_' { None } else { Some(name) } } + fn warn_about_unused_upvars(&self, entry_ln: LiveNode) { + let upvars = match self.ir.tcx.upvars_mentioned(self.ir.body_owner) { + None => return, + Some(upvars) => upvars, + }; + for (&var_hir_id, upvar) in upvars.iter() { + let var = self.variable(var_hir_id, upvar.span); + let upvar_id = ty::UpvarId { + var_path: ty::UpvarPath { hir_id: var_hir_id }, + closure_expr_id: self.ir.body_owner.expect_local(), + }; + match self.tables.upvar_capture(upvar_id) { + ty::UpvarCapture::ByValue => {} + ty::UpvarCapture::ByRef(..) => continue, + }; + if self.used_on_entry(entry_ln, var) { + if self.live_on_entry(entry_ln, var).is_none() { + if let Some(name) = self.should_warn(var) { + self.ir.tcx.struct_span_lint_hir( + lint::builtin::UNUSED_ASSIGNMENTS, + var_hir_id, + vec![upvar.span], + |lint| { + lint.build(&format!("value captured by `{}` is never read", name)) + .help("did you mean to capture by reference instead?") + .emit(); + }, + ); + } + } + } else { + if let Some(name) = self.should_warn(var) { + self.ir.tcx.struct_span_lint_hir( + lint::builtin::UNUSED_VARIABLES, + var_hir_id, + vec![upvar.span], + |lint| { + lint.build(&format!("unused variable: `{}`", name)) + .help("did you mean to capture by reference instead?") + .emit(); + }, + ); + } + } + } + } + fn warn_about_unused_args(&self, body: &hir::Body<'_>, entry_ln: LiveNode) { for p in body.params { self.check_unused_vars_in_pat(&p.pat, Some(entry_ln), |spans, hir_id, ln, var| { if self.live_on_entry(ln, var).is_none() { - self.report_dead_assign(hir_id, spans, var, true); + self.report_unsed_assign(hir_id, spans, var, |name| { + format!("value passed to `{}` is never read", name) + }); } }); } @@ -1651,35 +1772,30 @@ impl<'tcx> Liveness<'_, 'tcx> { fn warn_about_dead_assign(&self, spans: Vec, hir_id: HirId, ln: LiveNode, var: Variable) { if self.live_on_exit(ln, var).is_none() { - self.report_dead_assign(hir_id, spans, var, false); + self.report_unsed_assign(hir_id, spans, var, |name| { + format!("value assigned to `{}` is never read", name) + }); } } - fn report_dead_assign(&self, hir_id: HirId, spans: Vec, var: Variable, is_param: bool) { + fn report_unsed_assign( + &self, + hir_id: HirId, + spans: Vec, + var: Variable, + message: impl Fn(&str) -> String, + ) { if let Some(name) = self.should_warn(var) { - if is_param { - self.ir.tcx.struct_span_lint_hir( - lint::builtin::UNUSED_ASSIGNMENTS, - hir_id, - spans, - |lint| { - lint.build(&format!("value passed to `{}` is never read", name)) - .help("maybe it is overwritten before being read?") - .emit(); - }, - ) - } else { - self.ir.tcx.struct_span_lint_hir( - lint::builtin::UNUSED_ASSIGNMENTS, - hir_id, - spans, - |lint| { - lint.build(&format!("value assigned to `{}` is never read", name)) - .help("maybe it is overwritten before being read?") - .emit(); - }, - ) - } + self.ir.tcx.struct_span_lint_hir( + lint::builtin::UNUSED_ASSIGNMENTS, + hir_id, + spans, + |lint| { + lint.build(&message(&name)) + .help("maybe it is overwritten before being read?") + .emit(); + }, + ) } } } diff --git a/src/test/ui/closures/closure-immutable-outer-variable.fixed b/src/test/ui/closures/closure-immutable-outer-variable.fixed index 102f1f94a36e1..1b0feede34ecf 100644 --- a/src/test/ui/closures/closure-immutable-outer-variable.fixed +++ b/src/test/ui/closures/closure-immutable-outer-variable.fixed @@ -8,6 +8,6 @@ fn foo(mut f: Box) { fn main() { let mut y = true; - foo(Box::new(move || y = false) as Box<_>); + foo(Box::new(move || y = !y) as Box<_>); //~^ ERROR cannot assign to `y`, as it is not declared as mutable } diff --git a/src/test/ui/closures/closure-immutable-outer-variable.rs b/src/test/ui/closures/closure-immutable-outer-variable.rs index 6eb43b372c96c..50ec1c6148a04 100644 --- a/src/test/ui/closures/closure-immutable-outer-variable.rs +++ b/src/test/ui/closures/closure-immutable-outer-variable.rs @@ -8,6 +8,6 @@ fn foo(mut f: Box) { fn main() { let y = true; - foo(Box::new(move || y = false) as Box<_>); + foo(Box::new(move || y = !y) as Box<_>); //~^ ERROR cannot assign to `y`, as it is not declared as mutable } diff --git a/src/test/ui/closures/closure-immutable-outer-variable.stderr b/src/test/ui/closures/closure-immutable-outer-variable.stderr index 7e60f3cd8ffa4..799097889cd30 100644 --- a/src/test/ui/closures/closure-immutable-outer-variable.stderr +++ b/src/test/ui/closures/closure-immutable-outer-variable.stderr @@ -3,8 +3,8 @@ error[E0594]: cannot assign to `y`, as it is not declared as mutable | LL | let y = true; | - help: consider changing this to be mutable: `mut y` -LL | foo(Box::new(move || y = false) as Box<_>); - | ^^^^^^^^^ cannot assign +LL | foo(Box::new(move || y = !y) as Box<_>); + | ^^^^^^ cannot assign error: aborting due to previous error diff --git a/src/test/ui/issues/issue-11958.rs b/src/test/ui/issues/issue-11958.rs index 8fe8a8c606189..a7af01e25b4e2 100644 --- a/src/test/ui/issues/issue-11958.rs +++ b/src/test/ui/issues/issue-11958.rs @@ -1,5 +1,4 @@ // run-pass -#![forbid(warnings)] // We shouldn't need to rebind a moved upvar as mut if it's already // marked as mut @@ -7,4 +6,6 @@ pub fn main() { let mut x = 1; let _thunk = Box::new(move|| { x = 2; }); + //~^ WARN value assigned to `x` is never read + //~| WARN unused variable: `x` } diff --git a/src/test/ui/issues/issue-11958.stderr b/src/test/ui/issues/issue-11958.stderr new file mode 100644 index 0000000000000..25de6ff4c118c --- /dev/null +++ b/src/test/ui/issues/issue-11958.stderr @@ -0,0 +1,20 @@ +warning: value assigned to `x` is never read + --> $DIR/issue-11958.rs:8:36 + | +LL | let _thunk = Box::new(move|| { x = 2; }); + | ^ + | + = note: `#[warn(unused_assignments)]` on by default + = help: maybe it is overwritten before being read? + +warning: unused variable: `x` + --> $DIR/issue-11958.rs:8:36 + | +LL | let _thunk = Box::new(move|| { x = 2; }); + | ^ + | + = note: `#[warn(unused_variables)]` on by default + = help: did you mean to capture by reference instead? + +warning: 2 warnings emitted + diff --git a/src/test/ui/liveness/liveness-upvars.rs b/src/test/ui/liveness/liveness-upvars.rs new file mode 100644 index 0000000000000..b2837e74b8c51 --- /dev/null +++ b/src/test/ui/liveness/liveness-upvars.rs @@ -0,0 +1,108 @@ +// edition:2018 +// check-pass +#![warn(unused)] +#![allow(unreachable_code)] + +pub fn unintentional_copy_one() { + let mut last = None; + let mut f = move |s| { + last = Some(s); //~ WARN value assigned to `last` is never read + //~| WARN unused variable: `last` + }; + f("a"); + f("b"); + f("c"); + dbg!(last.unwrap()); +} + +pub fn unintentional_copy_two() { + let mut sum = 0; + (1..10).for_each(move |x| { + sum += x; //~ WARN unused variable: `sum` + }); + dbg!(sum); +} + +pub fn f() { + let mut c = 0; + + // Captured by value, but variable is dead on entry. + move || { + c = 1; //~ WARN value captured by `c` is never read + println!("{}", c); + }; + let _ = async move { + c = 1; //~ WARN value captured by `c` is never read + println!("{}", c); + }; + + // Read and written to, but never actually used. + move || { + c += 1; //~ WARN unused variable: `c` + }; + let _ = async move { + c += 1; //~ WARN value assigned to `c` is never read + //~| WARN unused variable: `c` + }; + + move || { + println!("{}", c); + // Value is read by closure itself on later invocations. + c += 1; + }; + let b = Box::new(42); + move || { + println!("{}", c); + // Never read because this is FnOnce closure. + c += 1; //~ WARN value assigned to `c` is never read + drop(b); + }; + let _ = async move { + println!("{}", c); + // Never read because this is a generator. + c += 1; //~ WARN value assigned to `c` is never read + }; +} + +pub fn nested() { + let mut d = None; + let mut e = None; + || { + || { + d = Some("d1"); //~ WARN value assigned to `d` is never read + d = Some("d2"); + }; + move || { + e = Some("e1"); //~ WARN value assigned to `e` is never read + //~| WARN unused variable: `e` + e = Some("e2"); //~ WARN value assigned to `e` is never read + }; + }; +} + +pub fn g(mut v: T) { + |r| { + if r { + v = T::default(); //~ WARN value assigned to `v` is never read + } else { + drop(v); + } + }; +} + +pub fn h() { + let mut z = T::default(); + move |b| { + loop { + if b { + z = T::default(); //~ WARN value assigned to `z` is never read + //~| WARN unused variable: `z` + } else { + return; + } + } + dbg!(z); + }; +} + +fn main() {} diff --git a/src/test/ui/liveness/liveness-upvars.stderr b/src/test/ui/liveness/liveness-upvars.stderr new file mode 100644 index 0000000000000..14fed91786436 --- /dev/null +++ b/src/test/ui/liveness/liveness-upvars.stderr @@ -0,0 +1,150 @@ +warning: value assigned to `last` is never read + --> $DIR/liveness-upvars.rs:9:9 + | +LL | last = Some(s); + | ^^^^ + | +note: the lint level is defined here + --> $DIR/liveness-upvars.rs:3:9 + | +LL | #![warn(unused)] + | ^^^^^^ + = note: `#[warn(unused_assignments)]` implied by `#[warn(unused)]` + = help: maybe it is overwritten before being read? + +warning: unused variable: `last` + --> $DIR/liveness-upvars.rs:9:9 + | +LL | last = Some(s); + | ^^^^ + | +note: the lint level is defined here + --> $DIR/liveness-upvars.rs:3:9 + | +LL | #![warn(unused)] + | ^^^^^^ + = note: `#[warn(unused_variables)]` implied by `#[warn(unused)]` + = help: did you mean to capture by reference instead? + +warning: unused variable: `sum` + --> $DIR/liveness-upvars.rs:21:9 + | +LL | sum += x; + | ^^^ + | + = help: did you mean to capture by reference instead? + +warning: value captured by `c` is never read + --> $DIR/liveness-upvars.rs:31:9 + | +LL | c = 1; + | ^ + | + = help: did you mean to capture by reference instead? + +warning: value captured by `c` is never read + --> $DIR/liveness-upvars.rs:35:9 + | +LL | c = 1; + | ^ + | + = help: did you mean to capture by reference instead? + +warning: unused variable: `c` + --> $DIR/liveness-upvars.rs:41:9 + | +LL | c += 1; + | ^ + | + = help: did you mean to capture by reference instead? + +warning: value assigned to `c` is never read + --> $DIR/liveness-upvars.rs:44:9 + | +LL | c += 1; + | ^ + | + = help: maybe it is overwritten before being read? + +warning: unused variable: `c` + --> $DIR/liveness-upvars.rs:44:9 + | +LL | c += 1; + | ^ + | + = help: did you mean to capture by reference instead? + +warning: value assigned to `c` is never read + --> $DIR/liveness-upvars.rs:57:9 + | +LL | c += 1; + | ^ + | + = help: maybe it is overwritten before being read? + +warning: value assigned to `c` is never read + --> $DIR/liveness-upvars.rs:63:9 + | +LL | c += 1; + | ^ + | + = help: maybe it is overwritten before being read? + +warning: value assigned to `d` is never read + --> $DIR/liveness-upvars.rs:72:13 + | +LL | d = Some("d1"); + | ^ + | + = help: maybe it is overwritten before being read? + +warning: value assigned to `e` is never read + --> $DIR/liveness-upvars.rs:76:13 + | +LL | e = Some("e1"); + | ^ + | + = help: maybe it is overwritten before being read? + +warning: value assigned to `e` is never read + --> $DIR/liveness-upvars.rs:78:13 + | +LL | e = Some("e2"); + | ^ + | + = help: maybe it is overwritten before being read? + +warning: unused variable: `e` + --> $DIR/liveness-upvars.rs:76:13 + | +LL | e = Some("e1"); + | ^ + | + = help: did you mean to capture by reference instead? + +warning: value assigned to `v` is never read + --> $DIR/liveness-upvars.rs:86:13 + | +LL | v = T::default(); + | ^ + | + = help: maybe it is overwritten before being read? + +warning: value assigned to `z` is never read + --> $DIR/liveness-upvars.rs:98:17 + | +LL | z = T::default(); + | ^ + | + = help: maybe it is overwritten before being read? + +warning: unused variable: `z` + --> $DIR/liveness-upvars.rs:98:17 + | +LL | z = T::default(); + | ^ + | + = help: did you mean to capture by reference instead? + +warning: 17 warnings emitted + diff --git a/src/test/ui/unboxed-closures/unboxed-closures-counter-not-moved.rs b/src/test/ui/unboxed-closures/unboxed-closures-counter-not-moved.rs index fb24df3c24e87..390386e57fa72 100644 --- a/src/test/ui/unboxed-closures/unboxed-closures-counter-not-moved.rs +++ b/src/test/ui/unboxed-closures/unboxed-closures-counter-not-moved.rs @@ -1,5 +1,4 @@ // run-pass -#![allow(unused_variables)] // Test that we mutate a counter on the stack only when we expect to. fn call(f: F) where F : FnOnce() { @@ -13,7 +12,7 @@ fn main() { call(|| { // Move `y`, but do not move `counter`, even though it is read // by value (note that it is also mutated). - for item in y { + for item in y { //~ WARN unused variable: `item` let v = counter; counter += v; } @@ -22,7 +21,8 @@ fn main() { call(move || { // this mutates a moved copy, and hence doesn't affect original - counter += 1; + counter += 1; //~ WARN value assigned to `counter` is never read + //~| WARN unused variable: `counter` }); assert_eq!(counter, 88); } diff --git a/src/test/ui/unboxed-closures/unboxed-closures-counter-not-moved.stderr b/src/test/ui/unboxed-closures/unboxed-closures-counter-not-moved.stderr new file mode 100644 index 0000000000000..ba4b3dac6705e --- /dev/null +++ b/src/test/ui/unboxed-closures/unboxed-closures-counter-not-moved.stderr @@ -0,0 +1,27 @@ +warning: unused variable: `item` + --> $DIR/unboxed-closures-counter-not-moved.rs:15:13 + | +LL | for item in y { + | ^^^^ help: if this is intentional, prefix it with an underscore: `_item` + | + = note: `#[warn(unused_variables)]` on by default + +warning: value assigned to `counter` is never read + --> $DIR/unboxed-closures-counter-not-moved.rs:24:9 + | +LL | counter += 1; + | ^^^^^^^ + | + = note: `#[warn(unused_assignments)]` on by default + = help: maybe it is overwritten before being read? + +warning: unused variable: `counter` + --> $DIR/unboxed-closures-counter-not-moved.rs:24:9 + | +LL | counter += 1; + | ^^^^^^^ + | + = help: did you mean to capture by reference instead? + +warning: 3 warnings emitted + diff --git a/src/test/ui/unboxed-closures/unboxed-closures-move-mutable.rs b/src/test/ui/unboxed-closures/unboxed-closures-move-mutable.rs index 9b519e63a95cc..e5b19db782231 100644 --- a/src/test/ui/unboxed-closures/unboxed-closures-move-mutable.rs +++ b/src/test/ui/unboxed-closures/unboxed-closures-move-mutable.rs @@ -13,11 +13,11 @@ fn set(x: &mut usize) { *x = 42; } fn main() { { let mut x = 0_usize; - move || x += 1; + move || x += 1; //~ WARN unused variable: `x` } { let mut x = 0_usize; - move || x += 1; + move || x += 1; //~ WARN unused variable: `x` } { let mut x = 0_usize; diff --git a/src/test/ui/unboxed-closures/unboxed-closures-move-mutable.stderr b/src/test/ui/unboxed-closures/unboxed-closures-move-mutable.stderr new file mode 100644 index 0000000000000..4dfd1bb307574 --- /dev/null +++ b/src/test/ui/unboxed-closures/unboxed-closures-move-mutable.stderr @@ -0,0 +1,19 @@ +warning: unused variable: `x` + --> $DIR/unboxed-closures-move-mutable.rs:16:17 + | +LL | move || x += 1; + | ^ + | + = note: `#[warn(unused_variables)]` on by default + = help: did you mean to capture by reference instead? + +warning: unused variable: `x` + --> $DIR/unboxed-closures-move-mutable.rs:20:17 + | +LL | move || x += 1; + | ^ + | + = help: did you mean to capture by reference instead? + +warning: 2 warnings emitted +