Add inheritance-checking lint

components/plugins/jstraceable.rs

@@ -14,12 +14,20 @@ use syntax::parse::token::InternedString;
 
 pub fn expand_dom_struct(_: &mut ExtCtxt, _: Span, _: &MetaItem, item: P<Item>) -> P<Item> {
     let mut item2 = (*item).clone();
-    item2.attrs.push(attr::mk_attr_outer(attr::mk_attr_id(), attr::mk_word_item(InternedString::new("must_root"))));
+    {
-    item2.attrs.push(attr::mk_attr_outer(attr::mk_attr_id(), attr::mk_word_item(InternedString::new("privatize"))));
+        let add_attr = |s| {
-    item2.attrs.push(attr::mk_attr_outer(attr::mk_attr_id(), attr::mk_word_item(InternedString::new("jstraceable"))));
+            item2.attrs.push(attr::mk_attr_outer(attr::mk_attr_id(), attr::mk_word_item(InternedString::new(s))));
+        };
+        add_attr("must_root");
+        add_attr("privatize");
+        add_attr("jstraceable");
 
-    // The following attribute is only for internal usage
+        // The following attributes are only for internal usage
-    item2.attrs.push(attr::mk_attr_outer(attr::mk_attr_id(), attr::mk_word_item(InternedString::new("_generate_reflector"))));
+        add_attr("_generate_reflector");
+        // #[dom_struct] gets consumed, so this lets us keep around a residue
+        // Do NOT register a modifier/decorator on this attribute
+        add_attr("_dom_struct_marker");
+    }
     P(item2)
 }
 

components/plugins/lib.rs

@@ -47,5 +47,6 @@ pub fn plugin_registrar(reg: &mut Registry) {
     reg.register_lint_pass(box lints::TransmutePass as LintPassObject);
     reg.register_lint_pass(box lints::UnrootedPass as LintPassObject);
     reg.register_lint_pass(box lints::PrivatizePass as LintPassObject);
+    reg.register_lint_pass(box lints::InheritancePass as LintPassObject);
 }
 

components/plugins/lints.rs

@@ -5,18 +5,22 @@
 use syntax::{ast, ast_map, ast_util, codemap, visit};
 use syntax::ast::Public;
 use syntax::attr::AttrMetaMethods;
-use rustc::lint::{Context, LintPass, LintArray};
+use rustc::lint::{Context, LintPass, LintArray, Level};
 use rustc::middle::ty::expr_ty;
 use rustc::middle::{ty, def};
 use rustc::middle::typeck::astconv::AstConv;
 use rustc::util::ppaux::Repr;
 
+use utils::match_lang_ty;
+
 declare_lint!(TRANSMUTE_TYPE_LINT, Allow,
               "Warn and report types being transmuted")
 declare_lint!(UNROOTED_MUST_ROOT, Deny,
               "Warn and report usage of unrooted jsmanaged objects")
 declare_lint!(PRIVATIZE, Deny,
               "Allows to enforce private fields for struct definitions")
+declare_lint!(INHERITANCE_INTEGRITY, Deny,
+              "Ensures that struct fields are properly laid out for inheritance to work")
 
 /// Lint for auditing transmutes
 ///
@@ -41,6 +45,12 @@ pub struct UnrootedPass;
 /// This lint (disable with `-A privatize`/`#[allow(privatize)]`) ensures all types marked with `#[privatize]` have no private fields
 pub struct PrivatizePass;
 
+/// Lint for ensuring proper layout of DOM structs
+///
+/// A DOM struct must have one Reflector field or one field
+/// which itself is a DOM struct (in which case it must be the first field).
+pub struct InheritancePass;
+
 impl LintPass for TransmutePass {
     fn get_lints(&self) -> LintArray {
         lint_array!(TRANSMUTE_TYPE_LINT)
@@ -251,3 +261,65 @@ impl LintPass for PrivatizePass {
         }
     }
 }
