Make unrooted_must_root a bit more aggressive. #8073

eefriedman · 2015-10-18T22:24:20Z

Basically, instead of trying to check for specific kinds of statements,
just check the types of all local variables.

Also included are some commented-out proposals for some slightly more
aggressive lints which might be useful (but trigger a little too
frequently at the moment).

nox · 2015-10-19T07:53:21Z

components/script/dom/filelist.rs

@@ -18,6 +18,7 @@ pub struct FileList {
 }

 impl FileList {
+    #[allow(unrooted_must_root)]
    fn new_inherited(files: Vec<JS<File>>) -> FileList {


Anything named new or new_something is supposed to get automatically white-listed, weird that you had to add these.

No, only box unrooted is safe in new, everything else isn't.

In this case it's the pat from the argument list that's being caught. It wasn't caught in the past since check_fn is not run on new_*.

This can be fixed by adding another state to the lint, "in_argument_list", which is set unconditionally by check_fn and turned off by check_block, and bailing in check_pat when in_new_function && in_argument_list

nox · 2015-10-19T07:54:41Z

r? @Manishearth

Manishearth · 2015-10-19T08:12:41Z

components/plugins/lints/unrooted_must_root.rs

-            cx.span_lint(UNROOTED_MUST_ROOT, expr.span,
-                                     &format!("Expression of type {:?} must be rooted", ty))
+    fn check_pat(&mut self, cx: &LateContext, pat: &hir::Pat) {
+        if let hir::PatIdent(hir::BindByValue(_), _, _) = pat.node {


This won't catch let (a, b, c) = foo(); We should allow any pat here I guess?

check_pat gets called for every hir::Pat in the tree, so we would end up checking a, b, and c individually. (This also allows us to avoid false positives involving by-ref matches.)

Oh, right, sorry.

But then, this would catch let ref a = b. It shouldn't.

Oh, no it won't. sorry.

pat_ty_opt returns the type of the variable in let ref a = b, so it works correctly.

Manishearth · 2015-10-19T08:14:10Z

Mostly looks good. Just need to add the second state variable for ignoring function arguments, and make the pat check handle all pats.

The alternative to the state variable is to keep check_stmt, move the ExprAssign/ExprMatch out into check_expr, along with an ExprClosure check. This will handle all possible forms of bindings without touching function bindings.

eefriedman · 2015-10-19T20:02:17Z

Came up with a different solution for ignoring function arguments: using a nested visitor. This is a bit more convenient for a couple reasons: it makes it easier to ensure the lint only sees patterns which actually represent variables, and it handles nested function definitions more correctly.

Manishearth · 2015-10-19T20:20:34Z

Note that this might affect compile times. The regular lint is implemented as a visitor which runs threaded together with the other lints. A separate visitor might make things slower. But we're not doing any crazy computations here so we should be fine.

Ms2ger · 2015-10-20T13:20:49Z

How is JS<T> in function arguments safe?

Manishearth · 2015-10-20T14:32:20Z

@Ms2ger It's not, but we need to ignore it for new_*() functions

jdm · 2015-10-20T15:21:00Z

@Manishearth I thought the goal was to suppress the error for return values for such functions; I don't recall any discussion about the function arguments.

Manishearth · 2015-10-20T15:34:30Z

Okay, we can turn on warnings for the argument list then.

eefriedman · 2015-10-20T18:00:38Z

I'm not exactly sure how the argument list is any worse than the return type? Conceptually, functions should always return a rooted value.

Anyway, I'll change it.

eefriedman · 2015-10-20T18:08:02Z

Updated.

jdm · 2015-10-20T18:11:22Z

@eefriedman The exception for the return value for new_inherited methods is for constructing the base types for derived interfaces, which are stored as Foo instead of JS<Foo>.

eefriedman · 2015-10-23T19:45:20Z

@Manishearth Ping.

bors-servo · 2015-10-23T21:39:07Z

☔ The latest upstream changes (presumably #8091) made this pull request unmergeable. Please resolve the merge conflicts.


        Make unrooted_must_root a bit more aggressive.

Basically, instead of trying to check for specific kinds of statements, just check the types of all local variables. Also included are some commented-out proposals for some slightly more aggressive lints which might be useful (but trigger a little too frequently at the moment).


        Don't allow unrooted values in function arguments.

eefriedman · 2015-10-23T21:47:40Z

Rebased.

Manishearth · 2015-10-24T00:21:47Z

Reviewed 4 of 8 files at r1, 4 of 4 files at r2, 5 of 5 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

^{components/plugins/lints/unrooted_must_root.rs, line 40 [r2] (raw file):}
Should be false by default?

Comments from the review on Reviewable.io

eefriedman · 2015-10-24T00:42:59Z

Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

^{components/plugins/lints/unrooted_must_root.rs, line 40 [r2] (raw file):}
What should be false by default? This code doesn't exist with my patch. My current approach doesn't try to check code outside of a function definition; it generally can't contain any relevant code.

Comments from the review on Reviewable.io

Manishearth · 2015-10-24T09:21:24Z

@bors-servo retry

It's asking for a license fix. Tried to ssh and fix it, but it seems like the admin password was updated.

bors-servo · 2015-10-24T09:21:27Z

⌛ Testing commit a9ef40c with merge 77dee47...


        Auto merge of #8073 - eefriedman:root-lint, r=Manishearth

Make unrooted_must_root a bit more aggressive. Basically, instead of trying to check for specific kinds of statements, just check the types of all local variables. Also included are some commented-out proposals for some slightly more aggressive lints which might be useful (but trigger a little too frequently at the moment).  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8073)

bors-servo · 2015-10-24T09:22:50Z

💔 Test failed - mac-rel-wpt

Manishearth · 2015-10-24T14:18:43Z

@bors-servo retry

bors-servo · 2015-10-24T14:18:45Z

⌛ Testing commit a9ef40c with merge 49d5245...


        Auto merge of #8073 - eefriedman:root-lint, r=Manishearth

Make unrooted_must_root a bit more aggressive. Basically, instead of trying to check for specific kinds of statements, just check the types of all local variables. Also included are some commented-out proposals for some slightly more aggressive lints which might be useful (but trigger a little too frequently at the moment).  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8073)

bors-servo · 2015-10-24T14:57:56Z

💔 Test failed - mac-rel-css

Manishearth · 2015-10-24T14:59:07Z

@bors-servo retry #7791

bors-servo · 2015-10-24T14:59:11Z

⌛ Testing commit a9ef40c with merge e66825e...


        Auto merge of #8073 - eefriedman:root-lint, r=Manishearth

Make unrooted_must_root a bit more aggressive. Basically, instead of trying to check for specific kinds of statements, just check the types of all local variables. Also included are some commented-out proposals for some slightly more aggressive lints which might be useful (but trigger a little too frequently at the moment).  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8073)

Manishearth · 2015-10-24T22:25:46Z

@frewsxcv Nothing's going to work till the builders come back

bors-servo · 2015-10-25T01:20:06Z

⌛ Testing commit a9ef40c with merge bb88832...


        Auto merge of #8073 - eefriedman:root-lint, r=Manishearth

Make unrooted_must_root a bit more aggressive. Basically, instead of trying to check for specific kinds of statements, just check the types of all local variables. Also included are some commented-out proposals for some slightly more aggressive lints which might be useful (but trigger a little too frequently at the moment).  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8073)

bors-servo · 2015-10-25T02:13:47Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt

highfive added the S-awaiting-review label Oct 18, 2015

eefriedman force-pushed the eefriedman:root-lint branch from ec7487c to 487984a Oct 18, 2015

nox reviewed Oct 19, 2015
View changes

highfive assigned Manishearth Oct 19, 2015

nox added I-safety A-plugins labels Oct 19, 2015

Manishearth reviewed Oct 19, 2015
View changes

eefriedman force-pushed the eefriedman:root-lint branch from 487984a to b1210f9 Oct 19, 2015

eefriedman force-pushed the eefriedman:root-lint branch from b1210f9 to 81701a5 Oct 20, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Oct 23, 2015

eefriedman added 2 commits Oct 18, 2015

Don't allow unrooted values in function arguments.

Loading status checks…

a9ef40c

eefriedman force-pushed the eefriedman:root-lint branch from 81701a5 to a9ef40c Oct 23, 2015

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 24, 2015

highfive added S-awaiting-merge and removed S-tests-failed labels Oct 24, 2015

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 24, 2015

highfive added S-awaiting-merge and removed S-tests-failed labels Oct 24, 2015

highfive added S-tests-failed and removed S-awaiting-merge labels Oct 24, 2015

highfive added S-awaiting-merge and removed S-tests-failed labels Oct 24, 2015

frewsxcv closed this Oct 24, 2015

frewsxcv reopened this Oct 24, 2015

bors-servo merged commit a9ef40c into servo:master Oct 25, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

servo / servo

Make unrooted_must_root a bit more aggressive. #8073

Make unrooted_must_root a bit more aggressive. #8073

eefriedman commented Oct 18, 2015

This comment has been minimized.

This comment has been minimized.

nox commented Oct 19, 2015

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Oct 19, 2015

eefriedman commented Oct 19, 2015

Manishearth commented Oct 19, 2015

Ms2ger commented Oct 20, 2015

Manishearth commented Oct 20, 2015

jdm commented Oct 20, 2015

Manishearth commented Oct 20, 2015

eefriedman commented Oct 20, 2015

eefriedman commented Oct 20, 2015

jdm commented Oct 20, 2015

eefriedman commented Oct 23, 2015

bors-servo commented Oct 23, 2015

eefriedman commented Oct 23, 2015

Manishearth commented Oct 24, 2015

eefriedman commented Oct 24, 2015

Manishearth commented Oct 24, 2015

bors-servo commented Oct 24, 2015

bors-servo commented Oct 24, 2015

Manishearth commented Oct 24, 2015

bors-servo commented Oct 24, 2015

bors-servo commented Oct 24, 2015

Manishearth commented Oct 24, 2015

bors-servo commented Oct 24, 2015

Manishearth commented Oct 24, 2015

bors-servo commented Oct 25, 2015

bors-servo commented Oct 25, 2015

servo / servo

Join GitHub today

Make unrooted_must_root a bit more aggressive. #8073

Make unrooted_must_root a bit more aggressive. #8073

Conversation

eefriedman commented Oct 18, 2015

This comment has been minimized.

nox Oct 19, 2015

This comment has been minimized.

Manishearth Oct 19, 2015

nox commented Oct 19, 2015

This comment has been minimized.

Manishearth Oct 19, 2015

This comment has been minimized.

eefriedman Oct 19, 2015

This comment has been minimized.

Manishearth Oct 19, 2015

This comment has been minimized.

Manishearth Oct 19, 2015

This comment has been minimized.

eefriedman Oct 19, 2015

Manishearth commented Oct 19, 2015

eefriedman commented Oct 19, 2015

Manishearth commented Oct 19, 2015

Ms2ger commented Oct 20, 2015

Manishearth commented Oct 20, 2015

jdm commented Oct 20, 2015

Manishearth commented Oct 20, 2015

eefriedman commented Oct 20, 2015

eefriedman commented Oct 20, 2015

jdm commented Oct 20, 2015

eefriedman commented Oct 23, 2015

bors-servo commented Oct 23, 2015

eefriedman commented Oct 23, 2015

Manishearth commented Oct 24, 2015

eefriedman commented Oct 24, 2015

Manishearth commented Oct 24, 2015

bors-servo commented Oct 24, 2015

bors-servo commented Oct 24, 2015

Manishearth commented Oct 24, 2015

bors-servo commented Oct 24, 2015

bors-servo commented Oct 24, 2015

Manishearth commented Oct 24, 2015

bors-servo commented Oct 24, 2015

Manishearth commented Oct 24, 2015

bors-servo commented Oct 25, 2015

bors-servo commented Oct 25, 2015