rustc: Remove used_mut_nodes from TyCtxt

alexcrichton · alexcrichton · commit 4df1278c69f6 · 2017-10-14T09:29:02.000-07:00
This updates the borrowck query to return a result, and this result is then used to incrementally check for unused mutable nodes given sets of all the used mutable nodes. Closes #42384
diff --git a/src/Cargo.lock b/src/Cargo.lock
diff --git a/src/librustc/lib.rs b/src/librustc/lib.rs
@@ -106,6 +106,7 @@ pub mod lint;
 
 pub mod middle {
     pub mod allocator;
+    pub mod borrowck;
     pub mod expr_use_visitor;
     pub mod const_val;
     pub mod cstore;
diff --git a/src/librustc/lint/builtin.rs b/src/librustc/lint/builtin.rs
@@ -222,6 +222,12 @@ declare_lint! {
     "unnecessary use of an `unsafe` block"
 }
 
+declare_lint! {
+    pub UNUSED_MUT,
+    Warn,
+    "detect mut variables which don't need to be mutable"
+}
+
 /// Does nothing as a lint pass, but registers some `Lint`s
 /// which are used by other parts of the compiler.
 #[derive(Copy, Clone)]
@@ -263,7 +269,8 @@ impl LintPass for HardwiredLints {
             PARENTHESIZED_PARAMS_IN_TYPES_AND_MODULES,
             LATE_BOUND_LIFETIME_ARGUMENTS,
             DEPRECATED,
-            UNUSED_UNSAFE
+            UNUSED_UNSAFE,
+            UNUSED_MUT
         )
     }
 }
diff --git a/src/librustc/middle/borrowck.rs b/src/librustc/middle/borrowck.rs
@@ -0,0 +1,31 @@
+// Copyright 2017 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.
+
+use ich::StableHashingContext;
+use hir::HirId;
+use util::nodemap::FxHashSet;
+
+use rustc_data_structures::stable_hasher::{HashStable, StableHasher,
+                                           StableHasherResult};
+
+pub struct BorrowCheckResult {
+    pub used_mut_nodes: FxHashSet<HirId>,
+}
+
+impl<'gcx> HashStable<StableHashingContext<'gcx>> for BorrowCheckResult {
+    fn hash_stable<W: StableHasherResult>(&self,
+                                          hcx: &mut StableHashingContext<'gcx>,
+                                          hasher: &mut StableHasher<W>) {
+        let BorrowCheckResult {
+            ref used_mut_nodes,
+        } = *self;
+        used_mut_nodes.hash_stable(hcx, hasher);
+    }
+}
diff --git a/src/librustc/ty/context.rs b/src/librustc/ty/context.rs
@@ -898,11 +898,6 @@ pub struct GlobalCtxt<'tcx> {
 
     pub inhabitedness_cache: RefCell<FxHashMap<Ty<'tcx>, DefIdForest>>,
 
-    /// Set of nodes which mark locals as mutable which end up getting used at
-    /// some point. Local variable definitions not in this set can be warned
-    /// about.
-    pub used_mut_nodes: RefCell<NodeSet>,
-
     /// Caches the results of trait selection. This cache is used
     /// for things that do not have to do with the parameters in scope.
     pub selection_cache: traits::SelectionCache<'tcx>,
@@ -1185,7 +1180,6 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
             rcache: RefCell::new(FxHashMap()),
             normalized_cache: RefCell::new(FxHashMap()),
             inhabitedness_cache: RefCell::new(FxHashMap()),
-            used_mut_nodes: RefCell::new(NodeSet()),
             selection_cache: traits::SelectionCache::new(),
             evaluation_cache: traits::EvaluationCache::new(),
             rvalue_promotable_to_static: RefCell::new(NodeMap()),
diff --git a/src/librustc/ty/maps/mod.rs b/src/librustc/ty/maps/mod.rs
@@ -15,6 +15,7 @@ use hir::def::{Def, Export};
 use hir::{self, TraitCandidate, ItemLocalId};
 use hir::svh::Svh;
 use lint;
