Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#14902 Write a MIR lint for rooting analysis #20264

Closed
wants to merge 15 commits into from
Closed
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Moved forward with generics checking & refactored the code

  • Loading branch information
mrowqa committed Mar 19, 2018
commit 4169308e771d60f2a9addb94a1a8cd41a2b1b7cc
@@ -40,44 +40,6 @@ impl UnrootedPass {
}
}

// left to let the code compile
/// Checks if a type is unrooted or contains any owned unrooted types
fn is_unrooted_ty(cx: &LateContext, ty: &ty::TyS, in_new_function: bool) -> bool {
let mut ret = false;
ty.maybe_walk(|t| {
match t.sty {
ty::TyAdt(did, _) => {
if cx.tcx.has_attr(did.did, "must_root") {
ret = true;
false
} 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"])
|| match_def_path(cx, did.did, &["core", "slice", "Iter"])
|| match_def_path(cx, did.did, &["std", "collections", "hash", "map", "Entry"])
|| match_def_path(cx, did.did, &["std", "collections", "hash", "map", "OccupiedEntry"])
|| match_def_path(cx, did.did, &["std", "collections", "hash", "map", "VacantEntry"])
|| match_def_path(cx, did.did, &["std", "collections", "hash", "map", "Iter"])
|| match_def_path(cx, did.did, &["std", "collections", "hash", "set", "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::TyRef(..) => false, // don't recurse down &ptrs
ty::TyRawPtr(..) => false, // don't recurse down *ptrs
ty::TyFnDef(..) | ty::TyFnPtr(_) => false,
_ => true
}
});
ret
}

impl LintPass for UnrootedPass {
fn get_lints(&self) -> LintArray {
lint_array!(UNROOTED_MUST_ROOT)
@@ -90,16 +52,20 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnrootedPass {
cx: &LateContext,
def: &hir::VariantData,
_n: ast::Name,
_gen: &hir::Generics, // use enum {generics(..), def_id(..)} instead of just def_id in UnrootedCx to (see below)
_gen: &hir::Generics,
id: ast::NodeId) {
let unrooted_cx = UnrootedCx {
late_cx: cx,
def_id: cx.tcx.hir.local_def_id(id),
};
let item = match cx.tcx.hir.get(id) {
ast_map::Node::NodeItem(item) => item,
_ => cx.tcx.hir.expect_item(cx.tcx.hir.get_parent(id)),
};
if item.attrs.iter().all(|a| !a.check_name("must_root")) {
for ref field in def.fields() {
let def_id = cx.tcx.hir.local_def_id(field.id);
if is_unrooted_ty(cx, cx.tcx.type_of(def_id), false) { // ... be able to call is_unrooted_ty here? then we don't need to query generics_of
if unrooted_cx.is_unrooted_ty(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")
}
@@ -110,12 +76,17 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> 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.hir;
if map.expect_item(map.get_parent(var.node.data.id())).attrs.iter().all(|a| !a.check_name("must_root")) {
let parent_node = map.get_parent(var.node.data.id());
let unrooted_cx = UnrootedCx {
late_cx: cx,
def_id: map.local_def_id(parent_node),
};
if map.expect_item(parent_node).attrs.iter().all(|a| !a.check_name("must_root")) {
match var.node.data {
hir::VariantData::Tuple(ref fields, _) => {
for ref field in fields {
let def_id = cx.tcx.hir.local_def_id(field.id);
if is_unrooted_ty(cx, cx.tcx.type_of(def_id), false) { // here same as above?
if unrooted_cx.is_unrooted_ty(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")
@@ -145,33 +116,18 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnrootedPass {

let def_id = cx.tcx.hir.local_def_id(id);
let mir = cx.tcx.optimized_mir(def_id);
let mut unrooted_cx = UnrootedCx {
let unrooted_cx = UnrootedCx {
late_cx: cx,
def_id: def_id,
};
let mut visitor = MirFnVisitor {
unrooted_cx: unrooted_cx,
in_new_function: in_new_function,
in_derive_expn: in_derive_expn(span), // why? shouldn't it be improper usage anyway?
mir: mir,
};

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 19, 2018

Contributor

the main reason to define this was to define is_unrooted_ty as a method, so that it could access self.def_id to compute generics_of

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 19, 2018

Contributor

it may not be necessary, just an idea to keep code nicer and not pass 100 arguments

This comment has been minimized.

Copy link
@mrowqa

mrowqa Mar 19, 2018

Author

I have implemented it as a method.


if !in_derive_expn(span) { // why? shouldn't it be improper usage anyway?
let ret_decl = mir.local_decls.iter().next().unwrap();
if !in_new_function && unrooted_cx.is_unrooted_ty(ret_decl.ty, false) {
cx.span_lint(UNROOTED_MUST_ROOT, ret_decl.source_info.span, "Function return type must be rooted.")
}

for decl_ind in mir.args_iter() {
let decl = &mir.local_decls[decl_ind];
if unrooted_cx.is_unrooted_ty(decl.ty, false) {
cx.span_lint(UNROOTED_MUST_ROOT, decl.source_info.span, "Function argument type must be rooted.")
}
}
}

unrooted_cx.visit_mir(mir); // seems to work
//for decl_ind in mir.vars_iter() {
// let decl = &mir.local_decls[decl_ind];
// if unrooted_cx.is_unrooted_ty(decl.ty, in_new_function) {
// cx.span_lint(UNROOTED_MUST_ROOT, decl.source_info.span, "Type of binding/expression must be rooted.")
// }
//}
visitor.visit_mir(mir);
}
}

@@ -180,7 +136,13 @@ struct UnrootedCx<'a, 'b: 'a, 'tcx: 'a + 'b> {

/// context of definition we want to check
def_id: hir::def_id::DefId,
}

struct MirFnVisitor<'a, 'b: 'a, 'tcx: 'a + 'b> {
unrooted_cx: UnrootedCx<'a, 'b, 'tcx>,
in_new_function: bool,
in_derive_expn: bool,
mir: &'a mir::Mir<'tcx>,
}

/// Checks if a type is unrooted or contains any owned unrooted types
@@ -234,14 +196,20 @@ impl<'a, 'b, 'tcx> UnrootedCx<'a, 'b, 'tcx> {
}
}

impl<'a, 'b, 'tcx> MirVisitor<'tcx> for UnrootedCx<'a, 'b, 'tcx> {
fn visit_local_decl(&mut self, _local: mir::Local, decl: &mir::LocalDecl<'tcx>) {
if decl.is_user_variable {
if self.is_unrooted_ty(decl.ty, self.in_new_function) {
self.late_cx.span_lint(UNROOTED_MUST_ROOT, decl.source_info.span, "Type of binding/expression must be rooted.")
impl<'a, 'b, 'tcx> MirVisitor<'tcx> for MirFnVisitor<'a, 'b, 'tcx> {
fn visit_local_decl(&mut self, local: mir::Local, decl: &mir::LocalDecl<'tcx>) {
let ur_cx = &self.unrooted_cx;
match self.mir.local_kind(local) {
mir::LocalKind::ReturnPointer => if !self.in_derive_expn && !self.in_new_function && ur_cx.is_unrooted_ty(decl.ty, false) {
ur_cx.late_cx.span_lint(UNROOTED_MUST_ROOT, decl.source_info.span, "Function return type must be rooted.")
},
mir::LocalKind::Arg => if !self.in_derive_expn && ur_cx.is_unrooted_ty(decl.ty, false) {
ur_cx.late_cx.span_lint(UNROOTED_MUST_ROOT, decl.source_info.span, "Function argument type must be rooted.")
}
mir::LocalKind::Var => if ur_cx.is_unrooted_ty(decl.ty, self.in_new_function) {
ur_cx.late_cx.span_lint(UNROOTED_MUST_ROOT, decl.source_info.span, "Type of binding/expression must be rooted.")
},
_ => {},
}
// how to check return value and arguments?
// unpack local and check the number? (0=ret place, 1..arg_count = args?) how to nicely pass arg_count/mir to this method?
}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.