collect unused unsafe code

arielb1 · arielb1 · commit 12aedc833cce · 2017-11-06T23:41:18.000+02:00
FIXME: de-uglify
diff --git a/src/librustc/dep_graph/dep_node.rs b/src/librustc/dep_graph/dep_node.rs
@@ -482,7 +482,7 @@ define_dep_nodes!( <'tcx>
     [] BorrowCheckKrate,
     [] BorrowCheck(DefId),
     [] MirBorrowCheck(DefId),
-    [] UnsafetyViolations(DefId),
+    [] UnsafetyCheckResult(DefId),
 
     [] Reachability,
     [] MirKeys,
diff --git a/src/librustc/ich/impls_mir.rs b/src/librustc/ich/impls_mir.rs
@@ -34,6 +34,7 @@ impl_stable_hash_for!(struct mir::LocalDecl<'tcx> {
 impl_stable_hash_for!(struct mir::UpvarDecl { debug_name, by_ref });
 impl_stable_hash_for!(struct mir::BasicBlockData<'tcx> { statements, terminator, is_cleanup });
 impl_stable_hash_for!(struct mir::UnsafetyViolation { source_info, description, lint_node_id });
+impl_stable_hash_for!(struct mir::UnsafetyCheckResult { violations, unsafe_blocks });
 
 impl<'gcx> HashStable<StableHashingContext<'gcx>>
 for mir::Terminator<'gcx> {
diff --git a/src/librustc/mir/mod.rs b/src/librustc/mir/mod.rs
@@ -33,6 +33,7 @@ use std::cell::Ref;
 use std::fmt::{self, Debug, Formatter, Write};
 use std::{iter, u32};
 use std::ops::{Index, IndexMut};
+use std::rc::Rc;
 use std::vec::IntoIter;
 use syntax::ast::{self, Name};
 use syntax_pos::Span;
@@ -1683,6 +1684,15 @@ pub struct UnsafetyViolation {
     pub lint_node_id: Option<ast::NodeId>,
 }
 
+#[derive(Clone, Debug, PartialEq, Eq, Hash)]
+pub struct UnsafetyCheckResult {
+    /// Violations that are propagated *upwards* from this function
+    pub violations: Rc<[UnsafetyViolation]>,
+    /// unsafe blocks in this function, along with whether they are used. This is
+    /// used for the "unused_unsafe" lint.
+    pub unsafe_blocks: Rc<[(ast::NodeId, bool)]>,
+}
+
 /// The layout of generator state
 #[derive(Clone, Debug, RustcEncodable, RustcDecodable)]
 pub struct GeneratorLayout<'tcx> {
diff --git a/src/librustc/ty/maps/mod.rs b/src/librustc/ty/maps/mod.rs
@@ -171,9 +171,8 @@ define_maps! { <'tcx>
     /// expression defining the closure.
     [] fn closure_kind: ClosureKind(DefId) -> ty::ClosureKind,
 
-    /// Unsafety violations for this def ID.
-    [] fn unsafety_violations: UnsafetyViolations(DefId)
-        -> Rc<[mir::UnsafetyViolation]>,
+    /// The result of unsafety-checking this def-id.
+    [] fn unsafety_check_result: UnsafetyCheckResult(DefId) -> mir::UnsafetyCheckResult,
 
     /// The signature of functions and closures.
     [] fn fn_sig: FnSignature(DefId) -> ty::PolyFnSig<'tcx>,
diff --git a/src/librustc/ty/maps/plumbing.rs b/src/librustc/ty/maps/plumbing.rs
@@ -720,7 +720,7 @@ pub fn force_from_dep_node<'a, 'gcx, 'lcx>(tcx: TyCtxt<'a, 'gcx, 'lcx>,
 
         DepKind::BorrowCheck => { force!(borrowck, def_id!()); }
         DepKind::MirBorrowCheck => { force!(mir_borrowck, def_id!()); }
-        DepKind::UnsafetyViolations => { force!(unsafety_violations, def_id!()); }
+        DepKind::UnsafetyCheckResult => { force!(unsafety_check_result, def_id!()); }
         DepKind::Reachability => { force!(reachable_set, LOCAL_CRATE); }
         DepKind::MirKeys => { force!(mir_keys, LOCAL_CRATE); }
         DepKind::CrateVariances => { force!(crate_variances, LOCAL_CRATE); }
diff --git a/src/librustc_mir/transform/check_unsafety.rs b/src/librustc_mir/transform/check_unsafety.rs
@@ -34,6 +34,7 @@ pub struct UnsafetyChecker<'a, 'tcx: 'a> {
     tcx: TyCtxt<'a, 'tcx, 'tcx>,
     param_env: ty::ParamEnv<'tcx>,
     used_unsafe: FxHashSet<ast::NodeId>,
+    inherited_blocks: Vec<(ast::NodeId, bool)>,
 }
 
 impl<'a, 'gcx, 'tcx> UnsafetyChecker<'a, 'tcx> {
@@ -52,6 +53,7 @@ impl<'a, 'gcx, 'tcx> UnsafetyChecker<'a, 'tcx> {
             tcx,
             param_env,
             used_unsafe: FxHashSet(),
+            inherited_blocks: vec![],
         }
     }
 }
