Auto merge of #8030 - Manishearth:fix-lint-fn, r=frewsxcv

Fix unrooted_must_root lint to handle arguments/return types properly r? @jdm  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8030)
servo · Oct 15, 2015 · 9d5f09e · 9d5f09e
2 parents 6f1db0f + 8819f0d
commit 9d5f09e
Show file tree

Hide file tree

Showing 8 changed files with 44 additions and 11 deletions.
diff --git a/components/plugins/lints/unrooted_must_root.rs b/components/plugins/lints/unrooted_must_root.rs
@@ -4,12 +4,11 @@
 
 use rustc::front::map as ast_map;
 use rustc::lint::{LateContext, LintPass, LintArray, LateLintPass, LintContext};
-use rustc::middle::astconv_util::ast_ty_to_prim_ty;
 use rustc::middle::ty;
 use rustc_front::{hir, visit};
 use syntax::attr::AttrMetaMethods;
 use syntax::{ast, codemap};
-use utils::{match_def_path, unsafe_context};
+use utils::{match_def_path, unsafe_context, in_derive_expn};
 
 declare_lint!(UNROOTED_MUST_ROOT, Deny,
               "Warn and report usage of unrooted jsmanaged objects");
@@ -107,7 +106,7 @@ impl LateLintPass for UnrootedPass {
             match var.node.kind {
                 hir::TupleVariantKind(ref vec) => {
                     for ty in vec {
-                        ast_ty_to_prim_ty(cx.tcx, &*ty.ty).map(|t| {
+                        cx.tcx.ast_ty_to_ty_cache.borrow().get(&ty.id).map(|t| {
                             if is_unrooted_ty(cx, t, false) {
                                 cx.span_lint(UNROOTED_MUST_ROOT, ty.ty.span,
                                              "Type must be rooted, use #[must_root] on \
@@ -122,7 +121,7 @@ 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) {
+                block: &hir::Block, span: codemap::Span, id: ast::NodeId) {
         match kind {
             visit::FnKind::ItemFn(n, _, _, _, _, _) |
             visit::FnKind::Method(n, _, _) if n.as_str() == "new"
@@ -145,12 +144,25 @@ impl LateLintPass for UnrootedPass {
         match block.rules {
             hir::DefaultBlock => {
                 for arg in &decl.inputs {
-                    ast_ty_to_prim_ty(cx.tcx, &*arg.ty).map(|t| {
+                    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")
+                        }
+                    });
+                }
             }
             _ => () // fn is `unsafe`
         }

diff --git a/components/plugins/utils.rs b/components/plugins/utils.rs
@@ -3,12 +3,13 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 use rustc::front::map as ast_map;
-use rustc::lint::LateContext;
+use rustc::lint::{LateContext, LintContext};
 use rustc::middle::def;
 use rustc::middle::def_id::DefId;
 use rustc_front::hir;
 use syntax::ast;
 use syntax::attr::mark_used;
+use syntax::codemap::{ExpnFormat, Span};
 use syntax::ptr::P;
 
 
@@ -100,3 +101,16 @@ pub fn match_def_path(cx: &LateContext, def_id: DefId, path: &[&str]) -> bool {
     cx.tcx.with_path(def_id, |iter| iter.map(|elem| elem.name())
         .zip(path.iter()).all(|(nm, p)| &nm.as_str() == p))
 }
+
+pub fn in_derive_expn(cx: &LateContext, span: Span) -> bool {
+    cx.sess().codemap().with_expn_info(span.expn_id,
+            |info| {
+                if let Some(i) = info {
+                    if let ExpnFormat::MacroAttribute(n) = i.callee.format {
+                        if n.as_str().contains("derive") {
+                            true
+                        } else { false }
+                    } else { false }
+                } else { false }
+            })
+}
diff --git a/components/script/dom/bindings/global.rs b/components/script/dom/bindings/global.rs
@@ -210,6 +210,7 @@ impl GlobalRoot {
 
 impl GlobalField {
     /// Create a new `GlobalField` from a rooted reference.
+    #[allow(unrooted_must_root)]
     pub fn from_rooted(global: &GlobalRef) -> GlobalField {
         match *global {
             GlobalRef::Window(window) => GlobalField::Window(JS::from_ref(window)),

diff --git a/components/script/dom/bindings/js.rs b/components/script/dom/bindings/js.rs
@@ -67,12 +67,14 @@ impl<T: Reflectable> JS<T> {
     }
     /// Create a JS<T> from a Root<T>
     /// XXX Not a great API. Should be a call on Root<T> instead
+    #[allow(unrooted_must_root)]
     pub fn from_rooted(root: &Root<T>) -> JS<T> {
         JS {
             ptr: unsafe { NonZero::new(&**root) }
         }
     }
     /// Create a JS<T> from a &T
+    #[allow(unrooted_must_root)]
     pub fn from_ref(obj: &T) -> JS<T> {
         JS {
             ptr: unsafe { NonZero::new(&*obj) }
@@ -125,6 +127,7 @@ impl<T> PartialEq for LayoutJS<T> {
 
 impl <T> Clone for JS<T> {
     #[inline]
+    #[allow(unrooted_must_root)]
     fn clone(&self) -> JS<T> {
         JS {
             ptr: self.ptr.clone()
@@ -288,6 +291,7 @@ impl<T: Reflectable> MutNullableHeap<JS<T>> {
 }
 
 impl<T: HeapGCValue + Copy> Default for MutNullableHeap<T> {
+    #[allow(unrooted_must_root)]
     fn default() -> MutNullableHeap<T> {
         MutNullableHeap {
             ptr: Cell::new(None)

diff --git a/components/script/dom/node.rs b/components/script/dom/node.rs
@@ -1352,10 +1352,12 @@ impl Node {
         Node::new_(flags, Some(doc))
     }
 
+    #[allow(unrooted_must_root)]
     pub fn new_document_node() -> Node {
         Node::new_(NodeFlags::new() | IS_IN_DOC, None)
     }
 
+    #[allow(unrooted_must_root)]
     fn new_(flags: NodeFlags, doc: Option<&Document>) -> Node {
         Node {
             eventtarget: EventTarget::new_inherited(),

diff --git a/components/script/dom/servohtmlparser.rs b/components/script/dom/servohtmlparser.rs
@@ -43,6 +43,7 @@ pub struct Sink {
 }
 
 impl Sink {
+    #[allow(unrooted_must_root)] // method is only run at parse time
     pub fn get_or_create(&self, child: NodeOrText<JS<Node>>) -> Root<Node> {
         match child {
             NodeOrText::AppendNode(n) => n.root(),

diff --git a/components/script/dom/webglrenderingcontext.rs b/components/script/dom/webglrenderingcontext.rs
@@ -147,10 +147,10 @@ impl WebGLRenderingContext {
         }
     }
 
-    pub fn bound_texture_for(&self, target: u32) -> Option<JS<WebGLTexture>> {
+    pub fn bound_texture_for(&self, target: u32) -> Option<Root<WebGLTexture>> {
         match target {
-            constants::TEXTURE_2D => self.bound_texture_2d.get(),
-            constants::TEXTURE_CUBE_MAP => self.bound_texture_cube_map.get(),
+            constants::TEXTURE_2D => self.bound_texture_2d.get().map(|t| t.root()),
+            constants::TEXTURE_CUBE_MAP => self.bound_texture_cube_map.get().map(|t| t.root()),
 
             _ => unreachable!(),
         }
@@ -906,7 +906,6 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
             constants::TEXTURE_2D |
             constants::TEXTURE_CUBE_MAP => {
                 if let Some(texture) = self.bound_texture_for(target) {
-                    let texture = texture.root();
                     let result = texture.r().tex_parameter(target, name, TexParameterValue::Float(value));
                     handle_potential_webgl_error!(self, result);
                 } else {
@@ -924,7 +923,6 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
             constants::TEXTURE_2D |
             constants::TEXTURE_CUBE_MAP => {
                 if let Some(texture) = self.bound_texture_for(target) {
-                    let texture = texture.root();
                     let result = texture.r().tex_parameter(target, name, TexParameterValue::Int(value));
                     handle_potential_webgl_error!(self, result);
                 } else {

diff --git a/components/script/page.rs b/components/script/page.rs
@@ -119,6 +119,7 @@ impl Page {
         old
     }
 
+    #[allow(unrooted_must_root)]
     pub fn set_frame(&self, frame: Option<Frame>) {
         *self.frame.borrow_mut() = frame;
     }