Mirrors/servo
1
0
Fork 0
mirror of https://github.com/servo/servo.git synced 2025-08-06 14:10:11 +01:00
Code Issues Projects Releases Packages Wiki Activity

Auto merge of #6688 - Manishearth:smarter-root-lint, r=jdm

Browse source 
Handle type parameters in unused_must_root

fixes #6651

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6688)
<!-- Reviewable:end -->
This commit is contained in:
bors-servo 2015-07-22 07:05:31 -06:00
commit 8a6681ba70
11 changed files with 118 additions and 57 deletions
Download patch file Download diff file Expand all files Collapse all files

6
components/plugins/lib.rs
View file

@ -27,6 +27,7 @@ use rustc::plugin::Registry;
use syntax::ext::base::*;
use syntax::parse::token::intern;
use syntax::feature_gate::AttributeType::Whitelisted;
// Public for documentation to show up
/// Handles the auto-deriving for `#[derive(JSTraceable)]`
@ -49,10 +50,13 @@ pub fn plugin_registrar(reg: &mut Registry) {
reg.register_macro("to_lower", casing::expand_lower);
reg.register_macro("to_upper", casing::expand_upper);
reg.register_lint_pass(box lints::transmute_type::TransmutePass as LintPassObject);
reg.register_lint_pass(box lints::unrooted_must_root::UnrootedPass as LintPassObject);
reg.register_lint_pass(box lints::unrooted_must_root::UnrootedPass::new() as LintPassObject);
reg.register_lint_pass(box lints::privatize::PrivatizePass as LintPassObject);
reg.register_lint_pass(box lints::inheritance_integrity::InheritancePass as LintPassObject);
reg.register_lint_pass(box lints::str_to_string::StrToStringPass as LintPassObject);
reg.register_lint_pass(box lints::ban::BanPass as LintPassObject);
reg.register_lint_pass(box tenacious::TenaciousPass as LintPassObject);
reg.register_attribute("must_root".to_string(), Whitelisted);
reg.register_attribute("servo_lang".to_string(), Whitelisted);
reg.register_attribute("allow_unrooted_interior".to_string(), Whitelisted);
}

109
components/plugins/lints/unrooted_must_root.rs
View file