@@ -124,8 +126,11 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
                 &AggregateKind::Adt(..) => {}
                 &AggregateKind::Closure(def_id, _) |
                 &AggregateKind::Generator(def_id, _, _) => {
-                    let unsafety_violations = self.tcx.unsafety_violations(def_id);
-                    self.register_violations(&unsafety_violations);
+                    let UnsafetyCheckResult {
+                        violations, unsafe_blocks
+                    } = self.tcx.unsafety_check_result(def_id);
+                    self.inherited_blocks.extend(unsafe_blocks.iter().cloned());
+                    self.register_violations(&violations, &unsafe_blocks);
                 }
             }
         }
@@ -194,7 +199,7 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
                         source_info,
                         description: "use of extern static",
                         lint_node_id: Some(lint_root)
-                    }]);
+                    }], &[]);
                 }
             }
         }
@@ -227,41 +232,49 @@ impl<'a, 'tcx> UnsafetyChecker<'a, 'tcx> {
         let source_info = self.source_info;
         self.register_violations(&[UnsafetyViolation {
             source_info, description, lint_node_id: None
-        }]);
+        }], &[]);
     }
 
-    fn register_violations(&mut self, violations: &[UnsafetyViolation]) {
-        match self.visibility_scope_info[self.source_info.scope].safety {
+    fn register_violations(&mut self,
+                           violations: &[UnsafetyViolation],
+                           unsafe_blocks: &[(ast::NodeId, bool)]) {
+        let within_unsafe = match self.visibility_scope_info[self.source_info.scope].safety {
             Safety::Safe => {
                 for violation in violations {
                     if !self.violations.contains(violation) {
                         self.violations.push(violation.clone())
                     }
                 }
+
+                false
             }
-            Safety::BuiltinUnsafe | Safety::FnUnsafe => {}
+            Safety::BuiltinUnsafe | Safety::FnUnsafe => true,
             Safety::ExplicitUnsafe(node_id) => {
                 if !violations.is_empty() {
                     self.used_unsafe.insert(node_id);
                 }
+                true
             }
-        }
+        };
+        self.inherited_blocks.extend(unsafe_blocks.iter().map(|&(node_id, is_used)| {
+            (node_id, is_used && !within_unsafe)
+        }));
     }
 }
 
 pub(crate) fn provide(providers: &mut Providers) {
     *providers = Providers {
-        unsafety_violations,
+        unsafety_check_result,
         ..*providers
     };
 }
 
-struct UnusedUnsafeVisitor<'a, 'tcx: 'a> {
-    tcx: TyCtxt<'a, 'tcx, 'tcx>,
-    used_unsafe: FxHashSet<ast::NodeId>
+struct UnusedUnsafeVisitor<'a> {
+    used_unsafe: &'a FxHashSet<ast::NodeId>,
+    unsafe_blocks: &'a mut Vec<(ast::NodeId, bool)>,
 }
 
