servo: Merge #8073 - Make unrooted_must_root a bit more aggressive (from eefriedman:root-lint); r=Manishearth

Basically, instead of trying to check for specific kinds of statements,
just check the types of all local variables.

Also included are some commented-out proposals for some slightly more
aggressive lints which might be useful (but trigger a little too
frequently at the moment).

Source-Repo: https://github.com/servo/servo
Source-Revision: bb88832c078fbb14fa03c413fac1252b2b755015

servo/components/plugins/lints/unrooted_must_root.rs

@@ -8,7 +8,7 @@ use rustc::middle::ty;
 use rustc_front::{hir, visit};
 use syntax::attr::AttrMetaMethods;
 use syntax::{ast, codemap};
-use utils::{match_def_path, unsafe_context, in_derive_expn};
+use utils::{match_def_path, in_derive_expn};
 
 declare_lint!(UNROOTED_MUST_ROOT, Deny,
               "Warn and report usage of unrooted jsmanaged objects");
@@ -30,15 +30,11 @@ declare_lint!(UNROOTED_MUST_ROOT, Deny,
 /// Structs which have their own mechanism of rooting their unrooted contents (e.g. `ScriptTask`)
 /// can be marked as `#[allow(unrooted_must_root)]`. Smart pointers which root their interior type
 /// can be marked as `#[allow_unrooted_interior]`
-pub struct UnrootedPass {
-    in_new_function: bool
-}
+pub struct UnrootedPass;
 
 impl UnrootedPass {
     pub fn new() -> UnrootedPass {
-        UnrootedPass {
-            in_new_function: true
-        }
+        UnrootedPass
     }
 }
 
@@ -55,9 +51,11 @@ fn is_unrooted_ty(cx: &LateContext, ty: &ty::TyS, in_new_function: bool) -> bool
                 } else if cx.tcx.has_attr(did.did, "allow_unrooted_interior") {
                     false
                 } else if match_def_path(cx, did.did, &["core", "cell", "Ref"])
-                        || match_def_path(cx, did.did, &["core", "cell", "RefMut"]) {
-                        // Ref and RefMut are borrowed pointers, okay to hold unrooted stuff
-                        // since it will be rooted elsewhere
+                        || match_def_path(cx, did.did, &["core", "cell", "RefMut"])
+                        || match_def_path(cx, did.did, &["core", "slice", "Iter"])
+                        || match_def_path(cx, did.did, &["std", "collections", "hash", "map", "OccupiedEntry"])
+                        || match_def_path(cx, did.did, &["std", "collections", "hash", "map", "VacantEntry"]) {
+                    // Structures which are semantically similar to an &ptr.
                     false
                 } else {
                     true
@@ -99,6 +97,7 @@ impl LateLintPass for UnrootedPass {
             }
         }
     }
