Make missing_const_for_fn operate on non-optimized MIR

[1c3t3a]

The main reason for this is that we might transform MIR in the optimization passes in a way that doesn't work with const-eval, but it is irrelevant since const-eval uses another MIR (mir_for_ctfe).

Specifically this came up when adding a new check in debug builds (rust-lang/rust#134424), which is added as part of an optimization pass.

changelog: none

[rustbot]

r? @blyxyas

rustbot has assigned @blyxyas.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[y21]

Huh, I'm surprised this works and doesn't ICE. I was under the impression that the only MIR we can use is optimized MIR, since every other intermediate form gets stolen/consumed by the next form.

We use optimized_mir in a bunch of places, and that seems to steal mir_drops_elaborated_and_const_checked?
https://github.com/rust-lang/rust/blob/341f60327fa5302732a4be366949c16f91870b6a/compiler/rustc_mir_transform/src/lib.rs#L709

[1c3t3a]

Ohh I was not aware of that, good point. It seems like it only works out of coincidence. Are you aware of any other way of obtaining a non-steal-able MIR before optimization passes? The problem is that the optimized MIR has checks inserted which are maybe not const (and that is fine if MIR for CTFE doesn't have these checks).

A current example would be that the clippy lint missing_const_for_fn does not propose the const-ness to be correct for deref_ptr_can_be_const in the following example:

struct Foo(*const u32);

impl Foo {
    const fn deref_ptr_can_be_const(self) -> usize {
        unsafe { *self.0 as usize }
    }
}

This is because the optimized MIR has a sequence of checks for alignment inserted:

_3 = deref_copy (_1.0: *const u32);
_4 = copy _3 as *const () (PtrToPtr);
_5 = copy _4 as usize (Transmute);

The Transmute breaks the constness here and clippy stops suggesting the function could be const, although it could be because MIR for CTFE doesn't have this check inserted and we can const-eval everything just fine.

[y21]

Yeah there are also quite a few false positive reports for this lint for the same reason as you described, e.g. the else block of an infallible let-else is not present in optimized_mir and the lint will incorrectly fire for let a = 1 else { return not_const(); }; (#13015). We also have a "tracking issue" for the general problem of using optimized_mir for lints in #6080 which has some discussion.

I have also been thinking about the same problem but haven't been able to come up with a good solution. (I had some very hacky ideas like overriding some MIR query to temporarily have access to the unoptimized body, on which it would run the min_const logic on the DefId and stores the result in some kind of DefId -> min version map that is then accessed by the lint, but I'm not super happy with that...).

For your specific example, maybe we can just accept CastKind::Transmute now to work around it for the time being to unblock your rustc PR? IIUC it's emitted for calls to the transmute(_unchecked) intrinsics, which seem to work fine in const now. From a cursory look that seems to be the only instruction that your new pass inserts that is rejected by the min const checking in clippy?

[1c3t3a]

Oke I see, thanks a lot for the pointer to the already open bug, that's an interesting read.

My mental model of how MIRs are created and thrown around isn't quite close to reality yet, but I still wonder why my code even works. I enabled all lints that create a MIR (all optimized_mir) in the test for this lint and I still don't observe an ICE due to a stolen MIR. Is this a coincidence and a potential race condition or is clippy's architecture around accessing the MIR actually able to handle this case?

Regarding the alternative for CastKind::Transmute: thanks for the background, I would be interested in any hints you have regarding the transmute(_unchecked) intrinsic and its safeness. It's true that the transmute is the only thing that breaks constness and I am fine to ignore it in the lint, however it will also just lead to another class of wrong representations of constness suggestions I guess. But I am happy to pivot this PR if the maintainers are fine with it!

[1c3t3a]

Also #6080 talks about adding tcx.unoptimized_mir. Maybe we wanna add that? It would solve all problems as well I guess.

[blyxyas]

Is this a coincidence and a potential race condition or is clippy's architecture around accessing the MIR

ctfe.rs also calls for the same function (and before this lint is ran), if it was a race condition I'm guessing someone would have fixed it by this point.

[y21]

Is this a coincidence and a potential race condition or is clippy's architecture around accessing the MIR

ctfe.rs also calls for the same function (and before this lint is ran), if it was a race condition I'm guessing someone would have fixed it by this point.

This only calls the query alone though and doesn't do anything with it, the part that can ICE is if you call .borrow() on the Steal returned from the query (like this PR does).

I also feel like it's a coincidence and missing_const_for_fn just happens to run before (all?) other lints that use optimized_mir. For example, if I change the optimized_mir call in large_stack_frames.rs to use mir_drops_elaborated_and_const_checked, then I do get the ICE.

thread 'rustc' panicked at clippy_lints/src/large_stack_frames.rs:152:81:
attempted to read from stolen value: rustc_middle::mir::Body
stack backtrace:
  ...

The warnings from missing_const_for_fn are also ordered before large_stack_frames so that matches with my understanding, though I'm not sure if that actually means anything

Also #6080 talks about adding tcx.unoptimized_mir. Maybe we wanna add that? It would solve all problems as well I guess.

I'm not sure what the optimal way to resolve this is tbh (I'm only aware that this is a known issue), I feel like that could be useful to create a thread for in the t-compiler zulip to discuss

however it will also just lead to another class of wrong representations of constness suggestions I guess

Do you mean that handling CastKind::Transmute in clippy_utils::qualify_min_const_fn to not error would result in false positives? I can't think of any such cases (since as I mentioned my understanding is that the only surface syntax where this instruction is emitted is for the core::intrinisics::{transmute,transmute_unchecked} functions, which are actually const now).

[1c3t3a]

Thanks a lot for the discussion, this is all very insightful! I created a Zulip thread and CC'ed both of you.

can't think of any such cases (since as I mentioned my understanding is that the only surface syntax where this instruction is emitted is for the core::intrinisics::{transmute,transmute_unchecked} functions, which are actually const now).

While this is true (and I saw that std::mem::transmute is indeed marked const), there is still this edge-case that clippy probably (?) wants to catch. From the docs:

Transmuting pointers to integers in a const context is undefined behavior, unless the pointer was originally created from an integer.

So as far as I understand, if clippy wants to properly model that case it would need to modify the existing check for transmute to only fire on pointers transmutes. For my case of debug checks this still keeps the problem, because we actually transmute a pointer (just not in the const-eval MIR). Does that sound correct to you? 😅

[blyxyas]

Looking at the whole situation, I think that I'll approve this PR. In theory, late linting happens in an isolated stage, one by one (which I'm working on to parallelize), so this should not mean anything bad. I'll try again to do a big lintcheck of the 1000 most popular crates and if everything's correct, I'll merge.

