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

Generic ADT with unrooted substs are detected.

  • Loading branch information
mrowqa committed Mar 27, 2018
commit d5eafe00eb2147c76be4e192523e4452f428be68
@@ -192,6 +192,26 @@ impl<'a, 'b, 'tcx> UnrootedCx<'a, 'b, 'tcx> {
});
ret
}

fn has_unrooted_generic_substs(&self, did: hir::def_id::DefId, substs: &ty::subst::Substs) -> bool {
let cx = self.late_cx;

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
// give it a must-root type, so just skip.
if cx.tcx.has_attr(ty_param_def.def_id, "must_root") {
continue;
}

let arg_ty = substs.type_at(ty_param_def.index as usize);

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 28, 2018

Contributor

Note that here we are using the index field; I advised you to do this intentionally because we are iterating over the generics from the child first, then up to the parent, which means we aren't walking the substs from 0..N, but really we are working sort of like i..N and then 0..i where i is the number of generics in the parent. That seems fine.

if self.is_unrooted_ty(arg_ty, false) {
return true;
}
}

false
}
}

impl<'a, 'b, 'tcx> MirVisitor<'tcx> for MirFnVisitor<'a, 'b, 'tcx> {
@@ -203,12 +223,25 @@ impl<'a, 'b, 'tcx> MirVisitor<'tcx> for MirFnVisitor<'a, 'b, 'tcx> {
},
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.")
},
_ => {},
}

let cx = ur_cx.late_cx;
match decl.ty.sty {
ty::TyAdt(adt_def, substs) => {
if adt_def.is_box() && self.in_new_function {
// Boxes of unrooted types are allowed in new functions.
}
else if ur_cx.has_unrooted_generic_substs(adt_def.did, substs) {
cx.span_lint(UNROOTED_MUST_ROOT, decl.source_info.span, "ADT generic type must be rooted.")
}
},
_ => {},
}
}

fn visit_constant(&mut self, constant: &mir::Constant<'tcx>, location: mir::Location) {
@@ -218,24 +251,11 @@ impl<'a, 'b, 'tcx> MirVisitor<'tcx> for MirFnVisitor<'a, 'b, 'tcx> {
let cx = ur_cx.late_cx;
match constant.ty.sty {
ty::TyFnDef(callee_def_id, callee_substs) => {
let callee_generics = cx.tcx.generics_of(callee_def_id);
for ty_param_def in &callee_generics.types {
// If type has `#[must_root]`, then it is ok to
// give it a must-root type, so just skip.
if cx.tcx.has_attr(ty_param_def.def_id, "must_root") {
continue;
}

let arg_ty = callee_substs.type_at(ty_param_def.index as usize);
if ur_cx.is_unrooted_ty(arg_ty, false) {
cx.span_lint(UNROOTED_MUST_ROOT, constant.span, "Callee generic type must be rooted.")
}
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.")
}
}
ty::TyAdt(_, _) => {
cx.span_lint(UNROOTED_MUST_ROOT, constant.span, "xd")
}
_ => { }
},
_ => {},
}
}
}
@@ -156,7 +156,7 @@ pub mod unrooted_must_root {
*/
pub fn ban_box() {}

/**
/* *
```
#![feature(plugin)]
#![plugin(script_plugins)]
@@ -171,7 +171,7 @@ pub mod unrooted_must_root {
fn main() {}
```
*/
pub fn allow_box_ref() {}
// pub fn allow_box_ref() {} // is it a correct usage? I guess it isn't.

/**
```compile_fail
@@ -197,20 +197,11 @@ pub mod unrooted_must_root {
```compile_fail
#![feature(plugin)]
#![plugin(script_plugins)]
#![feature(generic_param_attrs)]
#[must_root] struct Foo(i32);
struct SomeContainer<T>(T);
impl<T> SomeContainer<T> {
fn new(val: T) -> SomeContainer<T> {
SomeContainer(val)
}
}
fn test() {
// none should work, but both do
SomeContainer::new(Foo(3));
SomeContainer(Foo(3));

This comment has been minimized.

Copy link
@mrowqa

mrowqa Mar 19, 2018

Author

It still compiles. I haven't took a look at rustc yet.

}
@@ -226,22 +217,23 @@ pub mod unrooted_must_root {
#![feature(generic_param_attrs)]
#[must_root] struct Foo(i32);
#[must_root] struct SomeContainer<#[must_root] T>(T);
//#[must_root] struct SomeContainer<#[must_root] T>(T);
struct SomeContainer<T>(T);
impl<T> SomeContainer<T> { // impl should provide #[must_root]
fn new(val: T) -> SomeContainer<T> {
SomeContainer(val)
fn foo(val: T) {
SomeContainer(val);
}
}
fn test() {
SomeContainer(Foo(3));
SomeContainer::foo(Foo(3));
}
fn main() {}
```
*/
pub fn generic_container2() {}
pub fn generic_impl() {}

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