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

Further progress (theoretically it should be able to detect some impr…

…orer usages involving generics)
  • Loading branch information
mrowqa committed Mar 19, 2018
commit 011217b37767a248083a1c44b5a835be49b614dc
@@ -6,6 +6,8 @@ use rustc::hir;
use rustc::hir::intravisit as visit;
use rustc::hir::map as ast_map;
use rustc::lint::{LateContext, LintPass, LintArray, LateLintPass, LintContext};
use rustc::mir;
use rustc::mir::visit::Visitor as MirVisitor;
use rustc::ty;
use syntax::{ast, codemap};
use utils::{match_def_path, in_derive_expn};
@@ -38,6 +40,7 @@ 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 {

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 19, 2018

Contributor

I'm confused. It seems bad to have two copies of this. =)

Is there a reason to do so just because we didn't refactor everything yet?

This comment has been minimized.

Copy link
@mrowqa

mrowqa Mar 19, 2018

Author

Yes, I haven't refactored the code yet. That's why I asked questions about that num "either def_id or generics".

let mut ret = false;
@@ -87,7 +90,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnrootedPass {
cx: &LateContext,
def: &hir::VariantData,
_n: ast::Name,
_gen: &hir::Generics,
_gen: &hir::Generics, // use enum {generics(..), def_id(..)} instead of just def_id in UnrootedCx to (see below)

This comment has been minimized.

Copy link
@mrowqa

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 19, 2018

Contributor

I don't really understand this comment -- oh, are you saying you could pass either a DefId *or Generics`? i.e., an enum like

enum Gen {
  Generics(Generics), DefId(DefId)
}

?

If so, I wouldn't bother. Calls to generics_of are quite cheap, no need to avoid them.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 19, 2018

Contributor

(In particular, they are memoized)

This comment has been minimized.

Copy link
@mrowqa

mrowqa Mar 19, 2018

Author

I could have passed the DefId of context, but what is the proper one here in check_struct_def and below in check_variant? I thought that this _gen should provide good context. That's why I came up with this enum for either one or the another. Anyway, I think getting DefId is the preferred way.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 19, 2018

Contributor

@mrowqa in check_struct_def, it should be the DefId of the struct. In check_variant it should be the DefId of the enum.

id: ast::NodeId) {
let item = match cx.tcx.hir.get(id) {
ast_map::Node::NodeItem(item) => item,
@@ -96,7 +99,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnrootedPass {
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) {
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

This comment has been minimized.

Copy link
@mrowqa
cx.span_lint(UNROOTED_MUST_ROOT, field.span,
"Type must be rooted, use #[must_root] on the struct definition to propagate")
}
@@ -112,7 +115,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for UnrootedPass {
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) {
if is_unrooted_ty(cx, cx.tcx.type_of(def_id), false) { // here same as above?

This comment has been minimized.

Copy link
@mrowqa
cx.span_lint(UNROOTED_MUST_ROOT, field.ty.span,
"Type must be rooted, use #[must_root] on \
the enum definition to propagate")
@@ -142,26 +145,103 @@ 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 {
late_cx: cx,
def_id: def_id,
in_new_function: in_new_function,
};

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) {
if !in_derive_expn(span) { // why? shouldn't it be improper usage anyway?

This comment has been minimized.

Copy link
@mrowqa

mrowqa Mar 19, 2018

Author

AFAIR it fails cause something derives traits like Clone.

This comment has been minimized.

Copy link
@mrowqa

mrowqa Mar 19, 2018

Author

Anyone? @jdm?

let ret_decl = mir.local_decls.iter().next().unwrap();
if !in_new_function && is_unrooted_ty(cx, ret_decl.ty, false) {
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 is_unrooted_ty(cx, decl.ty, false) {
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.")
}
}
}

for decl_ind in mir.vars_iter() {
let decl = &mir.local_decls[decl_ind];
if is_unrooted_ty(cx, decl.ty, in_new_function) {
cx.span_lint(UNROOTED_MUST_ROOT, decl.source_info.span, "Type of binding/expression 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.")
// }
//}
}
}

struct UnrootedCx<'a, 'b: 'a, 'tcx: 'a + 'b> {
late_cx: &'a LateContext<'b, 'tcx>,

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

/// Checks if a type is unrooted or contains any owned unrooted types
impl<'a, 'b, 'tcx> UnrootedCx<'a, 'b, 'tcx> {
fn is_unrooted_ty(&self, ty: &ty::TyS, in_new_function: bool) -> bool {
let mut ret = false;
let cx = self.late_cx;
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::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>(..);}`?

This comment has been minimized.

Copy link
@mrowqa

This comment has been minimized.

Copy link
@mrowqa

mrowqa Mar 19, 2018

Author

@nikomatsakis I guess we don't care about it right know?

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 19, 2018

Contributor

I don't understand the question :)

This comment has been minimized.

Copy link
@mrowqa

mrowqa Mar 19, 2018

Author

In normal functions we will know all of the types in MIR, but in case we are in generic functions, we can use our generic parameter (which MIR, that we have, doesn't know exact type) to call other generic functions (bar in example above). Then we need to recursively track the functions, I guess. Maybe I have complicated it too much. Once I'll reach "our subgoal", I'll just write some test for it and we'll see then :)

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 20, 2018

Contributor

We don't need to recursively track functions -- if we call another generic function, that function will have its #[must_root] annotations (or not). Example:

fn foo<#[must_root] T: Default>() { bar::<T>(); }
fn bar<U>() { }

Here, when we are all done, the call to bar will yield an error. This is because U is not declared with #[must_root], and therefore we can't supply a value for U (in this case, T) that is "must-root".

This comment has been minimized.

Copy link
@mrowqa

mrowqa Mar 20, 2018

Author

Oh, I see. I should have tried to write it down on a paper instead of just communicating my thought.

if cx.tcx.has_attr(ty_param_def.def_id, "must_root") {
ret = true;
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<'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.")
}
}
// how to check return value and arguments?

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 19, 2018

Contributor

the return value and arguments are locals -- so you could just check the number, yes. As for passing mir, I would put a &Mir<'tcx> in the UnrootedCx.

This comment has been minimized.

Copy link
@mrowqa

mrowqa Mar 19, 2018

Author

Your comment above is not displayed in files changed section, that's why I have commented another dot below.

// unpack local and check the number? (0=ret place, 1..arg_count = args?) how to nicely pass arg_count/mir to this method?

This comment has been minimized.

Copy link
@mrowqa

This comment has been minimized.

Copy link
@mrowqa

mrowqa Mar 19, 2018

Author

@nikomatsakis I haven't removed the check from check_fn cause I don't know how to nicely move it here.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 19, 2018

Contributor

I would put the &Mir into the context and then use mir.local_kind()

}
}
@@ -45,6 +45,24 @@ pub mod unrooted_must_root {
*/
pub fn struct_field() {}

/**
```compile_fail
#![feature(plugin)]
#![plugin(script_plugins)]
#![feature(generic_param_attrs)]
#[must_root] struct Foo(i32);
struct Bar<#[must_root] T>(T); // is not detected

This comment has been minimized.

Copy link
@mrowqa

mrowqa Mar 19, 2018

Author

Even though I have written it explicitly, the check_struct_def hasn't detected it.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Mar 19, 2018

Contributor

this is presumably because has_attr doesn't work -- in particular, it will spuriously return false for TyParam until it is fixed (we talked about the fix on gitter)

fn test() {
let _ = &Bar(Foo(3));
}
fn main() {}
```
*/
pub fn generic_struct_field() {}

/**
```compile_fail
#![feature(plugin)]
@@ -160,12 +178,59 @@ pub mod unrooted_must_root {
```
*/
pub fn pattern_binding() {}
/**
```compile_fail
#![feature(plugin)]
#![plugin(script_plugins)]
// TODO how to test against:
// match expr.node {
// // Trait casts from #[must_root] types are not allowed
// hir::ExprCast(ref subexpr, _) => require_rooted(cx, self.in_new_function, &*subexpr),
// 'as' is for casting between primitive types
#[must_root] struct Foo(i32);
struct SomeContainer<T>(T);
// TODO do we want and how to test against Boxing types that are #[must_root]
impl<T> SomeContainer<T> {
fn new(val: T) -> SomeContainer<T> {
SomeContainer(val)
}
}
fn test() {
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.

}
fn main() {}
```
*/
pub fn generic_container() {}

/**
```compile_fail
#![feature(plugin)]
#![plugin(script_plugins)]
#[derive(Default)]
#[must_root] struct Foo(i32);
#[derive(Default)]
#[must_root] struct Bar(i32);
fn create_foo<T: Default>() -> T {
Default::default()
}
fn test() -> i32 {
//let factory = &create_foo::<Foo>;
//factory().0
let elem = &create_foo::<Foo>();
elem.0
}
fn test2() -> i32 {
//let factory = &create_foo::<Foo>;
//factory().0
let elem = &create_foo::<Bar>();
elem.0
}
fn main() {}
```
*/
pub fn derive_default() {}
}
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.