+
+impl LintPass for InheritancePass {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(INHERITANCE_INTEGRITY)
+    }
+
+    fn check_struct_def(&mut self, cx: &Context, def: &ast::StructDef, _i: ast::Ident, _gen: &ast::Generics, id: ast::NodeId) {
+        // Lints are run post expansion, so it's fine to use
+        // #[_dom_struct_marker] here without also checking for #[dom_struct]
+        if ty::has_attr(cx.tcx, ast_util::local_def(id), "_dom_struct_marker") {
+            // Find the reflector, if any
+            let reflector_span = def.fields.iter()
+                                    .find(|f| match_lang_ty(cx, &*f.node.ty, "reflector"))
+                                    .map(|f| f.span);
+            // Find all #[dom_struct] fields
+            let dom_spans: Vec<_> = def.fields.iter().enumerate().filter_map(|(ctr, f)| {
+                if let ast::TyPath(_, _, ty_id) = f.node.ty.node {
+                    if let Some(def::DefTy(def_id, _)) = cx.tcx.def_map.borrow().find_copy(&ty_id) {
+                        if ty::has_attr(cx.tcx, def_id, "_dom_struct_marker") {
+                            // If the field is not the first, it's probably
+                            // being misused (a)
+                            if ctr > 0 {
+                                cx.span_lint(INHERITANCE_INTEGRITY, f.span,
+                                             "Bare DOM structs should only be used as the first field of a \
+                                              DOM struct. Consider using JS<T> instead.");
+                            }
+                            return Some(f.span)
+                        }
+                    }
+                }
+                None
+            }).collect();
+
+            // We should not have both a reflector and a dom struct field
+            if let Some(sp) = reflector_span {
+                if dom_spans.len() > 0 {
+                    cx.span_lint(INHERITANCE_INTEGRITY, cx.tcx.map.expect_item(id).span,
+                                 "This DOM struct has both Reflector and bare DOM struct members");
+                    if cx.current_level(INHERITANCE_INTEGRITY) != Level::Allow {
+                        let sess = cx.sess();
+                        sess.span_note(sp, "Reflector found here");
+                        for span in dom_spans.iter() {
+                            sess.span_note(*span, "Bare DOM struct found here");
+                        }
+                    }
+                }
+            // Nor should we have more than one dom struct field
+            } else if dom_spans.len() > 1 {
+                cx.span_lint(INHERITANCE_INTEGRITY, cx.tcx.map.expect_item(id).span,
+                             "This DOM struct has multiple DOM struct members, only one is allowed");
+                if cx.current_level(INHERITANCE_INTEGRITY) != Level::Allow {
+                    for span in dom_spans.iter() {
+                        cx.sess().span_note(*span, "Bare DOM struct found here");
+                    }
+                }
+            } else if dom_spans.len() == 0 {
+                cx.span_lint(INHERITANCE_INTEGRITY, cx.tcx.map.expect_item(id).span,
+                             "This DOM struct has no reflector or parent DOM struct");                
+            }
+        }
+    }
+}

components/plugins/reflector.rs

