Stream: t-compiler

Topic: MIR lint pass


RalfJ (May 16 2020 at 11:06, on Zulip):

I am looking into implementing this. That lint pass basically has to be implemented on MIR, so it seems I cannot use the normal lint pass infrastructure. I figured I'd follow unsafety checking, which also is on the MIR, but I cannot even properly figure out how that is invoked... it's a query, so everything is very indirect. The only call to the query is in a function called mir_const which I assume is for consts only. I am a bit lost here. Also I am not sure if adding a new query for my lint is really appropriate, but I'd rather not make this part of unsafety checking which is critical code and already too complicated.

RalfJ (May 16 2020 at 11:07, on Zulip):

oh, found some docs which seems to indicate that mir_const is not just for consts. That's... a very confusingly chosen name.

RalfJ (May 16 2020 at 11:07, on Zulip):

so I could just call that lint directly from mir_const after unsafety checking, I guess?

oli (May 16 2020 at 15:49, on Zulip):

clippy has MIR lints

oli (May 16 2020 at 15:50, on Zulip):

https://github.com/rust-lang/rust-clippy/blob/0c9427309ce0c647ee32cd4c88ca5902b57fb7ee/clippy_lints/src/redundant_clone.rs#L80 is where it gets the MIR

oli (May 16 2020 at 15:50, on Zulip):

basically a lint that is run on all functions and processes them

oli (May 16 2020 at 15:53, on Zulip):

but yea, in rustc we can just create a MirPass that doesn't modify anything

ecstatic-morse (May 16 2020 at 16:01, on Zulip):

There's also the unconditional_recursion lint, which is run right after MIR building. See https://github.com/rust-lang/rust/blob/master/src/librustc_mir_build/lints.rs

RalfJ (May 16 2020 at 17:27, on Zulip):

ecstatic-morse said:

There's also the unconditional_recursion lint, which is run right after MIR building. See https://github.com/rust-lang/rust/blob/master/src/librustc_mir_build/lints.rs

interesting. so we have unsafety checking, unconditional_recursion and now my proposed https://github.com/rust-lang/rust/pull/72270 basically all being MIR lint passes and using entirely different infrastructures...

Félix Fischer (May 17 2020 at 02:24, on Zulip):

I mean... it makes some sense, right?

Félix Fischer (May 17 2020 at 02:25, on Zulip):

At least having Clippy vs rustc to check for correctness, I think, makes sense for the sake of compile times

Félix Fischer (May 17 2020 at 02:25, on Zulip):

Like maybe you have a check in Clippy that is somewhat costly to run all the time

Félix Fischer (May 17 2020 at 02:27, on Zulip):

Now, with regards to mir lints existing at different stages of rustc... that's really intriguing. I'd guess it makes sense if we, say, can run a certain lint with less difficulty at the beginning of the MIR stage and can run another one with less difficulty at the end or middle of the MIR stage

Félix Fischer (May 17 2020 at 02:28, on Zulip):

Sometimes there are checks that are obvious once you look at the optimized MIR, like some code that's rejected because upon constant propagation + folding, the compiler realizes that a code assertion is being invalidated (like when dividing by 0)

Félix Fischer (May 17 2020 at 02:28, on Zulip):

I dunno. It's interesting tho :3

Félix Fischer (May 17 2020 at 02:29, on Zulip):

I didn't know there was more than one part inside the compiler where MIR lints were issued

RalfJ (May 17 2020 at 08:43, on Zulip):

the three passes (unsafety checking is not a lint but a lot like it -- it doesnt modify MIR) run on basically the same MIR -- just before initial construction, just after initial construction, and just a bit after that. I don't immediately see any good reason for that. but then unsafety checking is a query and I do not understand why it should be, so there might be stuff I am missing.

ecstatic-morse (May 20 2020 at 20:43, on Zulip):

I looked at unsafety checking today for #72394 and I don't see a good reason for it to be a query. The query is used to bundle up all the unsafety violations for closures and generators so that it can apply unsafe blocks from the parent item. However, I think we can assume that there is only one AggregateKind::Closure or AggregateKind::Generator per DefId. If that's true, we can simply recurse when we see it instead of storing the results in the query table.

I suppose the downside is that unsafety checking would depend on mir_built for both the parent item and the closure, whereas it only depends on unsafety_check_result for the closure today.

Last update: May 29 2020 at 16:40UTC