+
     /// All enums containing #[must_root] types must be #[must_root] themselves
     fn check_variant(&mut self, cx: &LateContext, var: &hir::Variant, _gen: &hir::Generics) {
         let ref map = cx.tcx.map;
@@ -121,55 +120,56 @@ impl LateLintPass for UnrootedPass {
     }
     /// Function arguments that are #[must_root] types are not allowed
     fn check_fn(&mut self, cx: &LateContext, kind: visit::FnKind, decl: &hir::FnDecl,
-                block: &hir::Block, span: codemap::Span, id: ast::NodeId) {
-        match kind {
+                block: &hir::Block, span: codemap::Span, _id: ast::NodeId) {
+        let in_new_function = match kind {
             visit::FnKind::ItemFn(n, _, _, _, _, _) |
-            visit::FnKind::Method(n, _, _) if n.as_str() == "new"
-                                           || n.as_str().starts_with("new_") => {
-                self.in_new_function = true;
-                return;
-            },
-            visit::FnKind::ItemFn(_, _, style, _, _, _) => match style {
-                hir::Unsafety::Unsafe => return,
-                _ => ()
-            },
-            _ => ()
-        }
-        self.in_new_function = false;
-
-        if unsafe_context(&cx.tcx.map, id) {
-            return;
-        }
-
-        match block.rules {
-            hir::DefaultBlock => {
-                for arg in &decl.inputs {
-                    cx.tcx.ast_ty_to_ty_cache.borrow().get(&arg.ty.id).map(|t| {
-                        if is_unrooted_ty(cx, t, false) {
-                            if in_derive_expn(cx, span) {
-                                return;
-                            }
-                            cx.span_lint(UNROOTED_MUST_ROOT, arg.ty.span, "Type must be rooted")
-                        }
-                    });
-                }
-                if let hir::Return(ref ty) = decl.output {
-                    cx.tcx.ast_ty_to_ty_cache.borrow().get(&ty.id).map(|t| {
-                        if is_unrooted_ty(cx, t, false) {
-                            if in_derive_expn(cx, span) {
-                                return;
-                            }
-                            cx.span_lint(UNROOTED_MUST_ROOT, ty.span, "Type must be rooted")
-                        }
-                    });
-                }
+            visit::FnKind::Method(n, _, _) => {
+                n.as_str() == "new" || n.as_str().starts_with("new_")
             }
-            _ => () // fn is `unsafe`
-        }
-    }
+            visit::FnKind::Closure => return,
+        };
+
+        for arg in &decl.inputs {
+            cx.tcx.ast_ty_to_ty_cache.borrow().get(&arg.ty.id).map(|t| {
+                if is_unrooted_ty(cx, t, false) {
+                    if in_derive_expn(cx, span) {
+                        return;
+                    }
+                    cx.span_lint(UNROOTED_MUST_ROOT, arg.ty.span, "Type must be rooted")
+                }
+            });
+        }
+
+        if !in_new_function {
+            if let hir::Return(ref ty) = decl.output {
+                cx.tcx.ast_ty_to_ty_cache.borrow().get(&ty.id).map(|t| {
+                    if is_unrooted_ty(cx, t, false) {
+                        if in_derive_expn(cx, span) {
+                            return;
+                        }
+                        cx.span_lint(UNROOTED_MUST_ROOT, ty.span, "Type must be rooted")
+                    }
+                });
+            }
+        }
+
+        let mut visitor = FnDefVisitor {
+            cx: cx,
+            in_new_function: in_new_function,
+        };
+        visit::walk_block(&mut visitor, block);
+    }
+}
+
+struct FnDefVisitor<'a, 'b: 'a, 'tcx: 'a+'b> {
+    cx: &'a LateContext<'b, 'tcx>,
+    in_new_function: bool,
+}
+
+impl<'a, 'b: 'a, 'tcx: 'a+'b> visit::Visitor<'a> for FnDefVisitor<'a, 'b, 'tcx> {
+    fn visit_expr(&mut self, expr: &'a hir::Expr) {
+        let cx = self.cx;
 
-    /// Trait casts from #[must_root] types are not allowed
-    fn check_expr(&mut self, cx: &LateContext, expr: &hir::Expr) {
         fn require_rooted(cx: &LateContext, in_new_function: bool, subexpr: &hir::Expr) {
             let ty = cx.tcx.expr_ty(&*subexpr);
             if is_unrooted_ty(cx, ty, in_new_function) {
@@ -177,56 +177,52 @@ impl LateLintPass for UnrootedPass {
                              subexpr.span,
                              &format!("Expression of type {:?} must be rooted", ty))
             }
-        };
+        }
 
         match expr.node {
+            /// Trait casts from #[must_root] types are not allowed
             hir::ExprCast(ref subexpr, _) => require_rooted(cx, self.in_new_function, &*subexpr),
+            // This catches assignments... the main point of this would be to catch mutable
+            // references to `JS<T>`.
+            // FIXME: Enable this? Triggers on certain kinds of uses of DOMRefCell.
+            // hir::ExprAssign(_, ref rhs) => require_rooted(cx, self.in_new_function, &*rhs),
+            // This catches calls; basically, this enforces the constraint that only constructors
+            // can call other constructors.
+            // FIXME: Enable this? Currently triggers with constructs involving DOMRefCell, and
+            // constructs like Vec<JS<T>> and RootedVec<JS<T>>.
+            // hir::ExprCall(..) if !self.in_new_function => {
+            //     require_rooted(cx, self.in_new_function, expr);
+            // }
             _ => {
                 // TODO(pcwalton): Check generics with a whitelist of allowed generics.
             }
         }
+
+        visit::walk_expr(self, expr);
     }
 
-    // Partially copied from rustc::middle::lint::builtin
-    // Catches `let` statements and assignments which store a #[must_root] value
-    // Expressions which return out of blocks eventually end up in a `let` or assignment
-    // statement or a function return (which will be caught when it is used elsewhere)
-    fn check_stmt(&mut self, cx: &LateContext, s: &hir::Stmt) {
-        match s.node {
-            hir::StmtDecl(_, id) |
-            hir::StmtExpr(_, id) |
-            hir::StmtSemi(_, id) if unsafe_context(&cx.tcx.map, id) => {
-                return
-            },
-            _ => ()
-        };
+    fn visit_pat(&mut self, pat: &'a hir::Pat) {
+        let cx = self.cx;
 
-        let expr = match s.node {
-            // Catch a `let` binding
-            hir::StmtDecl(ref decl, _) => match decl.node {
-                hir::DeclLocal(ref loc) => match loc.init {
-                    Some(ref e) => &**e,
-                    _ => return
-                },
-                _ => return
-            },
-            hir::StmtExpr(ref expr, _) => match expr.node {
-                // This catches deferred `let` statements
-                hir::ExprAssign(_, ref e) |
-                // Match statements allow you to bind onto the variable later in an arm
-                // We need not check arms individually since enum/struct fields are already
-                // linted in `check_struct_def` and `check_variant`
-                // (so there is no way of destructuring out a `#[must_root]` field)
-                hir::ExprMatch(ref e, _, _) => &**e,
-                _ => return
-            },
-            _ => return
-        };
+        if let hir::PatIdent(hir::BindByValue(_), _, _) = pat.node {
+            let ty = cx.tcx.pat_ty(pat);
+            if is_unrooted_ty(cx, ty, self.in_new_function) {
+                cx.span_lint(UNROOTED_MUST_ROOT,
+                            pat.span,
+                            &format!("Expression of type {:?} must be rooted", ty))
+            }
+        }
 
-        let ty = cx.tcx.expr_ty(&*expr);
-        if is_unrooted_ty(cx, ty, self.in_new_function) {
-            cx.span_lint(UNROOTED_MUST_ROOT, expr.span,
-                                     &format!("Expression of type {:?} must be rooted", ty))
+        visit::walk_pat(self, pat);
+    }
+
+    fn visit_fn(&mut self, kind: visit::FnKind<'a>, decl: &'a hir::FnDecl,
+                block: &'a hir::Block, span: codemap::Span, _id: ast::NodeId) {
+        if kind == visit::FnKind::Closure {
+            visit::walk_fn(self, kind, decl, block, span);
         }
     }
+
+    fn visit_foreign_item(&mut self, _: &'a hir::ForeignItem) {}
+    fn visit_ty(&mut self, _: &'a hir::Ty) { }
 }

servo/components/script/dom/bindings/js.rs

@@ -310,11 +310,13 @@ impl<T: Reflectable> MutNullableHeap<JS<T>> {
 
     /// Retrieve a copy of the inner optional `JS<T>` as `LayoutJS<T>`.
     /// For use by layout, which can't use safe types like Temporary.
+    #[allow(unrooted_must_root)]
     pub unsafe fn get_inner_as_layout(&self) -> Option<LayoutJS<T>> {
         ptr::read(self.ptr.get()).map(|js| js.to_layout())
     }
 
     /// Get a rooted value out of this object
+    #[allow(unrooted_must_root)]
     pub fn get(&self) -> Option<Root<T>> {
         unsafe {
             ptr::read(self.ptr.get()).map(|o| o.root())

servo/components/script/dom/canvasrenderingcontext2d.rs

@@ -431,6 +431,7 @@ impl CanvasRenderingContext2DMethods for CanvasRenderingContext2D {
         self.ipc_renderer.send(CanvasMsg::Canvas2d(Canvas2dMsg::SaveContext)).unwrap();
     }
 
+    #[allow(unrooted_must_root)]
     // https://html.spec.whatwg.org/multipage/#dom-context-2d-restore
     fn Restore(&self) {
         let mut saved_states = self.saved_states.borrow_mut();

servo/components/script/dom/filelist.rs

@@ -18,6 +18,7 @@ pub struct FileList {
 }
 
 impl FileList {
+    #[allow(unrooted_must_root)]
     fn new_inherited(files: Vec<JS<File>>) -> FileList {
         FileList {
             reflector_: Reflector::new(),
@@ -25,6 +26,7 @@ impl FileList {
         }
     }
 
+    #[allow(unrooted_must_root)]
     pub fn new(window: &Window, files: Vec<JS<File>>) -> Root<FileList> {
         reflect_dom_object(box FileList::new_inherited(files), GlobalRef::Window(window), FileListBinding::Wrap)
     }

servo/components/script/dom/htmlcollection.rs

@@ -32,6 +32,7 @@ pub struct HTMLCollection {
 }
 
 impl HTMLCollection {
+    #[allow(unrooted_must_root)]
     fn new_inherited(collection: Collection) -> HTMLCollection {
         HTMLCollection {
             reflector_: Reflector::new(),
@@ -39,6 +40,7 @@ impl HTMLCollection {
         }
     }
 
+    #[allow(unrooted_must_root)]
     pub fn new(window: &Window, collection: Collection) -> Root<HTMLCollection> {
         reflect_dom_object(box HTMLCollection::new_inherited(collection),
                            GlobalRef::Window(window), HTMLCollectionBinding::Wrap)

servo/components/script/dom/nodelist.rs

@@ -27,6 +27,7 @@ pub struct NodeList {
 }
 
 impl NodeList {
+    #[allow(unrooted_must_root)]
     fn new_inherited(list_type: NodeListType) -> NodeList {
         NodeList {
             reflector_: Reflector::new(),
@@ -34,6 +35,7 @@ impl NodeList {
         }
     }
 
+    #[allow(unrooted_must_root)]
     pub fn new(window: &Window,
                list_type: NodeListType) -> Root<NodeList> {
         reflect_dom_object(box NodeList::new_inherited(list_type),

servo/components/script/dom/webglrenderingcontext.rs

@@ -109,6 +109,7 @@ impl WebGLRenderingContext {
         })
     }
 
+    #[allow(unrooted_must_root)]
     pub fn new(global: GlobalRef, canvas: &HTMLCanvasElement, size: Size2D<i32>, attrs: GLContextAttributes)
                -> Option<Root<WebGLRenderingContext>> {
         match WebGLRenderingContext::new_inherited(global, canvas, size, attrs) {