Add lint for reads and writes that depend on evaluation order

[scurest]

Fixes #277. Well, for simple cases anyway.

It adds a late lint pass that looks for assignments. When it finds one, it walks up the AST from it and for every ancestor expression that depends on evaluation order (calls, binops, etc.) it descends all its children looking for reads to the same variable.

There are some limitations mentioned in the test file, but the main one is that it only works when the l-value is a simple variable name (x).

[oli-obk]

we should either fix rustc's "unused_assignments" lint or produce our own lint for this case.

[scurest]

It's easy if you just want to detect an assignments to x whose RHS also assigns to x; the lint would just need to put here. I can add that if you want, but I don't know what to call it.

[oli-obk]

no need to do anything... rustc warns: https://is.gd/Gxtp1U

[scurest]

But not for x = { x = 5; x }; :)

[oli-obk]

that's because it's not an unused assignment ;)

[oli-obk]

x = (x = 5, x + 1).1; is the more interesting case, since blocks have a controlled order of execution.

[scurest]

(Ah, sorry, those aren't outdated, I just amended because I missed some spaces in the test file).

[oli-obk]

lgtm

There are some limitations mentioned in the test file, but the main one is that it only works when the l-value is a simple variable name (x).

we'll just open an issue :)

[Manishearth]

Super nice lint! However, does this have a measurable impact on execution time? Walking up the tree on every assignment seems expensive.

It's possible to make the same set of checks cheaper with a top-down approach -- using the _post methods on the lint pass, you can keep track of whether or not you're directly in an item scope and ignore those assignments (or make the whole check top-down, but this is more fiddly)

[scurest]

I was worried about that too, but I can't really detect a difference with time cargo test here. In my head, I ascribed that to (1) assignments aren't that common, and (2) most of the ancestors strictly sequence their sub-expressions.

I'm very interested in making it faster though. Could you elaborate on how the top-down approach would work?

[Manishearth]

Not time cargo test, cargo build -Z time-passes on a large enough crate that uses clippy. hyper or html5ever are decent examples. Check that late lint passes don't take much longer.

So I haven't thought about the top-down approach completely yet 😄

A simple optimization you can do is set a flag when your block is directly within a function/impl item. You'll need to use check_block and check_block_post to make this work. Then, only do the walking when you're in blocks nested deeper down. But that won't buy us much.

A better optimization is to track which blocks are getting assigned to things, and set a flag for the duration of these blocks. This might get us sufficient optimization.

Thinking more about it, getting it completely top-down is pretty icky and probably defeats the purpose since it would be just as expensive.

[Manishearth]

Furthermore, this lint isn't that inefficient since it's only walking up a few nodes for each assignment. Not too bad, and since the node types are encoded in the node tree, it's just one cache miss per parent access (at max). Shouldn't cause a big issue.

[scurest]

Looking at the lint checking pass, right? Assuming I did this correctly, the mean over seven samples went from 1.340 to 1.357 when compiling hyper. The maximum of (time with this lint) - (time with master) over all samples was 0.053 (although the minimum was negative). That is pretty high for one lint. If they all took that long linting would take over 8 seconds :/

A better optimization is to track which blocks are getting assigned to things, and set a flag for the duration of these blocks.

I'm not sure. It doesn't have to involve a block being assigned to something at all.

The worst case is probably something like a deeply nested, very wide (ie. lots of args in every call) set of calls where the assignment happens on the deepest level and no read is found, so the lint doesn't trigger.

[Manishearth]

I mean, a block being used within an op, assignment or otherwise.

The time difference isn't that bad -- looks like the mean would be pretty small. It's okay to have a few slow lints. But optimizations would be nice to have 😄