+use middle::borrowck::BorrowCheckResult;
 use middle::const_val;
 use middle::cstore::{ExternCrate, LinkagePreference, NativeLibrary,
                      ExternBodyNestedBodies};
@@ -183,7 +184,7 @@ define_maps! { <'tcx>
 
     [] fn coherent_trait: coherent_trait_dep_node((CrateNum, DefId)) -> (),
 
-    [] fn borrowck: BorrowCheck(DefId) -> (),
+    [] fn borrowck: BorrowCheck(DefId) -> Rc<BorrowCheckResult>,
     // FIXME: shouldn't this return a `Result<(), BorrowckErrors>` instead?
     [] fn mir_borrowck: MirBorrowCheck(DefId) -> (),
 
diff --git a/src/librustc_borrowck/Cargo.toml b/src/librustc_borrowck/Cargo.toml
@@ -15,5 +15,6 @@ syntax = { path = "../libsyntax" }
 syntax_pos = { path = "../libsyntax_pos" }
 graphviz = { path = "../libgraphviz" }
 rustc = { path = "../librustc" }
+rustc_back = { path = "../librustc_back" }
 rustc_mir = { path = "../librustc_mir" }
 rustc_errors = { path = "../librustc_errors" }
diff --git a/src/librustc_borrowck/borrowck/check_loans.rs b/src/librustc_borrowck/borrowck/check_loans.rs
@@ -770,7 +770,8 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
             let lp = opt_loan_path(&assignee_cmt).unwrap();
             self.move_data.each_assignment_of(assignment_id, &lp, |assign| {
                 if assignee_cmt.mutbl.is_mutable() {
-                    self.tcx().used_mut_nodes.borrow_mut().insert(local_id);
+                    let hir_id = self.bccx.tcx.hir.node_to_hir_id(local_id);
+                    self.bccx.used_mut_nodes.borrow_mut().insert(hir_id);
                 } else {
                     self.bccx.report_reassigned_immutable_variable(
                         assignment_span,
diff --git a/src/librustc_borrowck/borrowck/gather_loans/mod.rs b/src/librustc_borrowck/borrowck/gather_loans/mod.rs
@@ -442,13 +442,13 @@ impl<'a, 'tcx> GatherLoanCtxt<'a, 'tcx> {
             wrapped_path = match current_path.kind {
                 LpVar(local_id) => {
                     if !through_borrow {
-                        self.tcx().used_mut_nodes.borrow_mut().insert(local_id);
+                        let hir_id = self.bccx.tcx.hir.node_to_hir_id(local_id);
+                        self.bccx.used_mut_nodes.borrow_mut().insert(hir_id);
                     }
                     None
                 }
                 LpUpvar(ty::UpvarId{ var_id, closure_expr_id: _ }) => {
-                    let local_id = self.tcx().hir.hir_to_node_id(var_id);
-                    self.tcx().used_mut_nodes.borrow_mut().insert(local_id);
+                    self.bccx.used_mut_nodes.borrow_mut().insert(var_id);
                     None
                 }
                 LpExtend(ref base, mc::McInherited, LpDeref(pointer_kind)) |
diff --git a/src/librustc_borrowck/borrowck/mod.rs b/src/librustc_borrowck/borrowck/mod.rs
@@ -20,13 +20,15 @@ pub use self::MovedValueUseKind::*;
 
 use self::InteriorKind::*;
 
+use rustc::hir::HirId;
 use rustc::hir::map as hir_map;
 use rustc::hir::map::blocks::FnLikeNode;
 use rustc::cfg;
 use rustc::middle::dataflow::DataFlowContext;
 use rustc::middle::dataflow::BitwiseOperator;
 use rustc::middle::dataflow::DataFlowOperator;
 use rustc::middle::dataflow::KillFrom;
+use rustc::middle::borrowck::BorrowCheckResult;
 use rustc::hir::def_id::{DefId, DefIndex};
 use rustc::middle::expr_use_visitor as euv;
 use rustc::middle::mem_categorization as mc;
@@ -37,7 +39,9 @@ use rustc::middle::free_region::RegionRelations;
 use rustc::ty::{self, Ty, TyCtxt};
 use rustc::ty::maps::Providers;
 use rustc_mir::util::borrowck_errors::{BorrowckErrors, Origin};
+use rustc::util::nodemap::FxHashSet;
 
+use std::cell::RefCell;
 use std::fmt;
 use std::rc::Rc;
 use std::hash::{Hash, Hasher};
@@ -54,6 +58,8 @@ pub mod gather_loans;
 
 pub mod move_data;
 
+mod unused;
+
 #[derive(Clone, Copy)]
 pub struct LoanDataFlowOperator;
 
@@ -79,7 +85,9 @@ pub struct AnalysisData<'a, 'tcx: 'a> {
     pub move_data: move_data::FlowedMoveData<'a, 'tcx>,
 }
 
-fn borrowck<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, owner_def_id: DefId) {
+fn borrowck<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, owner_def_id: DefId)
+    -> Rc<BorrowCheckResult>
+{
     debug!("borrowck(body_owner_def_id={:?})", owner_def_id);
 
     let owner_id = tcx.hir.as_local_node_id(owner_def_id).unwrap();
@@ -91,7 +99,9 @@ fn borrowck<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, owner_def_id: DefId) {
             // those things (notably the synthesized constructors from
             // tuple structs/variants) do not have an associated body
             // and do not need borrowchecking.
-            return;
+            return Rc::new(BorrowCheckResult {
+                used_mut_nodes: FxHashSet(),
+            })
         }
         _ => { }
     }
@@ -100,7 +110,14 @@ fn borrowck<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, owner_def_id: DefId) {
     let tables = tcx.typeck_tables_of(owner_def_id);
     let region_scope_tree = tcx.region_scope_tree(owner_def_id);
     let body = tcx.hir.body(body_id);
-    let bccx = &mut BorrowckCtxt { tcx, tables, region_scope_tree, owner_def_id, body };
+    let mut bccx = BorrowckCtxt {
+        tcx,
+        tables,
+        region_scope_tree,
+        owner_def_id,
+        body,
+        used_mut_nodes: RefCell::new(FxHashSet()),
+    };
 
     // Eventually, borrowck will always read the MIR, but at the
     // moment we do not. So, for now, we always force MIR to be
@@ -118,14 +135,19 @@ fn borrowck<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, owner_def_id: DefId) {
     if let Some(AnalysisData { all_loans,
                                loans: loan_dfcx,
                                move_data: flowed_moves }) =
-        build_borrowck_dataflow_data(bccx, false, body_id,
+        build_borrowck_dataflow_data(&mut bccx, false, body_id,
                                      |bccx| {
                                          cfg = Some(cfg::CFG::new(bccx.tcx, &body));
                                          cfg.as_mut().unwrap()
                                      })
     {
-        check_loans::check_loans(bccx, &loan_dfcx, &flowed_moves, &all_loans, body);
+        check_loans::check_loans(&mut bccx, &loan_dfcx, &flowed_moves, &all_loans, body);
     }
+    unused::check(&mut bccx, body);
+
+    Rc::new(BorrowCheckResult {
+        used_mut_nodes: bccx.used_mut_nodes.into_inner(),
+    })
 }
 
 fn build_borrowck_dataflow_data<'a, 'c, 'tcx, F>(this: &mut BorrowckCtxt<'a, 'tcx>,
@@ -198,7 +220,14 @@ pub fn build_borrowck_dataflow_data_for_fn<'a, 'tcx>(
     let tables = tcx.typeck_tables_of(owner_def_id);
     let region_scope_tree = tcx.region_scope_tree(owner_def_id);
     let body = tcx.hir.body(body_id);
-    let mut bccx = BorrowckCtxt { tcx, tables, region_scope_tree, owner_def_id, body };
+    let mut bccx = BorrowckCtxt {
+        tcx,
+        tables,
+        region_scope_tree,
+        owner_def_id,
+        body,
+        used_mut_nodes: RefCell::new(FxHashSet()),
+    };
 
     let dataflow_data = build_borrowck_dataflow_data(&mut bccx, true, body_id, |_| cfg);
     (bccx, dataflow_data.unwrap())
@@ -219,6 +248,8 @@ pub struct BorrowckCtxt<'a, 'tcx: 'a> {
     owner_def_id: DefId,
 
     body: &'tcx hir::Body,
+
+    used_mut_nodes: RefCell<FxHashSet<HirId>>,
 }
 
 impl<'b, 'tcx: 'b> BorrowckErrors for BorrowckCtxt<'b, 'tcx> {
diff --git a/src/librustc_borrowck/borrowck/unused.rs b/src/librustc_borrowck/borrowck/unused.rs
@@ -0,0 +1,118 @@
+// Copyright 2017 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.
+
+use rustc::hir::intravisit::{Visitor, NestedVisitorMap};
+use rustc::hir::{self, HirId};
+use rustc::lint::builtin::UNUSED_MUT;
+use rustc::ty;
+use rustc::util::nodemap::{FxHashMap, FxHashSet};
+use rustc_back::slice;
+use syntax::ptr::P;
+
+use borrowck::BorrowckCtxt;
+
+pub fn check<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>, body: &'tcx hir::Body) {
+    let mut used_mut = bccx.used_mut_nodes.borrow().clone();
+    UsedMutFinder {
+        bccx,
+        set: &mut used_mut,
+    }.visit_expr(&body.value);
+    let mut cx = UnusedMutCx { bccx, used_mut };
+    for arg in body.arguments.iter() {
+        cx.check_unused_mut_pat(slice::ref_slice(&arg.pat));
+    }
+    cx.visit_expr(&body.value);
+}
+
+struct UsedMutFinder<'a, 'tcx: 'a> {
+    bccx: &'a BorrowckCtxt<'a, 'tcx>,
+    set: &'a mut FxHashSet<HirId>,
+}
+
+struct UnusedMutCx<'a, 'tcx: 'a> {
+    bccx: &'a BorrowckCtxt<'a, 'tcx>,
+    used_mut: FxHashSet<HirId>,
+}
+
+impl<'a, 'tcx> UnusedMutCx<'a, 'tcx> {
+    fn check_unused_mut_pat(&self, pats: &[P<hir::Pat>]) {
+        let tcx = self.bccx.tcx;
+        let mut mutables = FxHashMap();
+        for p in pats {
+            p.each_binding(|_, id, span, path1| {
+                let name = path1.node;
+
+                // Skip anything that looks like `_foo`
+                if name.as_str().starts_with("_") {
+                    return
+                }
+
+                // Skip anything that looks like `&foo` or `&mut foo`, only look
+                // for by-value bindings
+                let hir_id = tcx.hir.node_to_hir_id(id);
+                let bm = match self.bccx.tables.pat_binding_modes().get(hir_id) {
+                    Some(&bm) => bm,
+                    None => span_bug!(span, "missing binding mode"),
+                };
+                match bm {
+                    ty::BindByValue(hir::MutMutable) => {}
+                    _ => return,
+                }
+
+                mutables.entry(name).or_insert(Vec::new()).push((id, hir_id, span));
+            });
+        }
+
+        for (_name, ids) in mutables {
+            // If any id for this name was used mutably then consider them all
+            // ok, so move on to the next
+            if ids.iter().any(|&(_, ref id, _)| self.used_mut.contains(id)) {
+                continue
+            }
+
+            let mut_span = tcx.sess.codemap().span_until_char(ids[0].2, ' ');
+
+            // Ok, every name wasn't used mutably, so issue a warning that this
+            // didn't need to be mutable.
+            tcx.struct_span_lint_node(UNUSED_MUT,
+                                      ids[0].0,
+                                      ids[0].2,
+                                      "variable does not need to be mutable")
+                .span_suggestion_short(mut_span, "remove this `mut`", "".to_owned())
+                .emit();
+        }
+    }
+}
+
+impl<'a, 'tcx> Visitor<'tcx> for UnusedMutCx<'a, 'tcx> {
+    fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
+        NestedVisitorMap::OnlyBodies(&self.bccx.tcx.hir)
+    }
+
+    fn visit_arm(&mut self, arm: &hir::Arm) {
+        self.check_unused_mut_pat(&arm.pats)
+    }
+
+    fn visit_local(&mut self, local: &hir::Local) {
+        self.check_unused_mut_pat(slice::ref_slice(&local.pat));
+    }
+}
+
+impl<'a, 'tcx> Visitor<'tcx> for UsedMutFinder<'a, 'tcx> {
+    fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
+        NestedVisitorMap::OnlyBodies(&self.bccx.tcx.hir)
+    }
+
+    fn visit_nested_body(&mut self, id: hir::BodyId) {
+        let def_id = self.bccx.tcx.hir.body_owner_def_id(id);
+        self.set.extend(self.bccx.tcx.borrowck(def_id).used_mut_nodes.iter().cloned());
+        self.visit_body(self.bccx.tcx.hir.body(id));
+    }
+}
diff --git a/src/librustc_borrowck/lib.rs b/src/librustc_borrowck/lib.rs
@@ -22,6 +22,7 @@
 #[macro_use] extern crate syntax;
 extern crate syntax_pos;
 extern crate rustc_errors as errors;
