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

Calls to generic functions should be detected

  • Loading branch information
mrowqa committed Mar 26, 2018
commit 41f4774a7f4400ec7a4d5572195f6e25691b336b
@@ -177,8 +177,6 @@ impl<'a, 'b, 'tcx> UnrootedCx<'a, 'b, 'tcx> {
},
ty::TyParam(param_ty) => {
let ty_param_def = cx.tcx.generics_of(self.def_id).type_param(&param_ty, cx.tcx);
// will work for `foo::<Foo>(..)`
// but won't get enough information in `bar<T>(){foo::<T>(..);}`?
if cx.tcx.has_attr(ty_param_def.def_id, "must_root") {
ret = true;
false
@@ -212,4 +210,32 @@ impl<'a, 'b, 'tcx> MirVisitor<'tcx> for MirFnVisitor<'a, 'b, 'tcx> {
_ => {},
}
}

fn visit_constant(&mut self, constant: &mir::Constant<'tcx>, location: mir::Location) {
self.super_constant(constant, location);

let ur_cx = &self.unrooted_cx;
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.")
}
}
}
ty::TyAdt(_, _) => {
cx.span_lint(UNROOTED_MUST_ROOT, constant.span, "xd")

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 27, 2018

Contributor

you probably don't need this here -- this corresponds to a constant value of ADT type, e.g. const FOO: Option<i32> = Some(22) or something. This arm is checking the final type of the constant, and I think that's better done elsewhere. We only wanted to catch the case where we are supply unsuitable values for #[must_root] type parameters.

This comment has been minimized.

Copy link
@mrowqa

mrowqa Mar 27, 2018

Author

Oh, I forgot to remove this check. I was just playing around and then cleaning the code late in the night, so I missed it. Thanks for the explanation what it actually does!

}
_ => { }
}
}
}
@@ -52,7 +52,7 @@ pub mod unrooted_must_root {
#![feature(generic_param_attrs)]
#[must_root] struct Foo(i32);
struct Bar<#[must_root] T>(T); // is not detected
struct Bar<#[must_root] T>(T);
fn test() {
let _ = &Bar(Foo(3));
@@ -63,6 +63,20 @@ pub mod unrooted_must_root {
*/
pub fn generic_struct_field() {}

/**
```compile_fail
#![feature(plugin)]
#![plugin(script_plugins)]
#![feature(generic_param_attrs)]
fn foo<T>() { }
fn bar<#[must_root] U>() { foo::<U>(); }
fn main() {}
```
*/
pub fn generic_function_calling() {}

/**
```compile_fail
#![feature(plugin)]
@@ -178,10 +192,12 @@ pub mod unrooted_must_root {
```
*/
pub fn pattern_binding() {}

/**
```compile_fail
#![feature(plugin)]
#![plugin(script_plugins)]
#![feature(generic_param_attrs)]
#[must_root] struct Foo(i32);
struct SomeContainer<T>(T);
@@ -193,6 +209,8 @@ pub mod unrooted_must_root {
}
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.

}
@@ -205,13 +223,38 @@ pub mod unrooted_must_root {
```compile_fail
#![feature(plugin)]
#![plugin(script_plugins)]
#![feature(generic_param_attrs)]
#[must_root] struct Foo(i32);
#[must_root] struct SomeContainer<#[must_root] T>(T);
impl<T> SomeContainer<T> { // impl should provide #[must_root]
fn new(val: T) -> SomeContainer<T> {
SomeContainer(val)
}
}
fn test() {
SomeContainer(Foo(3));
}
fn main() {}
```
*/
pub fn generic_container2() {}

/* *
```
#![feature(plugin)]
#![plugin(script_plugins)]
#![feature(generic_param_attrs)]
#[derive(Default)]
#[must_root] struct Foo(i32);
#[derive(Default)]
#[must_root] struct Bar(i32);
fn create_foo<T: Default>() -> T {
fn create_foo<#[must_root] T: Default>() -> T {
Default::default()
}
@@ -232,5 +275,6 @@ pub mod unrooted_must_root {
fn main() {}
```
*/
pub fn derive_default() {}
//pub fn derive_default() {}
// ^ do we want to allow derivation?
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.