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

Fixed problem with unrooted interior.

  • Loading branch information
mrowqa committed Apr 8, 2018
commit c6d04fa3ad0e929b4b83d6c53f3eaabb692fac1a
@@ -123,7 +123,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnrootedPass {
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?
in_derive_expn: in_derive_expn(span), // TODO replace with whitelist
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.


@@ -196,6 +196,21 @@ impl<'a, 'b, 'tcx> UnrootedCx<'a, 'b, 'tcx> {
fn has_unrooted_generic_substs(&self, did: hir::def_id::DefId, substs: &ty::subst::Substs) -> bool {
let cx = self.late_cx;

if cx.tcx.has_attr(did, "allow_unrooted_interior") {
return false;
}
// TODO we'll see what's necessary and what is not
else if match_def_path(cx, did, &["core", "cell", "Ref"])
|| match_def_path(cx, did, &["core", "cell", "RefMut"])
|| match_def_path(cx, did, &["core", "slice", "Iter"])
|| match_def_path(cx, did, &["std", "collections", "hash", "map", "Entry"])
|| match_def_path(cx, did, &["std", "collections", "hash", "map", "OccupiedEntry"])
|| match_def_path(cx, did, &["std", "collections", "hash", "map", "VacantEntry"])
|| match_def_path(cx, did, &["std", "collections", "hash", "map", "Iter"])
|| match_def_path(cx, did, &["std", "collections", "hash", "set", "Iter"]) {
return false;
}

let generics = cx.tcx.generics_of(did);

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 28, 2018

Contributor

Right, so here we want to actually have a loop, sort of like:

let mut did = did;
loop {
  let generics = cx.tcx.generics_of(did);
  ... // just as you have it now
  match generics.parent {
    Some(d) => { def_id = d; }
    None => { return;
  }
}

See also:

https://github.com/rust-lang/rust/blob/9c9424de51da41fd3d1077ac7810276f8dc746fa/src/librustc/ty/mod.rs#L771-L772

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 28, 2018

Contributor

alternatively just having the function call itself recursively would be fine

for ty_param_def in &generics.types {
// If type has `#[must_root]`, then it is ok to
@@ -255,14 +270,18 @@ impl<'a, 'b, 'tcx> MirVisitor<'tcx> for MirFnVisitor<'a, 'b, 'tcx> {
match constant.ty.sty {
ty::TyFnDef(callee_def_id, callee_substs) => {
let def_path = get_def_path(cx, callee_def_id);
// TODO remove following commented code
// cx.span_lint(UNROOTED_MUST_ROOT, constant.span, &def_path.join("::")); // tmp auxiliary call
if self.in_new_function && (// match_def_path(cx, callee_def_id, &["alloc", "boxed", "{{impl}}", "new"]) ||
def_path.last().unwrap().starts_with("new_") || def_path.last().unwrap() == "new") {
// ^ need more checks / currently some dirty / proof of work code
// some explanation
// "new"-like (constructors) functions are allowed
}
else if ur_cx.has_unrooted_generic_substs(callee_def_id, callee_substs) {
cx.span_lint(UNROOTED_MUST_ROOT, constant.span, "Callee generic type must be rooted.")
// TODO tmp code - delete later
let mut msg = "Callee generic type must be rooted.".to_string();
msg.push_str(&def_path.join("::"));
cx.span_lint(UNROOTED_MUST_ROOT, constant.span, &msg);
//cx.span_lint(UNROOTED_MUST_ROOT, constant.span, "Callee generic type must be rooted.")
}
},
_ => {},
@@ -194,23 +194,6 @@ pub mod unrooted_must_root {
*/
pub fn ban_box() {}

/* *
```
#![feature(plugin)]
#![plugin(script_plugins)]
#[must_root] struct Foo(i32);
fn foo(x: &Foo) -> i32 {
let y = &Box::new(Foo(x.0 + 3));
y.0
}
fn main() {}
```
*/
// pub fn allow_box_ref() {} // is it a correct usage? I guess it isn't.

/**
```compile_fail
#![feature(plugin)]
@@ -272,6 +255,31 @@ pub mod unrooted_must_root {
*/
pub fn generic_impl() {}

/**
```
#![feature(plugin)]
#![plugin(script_plugins)]
#![feature(generic_param_attrs)]
#[must_root] struct Foo(i32);
#[allow_unrooted_interior] struct SomeContainer<T>(T);
impl<#[must_root] T> SomeContainer<T> {
fn foo(val: &T) {
SomeContainer(val);
}
}
fn test() {
SomeContainer(Foo(3));
SomeContainer::foo(&Foo(3));
}
fn main() {}
```
*/
pub fn allowing_unrooted_interior() {}

/* *
```
#![feature(plugin)]
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.