+extern crate rustc_back;
 
 // for "clarity", rename the graphviz crate to dot; graphviz within `borrowck`
 // refers to the borrowck-specific graphviz adapter traits.
diff --git a/src/librustc_lint/Cargo.toml b/src/librustc_lint/Cargo.toml
diff --git a/src/librustc_lint/lib.rs b/src/librustc_lint/lib.rs
diff --git a/src/librustc_lint/unused.rs b/src/librustc_lint/unused.rs
diff --git a/src/test/ui/lint/suggestions.stderr b/src/test/ui/lint/suggestions.stderr

Original file line number	Diff line number	Diff line change
`@@ -222,6 +222,12 @@ declare_lint! {`
`222`	`222`	"unnecessary use of an `unsafe` block"
`223`	`223`	`}`
`224`	`224`
	`225`	`+declare_lint! {`
	`226`	`+ pub UNUSED_MUT,`
	`227`	`+ Warn,`
	`228`	`+ "detect mut variables which don't need to be mutable"`
	`229`	`+}`
	`230`	`+`
`225`	`231`	/// Does nothing as a lint pass, but registers some `Lint`s
`226`	`232`	`/// which are used by other parts of the compiler.`
`227`	`233`	`#[derive(Copy, Clone)]`
`@@ -263,7 +269,8 @@ impl LintPass for HardwiredLints {`
`263`	`269`	`PARENTHESIZED_PARAMS_IN_TYPES_AND_MODULES,`
`264`	`270`	`LATE_BOUND_LIFETIME_ARGUMENTS,`
`265`	`271`	`DEPRECATED,`
`266`		`- UNUSED_UNSAFE`
	`272`	`+ UNUSED_UNSAFE,`
	`273`	`+ UNUSED_MUT`
