Do not optimize MIR for comptime ConstFns

[aaronrumph]

View all comments

Fixed #161770: ICE when trying to run clippy on core, etc. Two small changes, both involve checking that optimized MIR is not requested for comptime functions.

First change is to Clippy, changed so impl LateLintPass for RedundantClone checks whether a function is optimizable or not (i.e., whether it is both a ConstFn && it is a runtime not a comptime function).

Second change is to the pretty print function write_mir_pretty in rustc_middle, which needs the same check to make sure it does not request optimization for comptime functions, even if they're ConstFn.

Can check for the original bug with the following bootstrap.toml:
profile = "compiler"
change-id = 160100
rust.deny-warnings = false

then run the following commands from 'rust' root:

# first failure
RUSTFLAGS="--emit-mir" ./x.py clippy -- -Wclippy::redundant_clone

# second failure
rustc ./tests/ui/comptime/comptime_method_bounds.rs --emit=mir

# third failure
cd library/core
RUSTFLAGES="--emit=mir" cargo check

The first failure is fixed with just the change to impl<'tcx> LateLintPass for RedundantClone, while the second and third failures are fixed by the change to write_mir_pretty() which previously was trying to optimize MIR for comptime functions.

[rustbot]

clippy is developed in its own repository. If possible, consider making this change to rust-lang/rust-clippy instead.

cc @rust-lang/clippy

[rustbot]

Thanks for the pull request, and welcome! The Rust Project has assigned @khyperia (or someone else) to review your changes, you should hear from them (or someone else) within the next two weeks.

Please see the contribution instructions and our LLM policy for more information.

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler, mir
• compiler, mir expanded to 75 candidates
• Random selection from 19 candidates

[aaronrumph]

Please let me know if there are any changes I should make or if you want me to give more information, this is my first time contributing, so am happy to learn how I can improve! I based my PR on the comments I could find in rustc_mir_transform::optimized_mir() and rustc_mir_transform::inner_optimized_mir() which seemed to me to suggest that the panic/ICE is intentional, and it's the caller's responsibility not to try to optimize MIR for comptime functions.

[khyperia]

this is my first time contributing, so am happy to learn how I can improve!

welcome!! thanks so much, this looks really great! ❤️

I think the main thing here is that having some tests would be really nice. For the MIR pretty change, creating a file named... hmm, idk, maybe tests/ui/comptime/emit-mir.rs, with the contents something like this:

//@ check-pass
//@ compile-flags: --emit=mir

#![feature(rustc_attrs)]

#[rustc_comptime]
fn f() {}

fn main() {}

that file is basically a test form of what you wrote, of doing rustc ./tests/ui/comptime/comptime_method_bounds.rs --emit=mir.

View changes since this review

[aaronrumph]

@khyperia Thank you for the feedback/help/advice! I finally got around to making those changes. I changed it so both places check whether a function is marked #[rustc_comptime] using constness() as suggested, and added tests for both files I changed (which are passing on my machine). I formatted it as two new commits, the first has changes to [compiler/rustc_middle/src/mir/pretty.rs] and the accompanying test in [tests/ui/comptime/emit-mir.rs], and the second has the changes to [src/tools/clippy/clippy_lints/src/redundant_clone.rs] and the test (and accompanying files) in [src/tools/clippy/tests/ui/redundant_clone-comptime.rs]. I'm not sure if this was the right way to format my changes, so let me know if you would prefer if I squashed my changes into one commit!

Thank you for taking the time out to give so much help, and let me know if there's any other changes you'd like me to make!

[khyperia]

fantastic, thank you so much!

@bors r+ rollup

[rust-bors]

📌 Commit 282a6c7 has been approved by khyperia

It is now in the queue for this repository.