-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Make missing_const_for_fn operate on non-optimized MIR #14003
Make missing_const_for_fn operate on non-optimized MIR #14003
Conversation
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 |
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 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:
The |
Yeah there are also quite a few false positive reports for this lint for the same reason as you described, e.g. the 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 For your specific example, maybe we can just accept |
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 Regarding the alternative for |
Also #6080 talks about adding |
|
This only calls the query alone though and doesn't do anything with it, the part that can ICE is if you call I also feel like it's a coincidence and
The warnings from
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
Do you mean that handling |
Thanks a lot for the discussion, this is all very insightful! I created a Zulip thread and CC'ed both of you.
While this is true (and I saw that
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? 😅 |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
This has two reasons: First of all we don't need the optimized MIR for evaluating the checks of this lint, as const-eval anyways operates on `mir_for_ctf` which is derived from `mir_drops_elaborated_and_const_checked`. Second of all 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. 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.
ca79ead
to
3e960c1
Compare
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. |
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