`267`	`274`	`)`
`268`	`275`	`}`
`269`	`276`	`}`
Original file line number	Diff line number	Diff line change
`@@ -442,13 +442,13 @@ impl<'a, 'tcx> GatherLoanCtxt<'a, 'tcx> {`
`442`	`442`	`wrapped_path = match current_path.kind {`
`443`	`443`	`LpVar(local_id) => {`
`444`	`444`	`if !through_borrow {`
`445`		`- self.tcx().used_mut_nodes.borrow_mut().insert(local_id);`
	`445`	`+ let hir_id = self.bccx.tcx.hir.node_to_hir_id(local_id);`
	`446`	`+ self.bccx.used_mut_nodes.borrow_mut().insert(hir_id);`
`446`	`447`	`}`
`447`	`448`	`None`
`448`	`449`	`}`
`449`	`450`	`LpUpvar(ty::UpvarId{ var_id, closure_expr_id: _ }) => {`
`450`		`- let local_id = self.tcx().hir.hir_to_node_id(var_id);`
`451`		`- self.tcx().used_mut_nodes.borrow_mut().insert(local_id);`
	`451`	`+ self.bccx.used_mut_nodes.borrow_mut().insert(var_id);`
`452`	`452`	`None`
`453`	`453`	`}`
`454`	`454`	`LpExtend(ref base, mc::McInherited, LpDeref(pointer_kind)) \|`