Ideally, we would also check if other lints that use optimized_mir can also be omitted.

[blyxyas]

Well, after some more weeks of testing intermitently this PR, I think we can only merge it and hope for the best. If there is an ICE in the following releases, as we will enter a feature freeze and I will be specially open to ICEs.

Testing, lintchecking

I've done some tests, I've tried to setup a rust-lang/crater instance, or use upstream's Crater bot with some cheeky cargo flags (explcitly, a command with +cargoflags=--config build.rustc_workspace_wrapper='path/to/clippy-driver') but it seems that 1. Crater doesn't like to be executed from a local machine, I had to configure it for like 4 hours. and 2. It doesn't work with Clippy.

Crater just downloads the necessary rustc binary and necessary .so files from a static server, and either modifying Crater to download from a separate server or modifying the static server to include Clippy binaries is waaaaaay to hard.

Rationale for the merge

As far as I know, and I've delved way to deep in the lint system, this should be alright. The visitors get evaluated one at a time in a safe manner in a very different time that the .steal() method is called on the query.

Everything should be right... I'll keep an eye open.

---

@1c3t3a Could you fix the conflict?

[1c3t3a]

Thanks a lot for the analysis @blyxyas!! Super cool to see this landing and I hope it will resolve some confusion regarding this lint running on the "wrong" MIR. Should we mention that this also closes other issues, e.g.

• Linting shouldn't be run on optimized MIR #6080
• missing-const-for-fn: println is not const-(st)able yet #13015