@ -6,8 +6,9 @@ use syntax::{ast, codemap, visit};
use syntax::attr::AttrMetaMethods;
use rustc::ast_map;
use rustc::lint::{Context, LintPass, LintArray};
use rustc::middle::{ty, def};
use utils::unsafe_context;
use rustc::middle::ty;
use rustc::middle::astconv_util::ast_ty_to_prim_ty;
use utils::{match_def_path, unsafe_context};
declare_lint!(UNROOTED_MUST_ROOT, Deny,
"Warn and report usage of unrooted jsmanaged objects");
@ -25,27 +26,50 @@ declare_lint!(UNROOTED_MUST_ROOT, Deny,
///
/// This helps catch most situations where pointers like `JS<T>` are used in a way that they can be invalidated by a
/// GC pass.
pub struct UnrootedPass;
///
/// 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
}
// Checks if a type has the #[must_root] annotation.
// Unwraps pointers as well
// TODO (#3874, sort of): unwrap other types like Vec/Option/HashMap/etc
fn lint_unrooted_ty(cx: &Context, ty: &ast::Ty, warning: &str) {
match ty.node {
ast::TyVec(ref t) | ast::TyFixedLengthVec(ref t, _) =>
lint_unrooted_ty(cx, &**t, warning),
ast::TyPath(..) => {
match cx.tcx.def_map.borrow()[&ty.id] {
def::PathResolution{ base_def: def::DefTy(def_id, _), .. } => {
if cx.tcx.has_attr(def_id, "must_root") {
cx.span_lint(UNROOTED_MUST_ROOT, ty.span, warning);
}
}
_ => (),
impl UnrootedPass {
pub fn new() -> UnrootedPass {
UnrootedPass {
in_new_function: true
}
}
}
/// Checks if a type is unrooted or contains any owned unrooted types
fn is_unrooted_ty(cx: &Context, ty: &ty::TyS, in_new_function: bool) -> bool {
let mut ret = false;
ty.maybe_walk(|t| {
match t.sty {
ty::TyStruct(did, _) |
ty::TyEnum(did, _) => {
if cx.tcx.has_attr(did, "must_root") {
ret = true;
false
} else if cx.tcx.has_attr(did, "allow_unrooted_interior") {
false
} else if match_def_path(cx, did, &["core", "cell", "Ref"])
|| match_def_path(cx, did, &["core", "cell", "RefMut"]) {
// Ref and RefMut are borrowed pointers, okay to hold unrooted stuff
// since it will be rooted elsewhere
false
} else {
true
}
}
_ => (),
};
},
ty::TyBox(..) if in_new_function => false, // box in new() is okay
ty::TyRef(..) => false, // don't recurse down &ptrs
ty::TyRawPtr(..) => false, // don't recurse down *ptrs
_ => true
}
});
ret
}
impl LintPass for UnrootedPass {
@ -65,8 +89,10 @@ impl LintPass for UnrootedPass {
};
if item.attrs.iter().all(|a| !a.check_name("must_root")) {
for ref field in def.fields.iter() {
lint_unrooted_ty(cx, &*field.node.ty,
"Type must be rooted, use #[must_root] on the struct definition to propagate");
if is_unrooted_ty(cx, cx.tcx.node_id_to_type(field.node.id), false) {
cx.span_lint(UNROOTED_MUST_ROOT, field.span,
"Type must be rooted, use #[must_root] on the struct definition to propagate")
}
}
}
}
@ -77,8 +103,13 @@ impl LintPass for UnrootedPass {
match var.node.kind {
ast::TupleVariantKind(ref vec) => {
for ty in vec.iter() {
lint_unrooted_ty(cx, &*ty.ty,
"Type must be rooted, use #[must_root] on the enum definition to propagate")
ast_ty_to_prim_ty(cx.tcx, &*ty.ty).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 \
the enum definition to propagate")
}
});
}
}
_ => () // Struct variants already caught by check_struct_def
@ -90,7 +121,10 @@ impl LintPass for UnrootedPass {
block: &ast::Block, _span: codemap::Span, id: ast::NodeId) {
match kind {
visit::FkItemFn(i, _, _, _, _, _) |
visit::FkMethod(i, _, _) if i.as_str() == "new" || i.as_str() == "new_inherited" => {
visit::FkMethod(i, _, _) if i.as_str() == "new"
|| i.as_str() == "new_inherited"
|| i.as_str() == "new_initialized" => {
self.in_new_function = true;
return;
},
visit::FkItemFn(_, _, style, _, _, _) => match style {
@ -99,6 +133,7 @@ impl LintPass for UnrootedPass {
},
_ => ()
}
self.in_new_function = false;
if unsafe_context(&cx.tcx.map, id) {
return;
@ -107,8 +142,11 @@ impl LintPass for UnrootedPass {
match block.rules {
ast::DefaultBlock => {
for arg in decl.inputs.iter() {
lint_unrooted_ty(cx, &*arg.ty,
"Type must be rooted")
ast_ty_to_prim_ty(cx.tcx, &*arg.ty).map(|t| {
if is_unrooted_ty(cx, t, false) {
cx.span_lint(UNROOTED_MUST_ROOT, arg.ty.span, "Type must be rooted")
}
});
}
}
_ => () // fn is `unsafe`
@ -120,7 +158,6 @@ impl LintPass for UnrootedPass {
// 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: &Context, s: &ast::Stmt) {
match s.node {
ast::StmtDecl(_, id) |
ast::StmtExpr(_, id) |
@ -155,16 +192,10 @@ impl LintPass for UnrootedPass {
_ => return
};
let t = cx.tcx.expr_ty(&*expr);
match t.sty {
ty::TyStruct(did, _) |
ty::TyEnum(did, _) => {
if cx.tcx.has_attr(did, "must_root") {
cx.span_lint(UNROOTED_MUST_ROOT, expr.span,
&format!("Expression of type {:?} must be rooted", t));
}
}
_ => {}
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))
}
}
}

14
components/plugins/utils.rs
View file

@ -49,7 +49,11 @@ pub fn match_lang_ty(cx: &Context, ty: &Ty, value: &str) -> bool {
_ => return false,
};
cx.tcx.get_attrs(def_id).iter().any(|attr| {
match_lang_did(cx, def_id, value)
}
pub fn match_lang_did(cx: &Context, did: ast::DefId, value: &str) -> bool {
cx.tcx.get_attrs(did).iter().any(|attr| {
match attr.node.value.node {
ast::MetaNameValue(ref name, ref val) if &**name == "servo_lang" => {
match val.node {
@ -88,3 +92,11 @@ pub fn unsafe_context(map: &ast_map::Map, id: ast::NodeId) -> bool {
// to be added.
}
}
/// check if a DefId's path matches the given absolute type path
/// usage e.g. with
/// `match_def_path(cx, id, &["core", "option", "Option"])`
pub fn match_def_path(cx: &Context, def_id: ast::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))
}

8
components/script/dom/bindings/js.rs
View file

@ -81,6 +81,7 @@ impl<T: Reflectable> JS<T> {
/// An unrooted reference to a DOM object for use in layout. `Layout*Helpers`
/// traits must be implemented on this.
#[allow_unrooted_interior]
pub struct LayoutJS<T> {
ptr: NonZero<*const T>
}
@ -270,6 +271,12 @@ impl<T: Reflectable> MutNullableHeap<JS<T>> {
pub unsafe fn get_inner_as_layout(&self) -> Option<LayoutJS<T>> {
self.ptr.get().map(|js| js.to_layout())
}
/// Get a rooted value out of this object
// FIXME(#6684)
pub fn get_rooted(&self) -> Option<Root<T>> {
self.get().map(|o| o.root())
}
}
impl<T: HeapGCValue+Copy> Default for MutNullableHeap<T> {
@ -382,6 +389,7 @@ pub unsafe fn trace_roots(tracer: *mut JSTracer) {
/// are additive, so this object's destruction will not invalidate other roots
/// for the same JS value. `Root`s cannot outlive the associated
/// `RootCollection` object.
#[allow_unrooted_interior]
pub struct Root<T: Reflectable> {
/// Reference to rooted value that must not outlive this container
ptr: NonZero<*const T>,

1
components/script/dom/bindings/refcounted.rs
View file

@ -57,6 +57,7 @@ unsafe impl Send for TrustedReference {}
/// shared among tasks for use in asynchronous operations. The underlying
/// DOM object is guaranteed to live at least as long as the last outstanding
/// `Trusted<T>` instance.
#[allow_unrooted_interior]
pub struct Trusted<T: Reflectable> {
/// A pointer to the Rust DOM object of type T, but void to allow
/// sending `Trusted<T>` between tasks, regardless of T's sendability.

1
components/script/dom/bindings/trace.rs
View file

@ -455,6 +455,7 @@ impl<'a, T: JSTraceable> Drop for RootedTraceable<'a, T> {
#[allow(unrooted_must_root)]
#[no_move]
#[derive(JSTraceable)]
#[allow_unrooted_interior]
pub struct RootedVec<T: JSTraceable + Reflectable> {
v: Vec<T>
}

3
components/script/dom/bindings/utils.rs
View file

@ -413,8 +413,7 @@ pub fn reflect_dom_object<T: Reflectable>
}
/// A struct to store a reference to the reflector of a DOM object.
// Allowing unused_attribute because the lint sometimes doesn't run in order
#[allow(raw_pointer_derive, unrooted_must_root, unused_attributes)]
#[allow(raw_pointer_derive, unrooted_must_root)]
#[must_root]
#[servo_lang = "reflector"]
// If you're renaming or moving this field, update the path in plugins::reflector as well

1
components/script/dom/browsercontext.rs
View file

@ -30,6 +30,7 @@ use std::default::Default;
#[derive(JSTraceable)]
#[privatize]
#[allow(raw_pointer_derive)]
#[must_root]
pub struct BrowsingContext {
history: Vec<SessionHistoryEntry>,
active_index: usize,

29
components/script/dom/node.rs
View file

@ -328,14 +328,14 @@ impl<'a> PrivateNodeHelpers for &'a Node {
match before {
Some(ref before) => {
assert!(before.parent_node.get().map(Root::from_rooted).r() == Some(self));
match before.prev_sibling.get() {
let prev_sibling = before.GetPreviousSibling();
match prev_sibling {
None => {
assert!(Some(*before) == self.first_child.get().map(Root::from_rooted).r());
self.first_child.set(Some(JS::from_ref(new_child)));
},
Some(ref prev_sibling) => {
let prev_sibling = prev_sibling.root();
prev_sibling.r().next_sibling.set(Some(JS::from_ref(new_child)));
prev_sibling.next_sibling.set(Some(JS::from_ref(new_child)));
new_child.prev_sibling.set(Some(JS::from_ref(prev_sibling.r())));
},
}
@ -343,11 +343,11 @@ impl<'a> PrivateNodeHelpers for &'a Node {
new_child.next_sibling.set(Some(JS::from_ref(before)));
},
None => {
match self.last_child.get() {
let last_child = self.GetLastChild();
match last_child {
None => self.first_child.set(Some(JS::from_ref(new_child))),
Some(ref last_child) => {
let last_child = last_child.root();
assert!(last_child.r().next_sibling.get().is_none());
assert!(last_child.next_sibling.get().is_none());
last_child.r().next_sibling.set(Some(JS::from_ref(new_child)));
new_child.prev_sibling.set(Some(JS::from_rooted(&last_child)));
}
@ -365,22 +365,22 @@ impl<'a> PrivateNodeHelpers for &'a Node {
/// Fails unless `child` is a child of this node.
fn remove_child(self, child: &Node) {
assert!(child.parent_node.get().map(Root::from_rooted).r() == Some(self));
match child.prev_sibling.get() {
let prev_sibling = child.GetPreviousSibling();
match prev_sibling {
None => {
self.first_child.set(child.next_sibling.get());
}
Some(ref prev_sibling) => {
prev_sibling.root().r().next_sibling.set(child.next_sibling.get());
prev_sibling.next_sibling.set(child.next_sibling.get());
}
}
match child.next_sibling.get() {
let next_sibling = child.GetNextSibling();
match next_sibling {
None => {
self.last_child.set(child.prev_sibling.get());
}
Some(ref next_sibling) => {
next_sibling.root().r().prev_sibling.set(child.prev_sibling.get());
next_sibling.prev_sibling.set(child.prev_sibling.get());
}
}
@ -1476,9 +1476,10 @@ impl Node {
// https://dom.spec.whatwg.org/#concept-node-adopt
pub fn adopt(node: &Node, document: &Document) {
// Step 1.
match node.parent_node.get() {
let parent_node = node.GetParentNode();
match parent_node {
Some(ref parent) => {
Node::remove(node, parent.root().r(), SuppressObserver::Unsuppressed);
Node::remove(node, parent, SuppressObserver::Unsuppressed);
}
None => (),
}

1
components/script/page.rs
View file

@ -14,6 +14,7 @@ use std::rc::Rc;
/// Encapsulates a handle to a frame in a frame tree.
#[derive(JSTraceable)]
#[allow(unrooted_must_root)] // FIXME(#6687) this is wrong
pub struct Page {
/// Pipeline id associated with this page.
id: PipelineId,

2
components/script/script_task.rs
View file

@ -287,6 +287,8 @@ impl Drop for StackRootTLS {
/// Information for an entire page. Pages are top-level browsing contexts and can contain multiple
/// frames.
#[derive(JSTraceable)]
// ScriptTask instances are rooted on creation, so this is okay
#[allow(unrooted_must_root)]
pub struct ScriptTask {
/// A handle to the information pertaining to page layout
page: DOMRefCell<Option<Rc<Page>>>,