From 47c269360cebba4857433bc2ef4dead70b42a01a Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Sun, 29 Sep 2019 23:11:40 +0200 Subject: [PATCH 1/2] Remove redundant webidl_must_inherit compiler plugin lint At first I was considering moving it to a procedural macro (source-level information should be sufficient), and started by trying to reproduce the error case by changing `htmldivelement.rs` to use `Element` instead of `HTMLElement` as the first field. The output was: ```rust error[E0308]: mismatched types --> /home/simon/servo2/target/debug/build/script-4caa244faca7d10f/out/Bindings/HTMLDivElementBinding.rs:665:31 | 665 | let _: &HTMLElement = self.as_parent(); | ^^^^^^^^^^^^^^^^ expected struct `dom::htmlelement::HTMLElement`, found struct `dom::element::Element` | = note: expected type `&dom::htmlelement::HTMLElement` found type `&dom::element::Element` ``` This line number is inside a generated method called `__assert_parent_type`. As far as I can tell, any case where this lint would error is already caught by such methods. The lint is therefore redundant and can safely be removed. --- Cargo.lock | 12 - components/dom_struct/lib.rs | 1 - components/script/Cargo.toml | 3 +- components/script_plugins/Cargo.toml | 4 - components/script_plugins/lib.rs | 12 - .../script_plugins/webidl_must_inherit.rs | 210 ------------------ 6 files changed, 1 insertion(+), 241 deletions(-) delete mode 100644 components/script_plugins/webidl_must_inherit.rs diff --git a/Cargo.lock b/Cargo.lock index f019a379a12..bb75e1713ff 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3992,9 +3992,6 @@ dependencies = [ [[package]] name = "script_plugins" version = "0.0.1" -dependencies = [ - "weedle 0.10.0 (registry+https://github.com/rust-lang/crates.io-index)", -] [[package]] name = "script_plugins_tests" @@ -5673,14 +5670,6 @@ dependencies = [ "serde 1.0.88 (registry+https://github.com/rust-lang/crates.io-index)", ] -[[package]] -name = "weedle" -version = "0.10.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -dependencies = [ - "nom 4.1.1 (registry+https://github.com/rust-lang/crates.io-index)", -] - [[package]] name = "which" version = "3.0.0" @@ -6316,7 +6305,6 @@ dependencies = [ "checksum webrender_build 0.0.1 (git+https://github.com/servo/webrender)" = "" "checksum webxr 0.0.1 (git+https://github.com/servo/webxr)" = "" "checksum webxr-api 0.0.1 (git+https://github.com/servo/webxr)" = "" -"checksum weedle 0.10.0 (registry+https://github.com/rust-lang/crates.io-index)" = "3bb43f70885151e629e2a19ce9e50bd730fd436cfd4b666894c9ce4de9141164" "checksum which 3.0.0 (registry+https://github.com/rust-lang/crates.io-index)" = "240a31163872f7e8e49f35b42b58485e35355b07eb009d9f3686733541339a69" "checksum winapi 0.3.7 (registry+https://github.com/rust-lang/crates.io-index)" = "f10e386af2b13e47c89e7236a7a14a086791a2b88ebad6df9bf42040195cf770" "checksum winapi-i686-pc-windows-gnu 0.4.0 (registry+https://github.com/rust-lang/crates.io-index)" = "ac3b87c63620426dd9b991e5ce0329eff545bccbbb34f3be09ff6fb6ab51b7b6" diff --git a/components/dom_struct/lib.rs b/components/dom_struct/lib.rs index f316a6f22bb..e7b2b290ce5 100644 --- a/components/dom_struct/lib.rs +++ b/components/dom_struct/lib.rs @@ -19,7 +19,6 @@ pub fn dom_struct(args: TokenStream, input: TokenStream) -> TokenStream { #[derive(DenyPublicFields, DomObject, JSTraceable, MallocSizeOf)] #[must_root] #[repr(C)] - #[webidl] }; // Work around https://github.com/rust-lang/rust/issues/46489 diff --git a/components/script/Cargo.toml b/components/script/Cargo.toml index 6eec808b8df..5c9fa86e95f 100644 --- a/components/script/Cargo.toml +++ b/components/script/Cargo.toml @@ -16,8 +16,7 @@ path = "lib.rs" debugmozjs = ['js/debugmozjs'] profilemozjs = ['js/profilemozjs'] unrooted_must_root_lint = ["script_plugins/unrooted_must_root_lint"] -webidl_lint = ["script_plugins/webidl_lint"] -default = ["unrooted_must_root_lint", "webidl_lint"] +default = ["unrooted_must_root_lint"] webgl_backtrace = ["backtrace", "canvas_traits/webgl_backtrace"] js_backtrace = ["backtrace"] uwp = ["js/uwp"] diff --git a/components/script_plugins/Cargo.toml b/components/script_plugins/Cargo.toml index f1537b655a0..c6bc3eb2250 100644 --- a/components/script_plugins/Cargo.toml +++ b/components/script_plugins/Cargo.toml @@ -11,7 +11,3 @@ plugin = true [features] unrooted_must_root_lint = [] -webidl_lint = [] - -[dependencies] -weedle = "0.10" diff --git a/components/script_plugins/lib.rs b/components/script_plugins/lib.rs index cdebd231092..aa45098befe 100644 --- a/components/script_plugins/lib.rs +++ b/components/script_plugins/lib.rs @@ -24,8 +24,6 @@ extern crate rustc; extern crate rustc_driver; extern crate syntax; -extern crate weedle; - use rustc_driver::plugin::Registry; use syntax::feature_gate::AttributeType::Whitelisted; use syntax::symbol::Symbol; @@ -33,9 +31,6 @@ use syntax::symbol::Symbol; #[cfg(feature = "unrooted_must_root_lint")] mod unrooted_must_root; -#[cfg(feature = "webidl_lint")] -mod webidl_must_inherit; - /// Utilities for writing plugins #[cfg(feature = "unrooted_must_root_lint")] mod utils; @@ -49,15 +44,9 @@ pub fn plugin_registrar(reg: &mut Registry) { symbols.clone(), ))); - #[cfg(feature = "webidl_lint")] - reg.register_late_lint_pass(Box::new(webidl_must_inherit::WebIdlPass::new( - symbols.clone(), - ))); - reg.register_attribute(symbols.allow_unrooted_interior, Whitelisted); reg.register_attribute(symbols.allow_unrooted_in_rc, Whitelisted); reg.register_attribute(symbols.must_root, Whitelisted); - reg.register_attribute(symbols.webidl, Whitelisted); } macro_rules! symbols { @@ -82,7 +71,6 @@ symbols! { allow_unrooted_interior allow_unrooted_in_rc must_root - webidl alloc rc Rc diff --git a/components/script_plugins/webidl_must_inherit.rs b/components/script_plugins/webidl_must_inherit.rs deleted file mode 100644 index 95d8ad386a9..00000000000 --- a/components/script_plugins/webidl_must_inherit.rs +++ /dev/null @@ -1,210 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ - -use rustc::hir; -use rustc::lint::{LateContext, LateLintPass, LintArray, LintContext, LintPass}; -use rustc::ty; -use std::boxed; -use std::env; -use std::error::Error; -use std::fmt; -use std::fs; -use std::io; -use std::path; -use weedle; - -declare_lint!( - WEBIDL_INHERIT_CORRECT, - Deny, - "Warn and report usage of incorrect webidl inheritance" -); - -pub(crate) struct WebIdlPass { - symbols: crate::Symbols, -} - -#[derive(Clone, Debug)] -pub struct ParentMismatchError { - name: String, - rust_parent: String, - webidl_parent: String, -} - -impl fmt::Display for ParentMismatchError { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - write!( - f, - "webidl-rust inheritance mismatch, rust: {:?}, rust parent: {:?}, webidl parent: {:?}", - self.name, self.rust_parent, self.webidl_parent - ) - } -} - -impl Error for ParentMismatchError { - fn description(&self) -> &str { - "ParentMismatchError" - } - - fn cause(&self) -> Option<&dyn Error> { - None - } -} - -impl WebIdlPass { - pub fn new(symbols: crate::Symbols) -> WebIdlPass { - WebIdlPass { symbols } - } -} - -fn get_ty_name(ty: &str) -> &str { - if let Some(i) = ty.rfind(':') { - &ty[i + 1..] - } else { - ty - } -} - -fn get_manifest_dir() -> io::Result { - match env::var("CARGO_MANIFEST_DIR") { - Ok(var) => { - let mut dir = path::PathBuf::new(); - dir.push(var); - Ok(dir) - }, - Err(env::VarError::NotPresent) => Err(io::Error::new( - io::ErrorKind::NotFound, - "CARGO_MANIFEST_DIR environment variable was not found", - )), - Err(env::VarError::NotUnicode(_)) => Err(io::Error::new( - io::ErrorKind::InvalidData, - "CARGO_MANIFEST_DIR environment variable's contents are non valid UTF-8", - )), - } -} - -fn get_webidl_path(struct_name: &str) -> io::Result { - let mut dir = get_manifest_dir()?; - dir.push("dom/webidls/"); - dir.push(format!("{}.webidl", struct_name)); - - Ok(dir) -} - -fn is_webidl_ty(symbols: &crate::Symbols, cx: &LateContext, ty: &ty::TyS) -> bool { - let mut ret = false; - ty.maybe_walk(|t| { - match t.kind { - ty::Adt(did, _substs) => { - if cx.tcx.has_attr(did.did, symbols.webidl) { - ret = true; - } - false - }, - ty::Ref(..) => false, // don't recurse down &ptrs - ty::RawPtr(..) => false, // don't recurse down *ptrs - ty::FnDef(..) | ty::FnPtr(_) => false, - _ => true, - } - }); - ret -} - -fn check_inherits(code: &str, name: &str, parent_name: &str) -> Result<(), Box> { - let idl = weedle::parse(code).expect("Invalid webidl provided"); - let mut inherits = ""; - - for def in idl { - if let weedle::Definition::Interface(def) = def { - if let Some(parent) = def.inheritance { - inherits = parent.identifier.0; - break; - } - } else if let weedle::Definition::CallbackInterface(def) = def { - if let Some(parent) = def.inheritance { - inherits = parent.identifier.0; - break; - } - } - } - - if inherits == parent_name { - return Ok(()); - } - - // If there is no parent, first field must be of type Reflector. - if inherits == "" && parent_name == "Reflector" { - return Ok(()); - } - - if inherits == "" && - name == "PaintRenderingContext2D" && - parent_name == "CanvasRenderingContext2D" - { - // PaintRenderingContext2D embeds a CanvasRenderingContext2D - // instead of a Reflector as an optimization, - // but this is fine since CanvasRenderingContext2D - // also has a reflector - return Ok(()); - } - - Err(boxed::Box::from(ParentMismatchError { - name: name.to_string(), - rust_parent: parent_name.to_string(), - webidl_parent: inherits.to_string(), - })) -} - -fn check_webidl(name: &str, parent_name: &Option) -> Result<(), Box> { - let path = get_webidl_path(&name)?; - if let Some(parent) = parent_name { - let code = fs::read_to_string(path)?; - return check_inherits(&code, name, &parent); - } - - Ok(()) -} - -impl LintPass for WebIdlPass { - fn name(&self) -> &'static str { - "ServoWebIDLPass" - } - - fn get_lints(&self) -> LintArray { - lint_array!(WEBIDL_INHERIT_CORRECT) - } -} - -impl<'a, 'tcx> LateLintPass<'a, 'tcx> for WebIdlPass { - fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::Item) { - let def = match &item.kind { - hir::ItemKind::Struct(def, ..) => def, - _ => return, - }; - let id = item.hir_id; - let def_id = cx.tcx.hir().local_def_id(id); - if !is_webidl_ty(&self.symbols, cx, cx.tcx.type_of(def_id)) { - return; - } - - let item = match cx.tcx.hir().get(id) { - hir::Node::Item(item) => item, - _ => cx.tcx.hir().expect_item(cx.tcx.hir().get_parent_item(id)), - }; - - let parent_name = def.fields().iter().next().map(|field| { - let def_id = cx.tcx.hir().local_def_id(field.hir_id); - let ty = cx.tcx.type_of(def_id).to_string(); - get_ty_name(&ty).to_string() - }); - - let struct_name = item.ident.to_string(); - match check_webidl(&struct_name, &parent_name) { - Ok(()) => {}, - Err(e) => { - let description = format!("{}", e); - cx.span_lint(WEBIDL_INHERIT_CORRECT, item.ident.span, &description) - }, - }; - } -} From 81b7f046bfbc9b51a4a2b81e4640f39c03255223 Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Sun, 29 Sep 2019 23:20:48 +0200 Subject: [PATCH 2/2] Move script_plugins to a single file --- components/script_plugins/lib.rs | 352 +++++++++++++++++- .../script_plugins/unrooted_must_root.rs | 312 ---------------- components/script_plugins/utils.rs | 38 -- 3 files changed, 337 insertions(+), 365 deletions(-) delete mode 100644 components/script_plugins/unrooted_must_root.rs delete mode 100644 components/script_plugins/utils.rs diff --git a/components/script_plugins/lib.rs b/components/script_plugins/lib.rs index aa45098befe..ea00a58848d 100644 --- a/components/script_plugins/lib.rs +++ b/components/script_plugins/lib.rs @@ -17,36 +17,358 @@ #![feature(plugin)] #![feature(plugin_registrar)] #![feature(rustc_private)] +#![cfg(feature = "unrooted_must_root_lint")] -#[cfg(feature = "unrooted_must_root_lint")] #[macro_use] extern crate rustc; extern crate rustc_driver; extern crate syntax; +use rustc::hir::def_id::DefId; +use rustc::hir::intravisit as visit; +use rustc::hir::{self, ExprKind, HirId}; +use rustc::lint::{LateContext, LateLintPass, LintArray, LintContext, LintPass}; +use rustc::ty; use rustc_driver::plugin::Registry; use syntax::feature_gate::AttributeType::Whitelisted; +use syntax::source_map; +use syntax::source_map::{ExpnKind, MacroKind, Span}; +use syntax::symbol::sym; use syntax::symbol::Symbol; -#[cfg(feature = "unrooted_must_root_lint")] -mod unrooted_must_root; - -/// Utilities for writing plugins -#[cfg(feature = "unrooted_must_root_lint")] -mod utils; - #[plugin_registrar] pub fn plugin_registrar(reg: &mut Registry) { - let symbols = crate::Symbols::new(); - - #[cfg(feature = "unrooted_must_root_lint")] - reg.register_late_lint_pass(Box::new(unrooted_must_root::UnrootedPass::new( - symbols.clone(), - ))); - + let symbols = Symbols::new(); reg.register_attribute(symbols.allow_unrooted_interior, Whitelisted); reg.register_attribute(symbols.allow_unrooted_in_rc, Whitelisted); reg.register_attribute(symbols.must_root, Whitelisted); + reg.register_late_lint_pass(Box::new(UnrootedPass::new(symbols))); +} + +declare_lint!( + UNROOTED_MUST_ROOT, + Deny, + "Warn and report usage of unrooted jsmanaged objects" +); + +/// Lint for ensuring safe usage of unrooted pointers +/// +/// This lint (disable with `-A unrooted-must-root`/`#[allow(unrooted_must_root)]`) ensures that `#[must_root]` +/// values are used correctly. +/// +/// "Incorrect" usage includes: +/// +/// - Not being used in a struct/enum field which is not `#[must_root]` itself +/// - Not being used as an argument to a function (Except onces named `new` and `new_inherited`) +/// - Not being bound locally in a `let` statement, assignment, `for` loop, or `match` statement. +/// +/// This helps catch most situations where pointers like `JS` are used in a way that they can be invalidated by a +/// GC pass. +/// +/// Structs which have their own mechanism of rooting their unrooted contents (e.g. `ScriptThread`) +/// can be marked as `#[allow(unrooted_must_root)]`. Smart pointers which root their interior type +/// can be marked as `#[allow_unrooted_interior]` +pub(crate) struct UnrootedPass { + symbols: Symbols, +} + +impl UnrootedPass { + pub fn new(symbols: Symbols) -> UnrootedPass { + UnrootedPass { symbols } + } +} + +/// Checks if a type is unrooted or contains any owned unrooted types +fn is_unrooted_ty(sym: &Symbols, cx: &LateContext, ty: &ty::TyS, in_new_function: bool) -> bool { + let mut ret = false; + ty.maybe_walk(|t| { + match t.kind { + ty::Adt(did, substs) => { + if cx.tcx.has_attr(did.did, sym.must_root) { + ret = true; + false + } else if cx.tcx.has_attr(did.did, sym.allow_unrooted_interior) { + false + } else if match_def_path(cx, did.did, &[sym.alloc, sym.rc, sym.Rc]) { + // Rc is okay + let inner = substs.type_at(0); + if let ty::Adt(did, _) = inner.kind { + if cx.tcx.has_attr(did.did, sym.allow_unrooted_in_rc) { + false + } else { + true + } + } else { + true + } + } else if match_def_path(cx, did.did, &[sym::core, sym.cell, sym.Ref]) || + match_def_path(cx, did.did, &[sym::core, sym.cell, sym.RefMut]) || + match_def_path(cx, did.did, &[sym::core, sym.slice, sym.Iter]) || + match_def_path(cx, did.did, &[sym::core, sym.slice, sym.IterMut]) || + match_def_path( + cx, + did.did, + &[sym::std, sym.collections, sym.hash, sym.map, sym.Entry], + ) || + match_def_path( + cx, + did.did, + &[ + sym::std, + sym.collections, + sym.hash, + sym.map, + sym.OccupiedEntry, + ], + ) || + match_def_path( + cx, + did.did, + &[ + sym::std, + sym.collections, + sym.hash, + sym.map, + sym.VacantEntry, + ], + ) || + match_def_path( + cx, + did.did, + &[sym::std, sym.collections, sym.hash, sym.map, sym.Iter], + ) || + match_def_path( + cx, + did.did, + &[sym::std, sym.collections, sym.hash, sym.set, sym.Iter], + ) + { + // Structures which are semantically similar to an &ptr. + false + } else if did.is_box() && in_new_function { + // box in new() is okay + false + } else { + true + } + }, + ty::Ref(..) => false, // don't recurse down &ptrs + ty::RawPtr(..) => false, // don't recurse down *ptrs + ty::FnDef(..) | ty::FnPtr(_) => false, + _ => true, + } + }); + ret +} + +impl LintPass for UnrootedPass { + fn name(&self) -> &'static str { + "ServoUnrootedPass" + } + + fn get_lints(&self) -> LintArray { + lint_array!(UNROOTED_MUST_ROOT) + } +} + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnrootedPass { + /// All structs containing #[must_root] types must be #[must_root] themselves + fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::Item) { + if item + .attrs + .iter() + .any(|a| a.check_name(self.symbols.must_root)) + { + return; + } + if let hir::ItemKind::Struct(def, ..) = &item.kind { + for ref field in def.fields() { + let def_id = cx.tcx.hir().local_def_id(field.hir_id); + if is_unrooted_ty(&self.symbols, cx, cx.tcx.type_of(def_id), false) { + cx.span_lint(UNROOTED_MUST_ROOT, field.span, + "Type must be rooted, use #[must_root] on the struct definition to propagate") + } + } + } + } + + /// All enums containing #[must_root] types must be #[must_root] themselves + fn check_variant(&mut self, cx: &LateContext, var: &hir::Variant) { + let ref map = cx.tcx.hir(); + if map + .expect_item(map.get_parent_item(var.id)) + .attrs + .iter() + .all(|a| !a.check_name(self.symbols.must_root)) + { + match var.data { + hir::VariantData::Tuple(ref fields, ..) => { + for ref field in fields { + let def_id = cx.tcx.hir().local_def_id(field.hir_id); + if is_unrooted_ty(&self.symbols, cx, cx.tcx.type_of(def_id), false) { + cx.span_lint( + UNROOTED_MUST_ROOT, + field.ty.span, + "Type must be rooted, use #[must_root] on \ + the enum definition to propagate", + ) + } + } + }, + _ => (), // Struct variants already caught by check_struct_def + } + } + } + /// Function arguments that are #[must_root] types are not allowed + fn check_fn( + &mut self, + cx: &LateContext<'a, 'tcx>, + kind: visit::FnKind<'tcx>, + decl: &'tcx hir::FnDecl, + body: &'tcx hir::Body, + span: source_map::Span, + id: HirId, + ) { + let in_new_function = match kind { + visit::FnKind::ItemFn(n, _, _, _, _) | visit::FnKind::Method(n, _, _, _) => { + &*n.as_str() == "new" || n.as_str().starts_with("new_") + }, + visit::FnKind::Closure(_) => return, + }; + + if !in_derive_expn(span) { + let def_id = cx.tcx.hir().local_def_id(id); + let sig = cx.tcx.type_of(def_id).fn_sig(cx.tcx); + + for (arg, ty) in decl.inputs.iter().zip(sig.inputs().skip_binder().iter()) { + if is_unrooted_ty(&self.symbols, cx, ty, false) { + cx.span_lint(UNROOTED_MUST_ROOT, arg.span, "Type must be rooted") + } + } + + if !in_new_function { + if is_unrooted_ty(&self.symbols, cx, sig.output().skip_binder(), false) { + cx.span_lint( + UNROOTED_MUST_ROOT, + decl.output.span(), + "Type must be rooted", + ) + } + } + } + + let mut visitor = FnDefVisitor { + symbols: &self.symbols, + cx: cx, + in_new_function: in_new_function, + }; + visit::walk_expr(&mut visitor, &body.value); + } +} + +struct FnDefVisitor<'a, 'b: 'a, 'tcx: 'a + 'b> { + symbols: &'a Symbols, + cx: &'a LateContext<'b, 'tcx>, + in_new_function: bool, +} + +impl<'a, 'b, 'tcx> visit::Visitor<'tcx> for FnDefVisitor<'a, 'b, 'tcx> { + fn visit_expr(&mut self, expr: &'tcx hir::Expr) { + let cx = self.cx; + + let require_rooted = |cx: &LateContext, in_new_function: bool, subexpr: &hir::Expr| { + let ty = cx.tables.expr_ty(&subexpr); + if is_unrooted_ty(&self.symbols, cx, ty, in_new_function) { + cx.span_lint( + UNROOTED_MUST_ROOT, + subexpr.span, + &format!("Expression of type {:?} must be rooted", ty), + ) + } + }; + + match expr.kind { + // Trait casts from #[must_root] types are not allowed + ExprKind::Cast(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`. + // 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> and RootedVec>. + // 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); + } + + fn visit_pat(&mut self, pat: &'tcx hir::Pat) { + let cx = self.cx; + + // We want to detect pattern bindings that move a value onto the stack. + // When "default binding modes" https://github.com/rust-lang/rust/issues/42640 + // are implemented, the `Unannotated` case could cause false-positives. + // These should be fixable by adding an explicit `ref`. + match pat.kind { + hir::PatKind::Binding(hir::BindingAnnotation::Unannotated, ..) | + hir::PatKind::Binding(hir::BindingAnnotation::Mutable, ..) => { + let ty = cx.tables.pat_ty(pat); + if is_unrooted_ty(&self.symbols, cx, ty, self.in_new_function) { + cx.span_lint( + UNROOTED_MUST_ROOT, + pat.span, + &format!("Expression of type {:?} must be rooted", ty), + ) + } + }, + _ => {}, + } + + visit::walk_pat(self, pat); + } + + fn visit_ty(&mut self, _: &'tcx hir::Ty) {} + + fn nested_visit_map<'this>(&'this mut self) -> hir::intravisit::NestedVisitorMap<'this, 'tcx> { + hir::intravisit::NestedVisitorMap::OnlyBodies(&self.cx.tcx.hir()) + } +} + +/// check if a DefId's path matches the given absolute type path +/// usage e.g. with +/// `match_def_path(cx, id, &["core", "option", "Option"])` +fn match_def_path(cx: &LateContext, def_id: DefId, path: &[Symbol]) -> bool { + let krate = &cx.tcx.crate_name(def_id.krate); + if krate != &path[0] { + return false; + } + + let path = &path[1..]; + let other = cx.tcx.def_path(def_id).data; + + if other.len() != path.len() { + return false; + } + + other + .into_iter() + .zip(path) + .all(|(e, p)| e.data.as_interned_str().as_symbol() == *p) +} + +fn in_derive_expn(span: Span) -> bool { + if let ExpnKind::Macro(MacroKind::Attr, n) = span.ctxt().outer_expn_data().kind { + n.as_str().contains("derive") + } else { + false + } } macro_rules! symbols { diff --git a/components/script_plugins/unrooted_must_root.rs b/components/script_plugins/unrooted_must_root.rs deleted file mode 100644 index c0587d8497a..00000000000 --- a/components/script_plugins/unrooted_must_root.rs +++ /dev/null @@ -1,312 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ - -use crate::utils::{in_derive_expn, match_def_path}; -use rustc::hir::intravisit as visit; -use rustc::hir::{self, ExprKind, HirId}; -use rustc::lint::{LateContext, LateLintPass, LintArray, LintContext, LintPass}; -use rustc::ty; -use syntax::source_map; -use syntax::symbol::sym; - -declare_lint!( - UNROOTED_MUST_ROOT, - Deny, - "Warn and report usage of unrooted jsmanaged objects" -); - -/// Lint for ensuring safe usage of unrooted pointers -/// -/// This lint (disable with `-A unrooted-must-root`/`#[allow(unrooted_must_root)]`) ensures that `#[must_root]` -/// values are used correctly. -/// -/// "Incorrect" usage includes: -/// -/// - Not being used in a struct/enum field which is not `#[must_root]` itself -/// - Not being used as an argument to a function (Except onces named `new` and `new_inherited`) -/// - Not being bound locally in a `let` statement, assignment, `for` loop, or `match` statement. -/// -/// This helps catch most situations where pointers like `JS` are used in a way that they can be invalidated by a -/// GC pass. -/// -/// Structs which have their own mechanism of rooting their unrooted contents (e.g. `ScriptThread`) -/// can be marked as `#[allow(unrooted_must_root)]`. Smart pointers which root their interior type -/// can be marked as `#[allow_unrooted_interior]` -pub(crate) struct UnrootedPass { - symbols: crate::Symbols, -} - -impl UnrootedPass { - pub fn new(symbols: crate::Symbols) -> UnrootedPass { - UnrootedPass { symbols } - } -} - -/// Checks if a type is unrooted or contains any owned unrooted types -fn is_unrooted_ty( - sym: &crate::Symbols, - cx: &LateContext, - ty: &ty::TyS, - in_new_function: bool, -) -> bool { - let mut ret = false; - ty.maybe_walk(|t| { - match t.kind { - ty::Adt(did, substs) => { - if cx.tcx.has_attr(did.did, sym.must_root) { - ret = true; - false - } else if cx.tcx.has_attr(did.did, sym.allow_unrooted_interior) { - false - } else if match_def_path(cx, did.did, &[sym.alloc, sym.rc, sym.Rc]) { - // Rc is okay - let inner = substs.type_at(0); - if let ty::Adt(did, _) = inner.kind { - if cx.tcx.has_attr(did.did, sym.allow_unrooted_in_rc) { - false - } else { - true - } - } else { - true - } - } else if match_def_path(cx, did.did, &[sym::core, sym.cell, sym.Ref]) || - match_def_path(cx, did.did, &[sym::core, sym.cell, sym.RefMut]) || - match_def_path(cx, did.did, &[sym::core, sym.slice, sym.Iter]) || - match_def_path(cx, did.did, &[sym::core, sym.slice, sym.IterMut]) || - match_def_path( - cx, - did.did, - &[sym::std, sym.collections, sym.hash, sym.map, sym.Entry], - ) || - match_def_path( - cx, - did.did, - &[ - sym::std, - sym.collections, - sym.hash, - sym.map, - sym.OccupiedEntry, - ], - ) || - match_def_path( - cx, - did.did, - &[ - sym::std, - sym.collections, - sym.hash, - sym.map, - sym.VacantEntry, - ], - ) || - match_def_path( - cx, - did.did, - &[sym::std, sym.collections, sym.hash, sym.map, sym.Iter], - ) || - match_def_path( - cx, - did.did, - &[sym::std, sym.collections, sym.hash, sym.set, sym.Iter], - ) - { - // Structures which are semantically similar to an &ptr. - false - } else if did.is_box() && in_new_function { - // box in new() is okay - false - } else { - true - } - }, - ty::Ref(..) => false, // don't recurse down &ptrs - ty::RawPtr(..) => false, // don't recurse down *ptrs - ty::FnDef(..) | ty::FnPtr(_) => false, - _ => true, - } - }); - ret -} - -impl LintPass for UnrootedPass { - fn name(&self) -> &'static str { - "ServoUnrootedPass" - } - - fn get_lints(&self) -> LintArray { - lint_array!(UNROOTED_MUST_ROOT) - } -} - -impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnrootedPass { - /// All structs containing #[must_root] types must be #[must_root] themselves - fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx hir::Item) { - if item - .attrs - .iter() - .any(|a| a.check_name(self.symbols.must_root)) - { - return; - } - if let hir::ItemKind::Struct(def, ..) = &item.kind { - for ref field in def.fields() { - let def_id = cx.tcx.hir().local_def_id(field.hir_id); - if is_unrooted_ty(&self.symbols, cx, cx.tcx.type_of(def_id), false) { - cx.span_lint(UNROOTED_MUST_ROOT, field.span, - "Type must be rooted, use #[must_root] on the struct definition to propagate") - } - } - } - } - - /// All enums containing #[must_root] types must be #[must_root] themselves - fn check_variant(&mut self, cx: &LateContext, var: &hir::Variant) { - let ref map = cx.tcx.hir(); - if map - .expect_item(map.get_parent_item(var.id)) - .attrs - .iter() - .all(|a| !a.check_name(self.symbols.must_root)) - { - match var.data { - hir::VariantData::Tuple(ref fields, ..) => { - for ref field in fields { - let def_id = cx.tcx.hir().local_def_id(field.hir_id); - if is_unrooted_ty(&self.symbols, cx, cx.tcx.type_of(def_id), false) { - cx.span_lint( - UNROOTED_MUST_ROOT, - field.ty.span, - "Type must be rooted, use #[must_root] on \ - the enum definition to propagate", - ) - } - } - }, - _ => (), // Struct variants already caught by check_struct_def - } - } - } - /// Function arguments that are #[must_root] types are not allowed - fn check_fn( - &mut self, - cx: &LateContext<'a, 'tcx>, - kind: visit::FnKind<'tcx>, - decl: &'tcx hir::FnDecl, - body: &'tcx hir::Body, - span: source_map::Span, - id: HirId, - ) { - let in_new_function = match kind { - visit::FnKind::ItemFn(n, _, _, _, _) | visit::FnKind::Method(n, _, _, _) => { - &*n.as_str() == "new" || n.as_str().starts_with("new_") - }, - visit::FnKind::Closure(_) => return, - }; - - if !in_derive_expn(span) { - let def_id = cx.tcx.hir().local_def_id(id); - let sig = cx.tcx.type_of(def_id).fn_sig(cx.tcx); - - for (arg, ty) in decl.inputs.iter().zip(sig.inputs().skip_binder().iter()) { - if is_unrooted_ty(&self.symbols, cx, ty, false) { - cx.span_lint(UNROOTED_MUST_ROOT, arg.span, "Type must be rooted") - } - } - - if !in_new_function { - if is_unrooted_ty(&self.symbols, cx, sig.output().skip_binder(), false) { - cx.span_lint( - UNROOTED_MUST_ROOT, - decl.output.span(), - "Type must be rooted", - ) - } - } - } - - let mut visitor = FnDefVisitor { - symbols: &self.symbols, - cx: cx, - in_new_function: in_new_function, - }; - visit::walk_expr(&mut visitor, &body.value); - } -} - -struct FnDefVisitor<'a, 'b: 'a, 'tcx: 'a + 'b> { - symbols: &'a crate::Symbols, - cx: &'a LateContext<'b, 'tcx>, - in_new_function: bool, -} - -impl<'a, 'b, 'tcx> visit::Visitor<'tcx> for FnDefVisitor<'a, 'b, 'tcx> { - fn visit_expr(&mut self, expr: &'tcx hir::Expr) { - let cx = self.cx; - - let require_rooted = |cx: &LateContext, in_new_function: bool, subexpr: &hir::Expr| { - let ty = cx.tables.expr_ty(&subexpr); - if is_unrooted_ty(&self.symbols, cx, ty, in_new_function) { - cx.span_lint( - UNROOTED_MUST_ROOT, - subexpr.span, - &format!("Expression of type {:?} must be rooted", ty), - ) - } - }; - - match expr.kind { - // Trait casts from #[must_root] types are not allowed - ExprKind::Cast(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`. - // 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> and RootedVec>. - // 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); - } - - fn visit_pat(&mut self, pat: &'tcx hir::Pat) { - let cx = self.cx; - - // We want to detect pattern bindings that move a value onto the stack. - // When "default binding modes" https://github.com/rust-lang/rust/issues/42640 - // are implemented, the `Unannotated` case could cause false-positives. - // These should be fixable by adding an explicit `ref`. - match pat.kind { - hir::PatKind::Binding(hir::BindingAnnotation::Unannotated, ..) | - hir::PatKind::Binding(hir::BindingAnnotation::Mutable, ..) => { - let ty = cx.tables.pat_ty(pat); - if is_unrooted_ty(&self.symbols, cx, ty, self.in_new_function) { - cx.span_lint( - UNROOTED_MUST_ROOT, - pat.span, - &format!("Expression of type {:?} must be rooted", ty), - ) - } - }, - _ => {}, - } - - visit::walk_pat(self, pat); - } - - fn visit_ty(&mut self, _: &'tcx hir::Ty) {} - - fn nested_visit_map<'this>(&'this mut self) -> hir::intravisit::NestedVisitorMap<'this, 'tcx> { - hir::intravisit::NestedVisitorMap::OnlyBodies(&self.cx.tcx.hir()) - } -} diff --git a/components/script_plugins/utils.rs b/components/script_plugins/utils.rs deleted file mode 100644 index 6815b7bb0a6..00000000000 --- a/components/script_plugins/utils.rs +++ /dev/null @@ -1,38 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at https://mozilla.org/MPL/2.0/. */ - -use rustc::hir::def_id::DefId; -use rustc::lint::LateContext; -use syntax::source_map::{ExpnKind, MacroKind, Span}; -use syntax::symbol::Symbol; - -/// 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: &LateContext, def_id: DefId, path: &[Symbol]) -> bool { - let krate = &cx.tcx.crate_name(def_id.krate); - if krate != &path[0] { - return false; - } - - let path = &path[1..]; - let other = cx.tcx.def_path(def_id).data; - - if other.len() != path.len() { - return false; - } - - other - .into_iter() - .zip(path) - .all(|(e, p)| e.data.as_interned_str().as_symbol() == *p) -} - -pub fn in_derive_expn(span: Span) -> bool { - if let ExpnKind::Macro(MacroKind::Attr, n) = span.ctxt().outer_expn_data().kind { - n.as_str().contains("derive") - } else { - false - } -}