-impl<'a, 'tcx> hir::intravisit::Visitor<'tcx> for UnusedUnsafeVisitor<'a, 'tcx> {
+impl<'a, 'tcx> hir::intravisit::Visitor<'tcx> for UnusedUnsafeVisitor<'a> {
     fn nested_visit_map<'this>(&'this mut self) ->
         hir::intravisit::NestedVisitorMap<'this, 'tcx>
     {
@@ -272,50 +285,15 @@ impl<'a, 'tcx> hir::intravisit::Visitor<'tcx> for UnusedUnsafeVisitor<'a, 'tcx>
         hir::intravisit::walk_block(self, block);
 
         if let hir::UnsafeBlock(hir::UserProvided) = block.rules {
-            if !self.used_unsafe.contains(&block.id) {
-                self.report_unused_unsafe(block);
-            }
-        }
-    }
-}
-
-impl<'a, 'tcx> UnusedUnsafeVisitor<'a, 'tcx> {
-    /// Return the NodeId for an enclosing scope that is also `unsafe`
-    fn is_enclosed(&self, id: ast::NodeId) -> Option<(String, ast::NodeId)> {
-        let parent_id = self.tcx.hir.get_parent_node(id);
-        if parent_id != id {
-            if self.used_unsafe.contains(&parent_id) {
-                Some(("block".to_string(), parent_id))
-            } else if let Some(hir::map::NodeItem(&hir::Item {
-                node: hir::ItemFn(_, hir::Unsafety::Unsafe, _, _, _, _),
-                ..
-            })) = self.tcx.hir.find(parent_id) {
-                Some(("fn".to_string(), parent_id))
-            } else {
-                self.is_enclosed(parent_id)
-            }
-        } else {
-            None
+            self.unsafe_blocks.push((block.id, self.used_unsafe.contains(&block.id)));
         }
     }
-
-    fn report_unused_unsafe(&self, block: &'tcx hir::Block) {
-        let mut db = self.tcx.struct_span_lint_node(UNUSED_UNSAFE,
-                                                    block.id,
-                                                    block.span,
-                                                    "unnecessary `unsafe` block");
-        db.span_label(block.span, "unnecessary `unsafe` block");
-        if let Some((kind, id)) = self.is_enclosed(block.id) {
-            db.span_note(self.tcx.hir.span(id),
-                         &format!("because it's nested under this `unsafe` {}", kind));
-        }
-        db.emit();
-    }
 }
 
 fn check_unused_unsafe<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
                                  def_id: DefId,
-                                 used_unsafe: FxHashSet<ast::NodeId>)
+                                 used_unsafe: &FxHashSet<ast::NodeId>,
+                                 unsafe_blocks: &'a mut Vec<(ast::NodeId, bool)>)
 {
     let body_id =
         tcx.hir.as_local_node_id(def_id).and_then(|node_id| {
@@ -333,13 +311,12 @@ fn check_unused_unsafe<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
     debug!("check_unused_unsafe({:?}, body={:?}, used_unsafe={:?})",
            def_id, body, used_unsafe);
 
-    hir::intravisit::Visitor::visit_body(
-        &mut UnusedUnsafeVisitor { tcx, used_unsafe },
-        body);
+    let mut visitor =  UnusedUnsafeVisitor { used_unsafe, unsafe_blocks };
+    hir::intravisit::Visitor::visit_body(&mut visitor, body);
 }
 
-fn unsafety_violations<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) ->
-    Rc<[UnsafetyViolation]>
+fn unsafety_check_result<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId)
+                                   -> UnsafetyCheckResult
 {
     debug!("unsafety_violations({:?})", def_id);
 
@@ -351,7 +328,10 @@ fn unsafety_violations<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) ->
         ClearOnDecode::Set(ref data) => data,
         ClearOnDecode::Clear => {
             debug!("unsafety_violations: {:?} - remote, skipping", def_id);
-            return Rc::new([])
+            return UnsafetyCheckResult {
+                violations: Rc::new([]),
+                unsafe_blocks: Rc::new([])
+            }
         }
     };
 
@@ -360,8 +340,43 @@ fn unsafety_violations<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) ->
         mir, visibility_scope_info, tcx, param_env);
     checker.visit_mir(mir);
 