@@ -11,9 +11,9 @@ use utils::match_ty_unwrap;
 
 
 pub fn expand_reflector(cx: &mut ExtCtxt, span: Span, _: &MetaItem, item: &Item, push: |P<Item>|) {
-
     if let ast::ItemStruct(ref def, _) = item.node {
         let struct_name = item.ident;
+        // This path has to be hardcoded, unfortunately, since we can't resolve paths at expansion time
         match def.fields.iter().find(|f| match_ty_unwrap(&*f.node.ty, &["dom", "bindings", "utils", "Reflector"]).is_some()) {
             // If it has a field that is a Reflector, use that
             Some(f) => {
@@ -28,10 +28,6 @@ pub fn expand_reflector(cx: &mut ExtCtxt, span: Span, _: &MetaItem, item: &Item,
                 impl_item.map(|it| push(it))
             },
             // Or just call it on the first field (supertype).
-            // TODO: Write a lint to ensure that this first field is indeed a #[dom_struct],
-            // and the only such field in the struct definition (including reflectors)
-            // Unfortunately we can't do it here itself because a def_map (from middle) is not available
-            // at expansion time
             None => {
                 let field_name = def.fields[0].node.ident();
                 let impl_item = quote_item!(cx,
@@ -45,6 +41,6 @@ pub fn expand_reflector(cx: &mut ExtCtxt, span: Span, _: &MetaItem, item: &Item,
             }
         };
     } else {
-        cx.span_bug(span, "#[dom_struct] seems to have been applied to a non-struct");
+        cx.span_err(span, "#[dom_struct] seems to have been applied to a non-struct");
     }
 }

components/plugins/utils.rs

@@ -2,10 +2,17 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+use rustc::lint::Context;
+use rustc::middle::{ty, def};
+
 use syntax::ptr::P;
+use syntax::ast;
 use syntax::ast::{TyPath, Path, AngleBracketedParameters, PathSegment, Ty};
+use syntax::attr::mark_used;
 
 /// Matches a type with a provided string, and returns its type parameters if successful
+///
+/// Try not to use this for types defined in crates you own, use match_lang_ty instead (for lint passes)
 pub fn match_ty_unwrap<'a>(ty: &'a Ty, segments: &[&str]) -> Option<&'a [P<Ty>]> {
     match ty.node {
         TyPath(Path {segments: ref seg, ..}, _, _) => {
@@ -27,3 +34,29 @@ pub fn match_ty_unwrap<'a>(ty: &'a Ty, segments: &[&str]) -> Option<&'a [P<Ty>]>
         _ => None
     }
 }
+
+/// Checks if a type has a #[servo_lang = "str"] attribute
+pub fn match_lang_ty(cx: &Context, ty: &Ty, value: &str) -> bool {
+    let mut found = false;
+    if let TyPath(_, _, ty_id) = ty.node {
+        if let Some(def::DefTy(def_id, _)) = cx.tcx.def_map.borrow().find_copy(&ty_id) {
+            // Iterating through attributes is hard because of cross-crate defs
+            ty::each_attr(cx.tcx, def_id, |attr| {
+                if let ast::MetaNameValue(ref name, ref val) = attr.node.value.node {
+                    if name.get() == "servo_lang" {
+                        if let ast::LitStr(ref v, _) = val.node {
+                            if v.get() == value {
+                                mark_used(attr);
+                                found = true;
+                                // We're done with the loop
+                                return false;
+                            }
+                        }
+                    }
+                }
+                true
+            });
+        };
+    }
+    found
+}

components/script/dom/bindings/utils.rs

@@ -346,9 +346,12 @@ pub fn reflect_dom_object<T: Reflectable>
 }
 
 /// A struct to store a reference to the reflector of a DOM object.
-#[allow(raw_pointer_deriving, unrooted_must_root)]
+// Allowing unused_attribute because the lint sometimes doesn't run in order
+#[allow(raw_pointer_deriving, unrooted_must_root, unused_attributes)]
 #[deriving(PartialEq)]
 #[must_root]
+#[servo_lang = "reflector"]
+// If you're renaming or moving this field, update the path in plugins::reflector as well
 pub struct Reflector {
     object: Cell<*mut JSObject>,
 }

components/script/dom/browsercontext.rs

@@ -65,7 +65,9 @@ impl BrowserContext {
     }
 }
 
-#[dom_struct]
+#[must_root]
+#[privatize]
+#[jstraceable]
 pub struct SessionHistoryEntry {
     document: JS<Document>,
     children: Vec<BrowserContext>

components/script/dom/errorevent.rs

@@ -12,7 +12,7 @@ use dom::bindings::js::{JSRef, Temporary, MutHeap};
 use js::jsapi::JSContext;
 use dom::bindings::trace::JSTraceable;
 
-use dom::bindings::utils::{Reflectable, Reflector, reflect_dom_object};
+use dom::bindings::utils::reflect_dom_object;
 use dom::event::{Event, EventTypeId, EventBubbles, EventCancelable};
 use servo_util::str::DOMString;
 
@@ -127,9 +127,3 @@ impl<'a> ErrorEventMethods for JSRef<'a, ErrorEvent> {
     }
 
 }
-
-impl Reflectable for ErrorEvent {
-    fn reflector<'a>(&'a self) -> &'a Reflector {
-        self.event.reflector()
-    }
-}

components/script/dom/event.rs

@@ -240,3 +240,12 @@ impl<'a> EventMethods for JSRef<'a, Event> {
     }
 }
 
+pub trait EventHelpers {
+    fn set_trusted(self, trusted: bool);
+}
+
+impl<'a> EventHelpers for JSRef<'a, Event> {
+    fn set_trusted(self, trusted: bool) {
+        self.trusted.set(trusted);
+    }
+}