-    check_unused_unsafe(tcx, def_id, checker.used_unsafe);
-    checker.violations.into()
+    check_unused_unsafe(tcx, def_id, &checker.used_unsafe, &mut checker.inherited_blocks);
+    UnsafetyCheckResult {
+        violations: checker.violations.into(),
+        unsafe_blocks: checker.inherited_blocks.into()
+    }
+}
+
+/// Return the NodeId for an enclosing scope that is also `unsafe`
+fn is_enclosed(tcx: TyCtxt,
+               used_unsafe: &FxHashSet<ast::NodeId>,
+               id: ast::NodeId) -> Option<(String, ast::NodeId)> {
+    let parent_id = tcx.hir.get_parent_node(id);
+    if parent_id != id {
+        if used_unsafe.contains(&parent_id) {
+            Some(("block".to_string(), parent_id))
+        } else if let Some(hir::map::NodeItem(&hir::Item {
+            node: hir::ItemFn(_, hir::Unsafety::Unsafe, _, _, _, _),
+            ..
+        })) = tcx.hir.find(parent_id) {
+            Some(("fn".to_string(), parent_id))
+        } else {
+            is_enclosed(tcx, used_unsafe, parent_id)
+        }
+    } else {
+        None
+    }
+}
+
+fn report_unused_unsafe(tcx: TyCtxt, used_unsafe: &FxHashSet<ast::NodeId>, id: ast::NodeId) {
+    let span = tcx.hir.span(id);
+    let mut db = tcx.struct_span_lint_node(UNUSED_UNSAFE, id, span, "unnecessary `unsafe` block");
+    db.span_label(span, "unnecessary `unsafe` block");
+    if let Some((kind, id)) = is_enclosed(tcx, used_unsafe, id) {
+        db.span_note(tcx.hir.span(id),
+                     &format!("because it's nested under this `unsafe` {}", kind));
+    }
+    db.emit();
 }
 
 pub fn check_unsafety<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) {
@@ -372,9 +387,14 @@ pub fn check_unsafety<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) {
         _ => {}
     };
 
+    let UnsafetyCheckResult {
+        violations,
+        unsafe_blocks
+    } = tcx.unsafety_check_result(def_id);
+
     for &UnsafetyViolation {
         source_info, description, lint_node_id
-    } in &*tcx.unsafety_violations(def_id) {
+    } in violations.iter() {
         // Report an error.
         if let Some(lint_node_id) = lint_node_id {
             tcx.lint_node(SAFE_EXTERN_STATICS,
@@ -390,4 +410,15 @@ pub fn check_unsafety<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) {
                 .emit();
         }
     }
+
+    let mut unsafe_blocks: Vec<_> = unsafe_blocks.into_iter().collect();
+    unsafe_blocks.sort();
+    let used_unsafe: FxHashSet<_> = unsafe_blocks.iter()
+        .flat_map(|&&(id, used)| if used { Some(id) } else { None })
+        .collect();
+    for &(block_id, is_used) in unsafe_blocks {
+        if !is_used {
+            report_unused_unsafe(tcx, &used_unsafe, block_id);
+        }
+    }
 }
diff --git a/src/librustc_mir/transform/mod.rs b/src/librustc_mir/transform/mod.rs
@@ -111,7 +111,7 @@ fn mir_built<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> &'tcx Stea
 
 fn mir_const<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> &'tcx Steal<Mir<'tcx>> {
     // Unsafety check uses the raw mir, so make sure it is run
-    let _ = tcx.unsafety_violations(def_id);
+    let _ = tcx.unsafety_check_result(def_id);
 
     let source = MirSource::from_local_def_id(tcx, def_id);
     let mut mir = tcx.mir_built(def_id).steal();
diff --git a/src/test/ui/issue-45107-unnecessary-unsafe-in-closure.rs b/src/test/ui/issue-45107-unnecessary-unsafe-in-closure.rs
@@ -0,0 +1,35 @@
+// 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.
+
+#[deny(unused_unsafe)]
+fn main() {
+    let mut v = Vec::<i32>::with_capacity(24);
+
+    unsafe {
+        let f = |v: &mut Vec<_>| {
+            unsafe {
+                v.set_len(24);
+                |w: &mut Vec<u32>| { unsafe {
+                    w.set_len(32);
+                } };
+            }
+            |x: &mut Vec<u32>| { unsafe {
+                x.set_len(40);
+            } };
+        };
+
+        v.set_len(0);
+        f(&mut v);
+    }
+
+    |y: &mut Vec<u32>| { unsafe {
+        y.set_len(48);
+    } };
+}
diff --git a/src/test/ui/issue-45107-unnecessary-unsafe-in-closure.stderr b/src/test/ui/issue-45107-unnecessary-unsafe-in-closure.stderr
diff --git a/src/test/ui/span/lint-unused-unsafe.stderr b/src/test/ui/span/lint-unused-unsafe.stderr
