Skip to content
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

'cargo check' passes but 'cargo build' fails when there are errors during monomorphization #99682

Open
RalfJung opened this issue Jul 24, 2022 · 6 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@RalfJung
Copy link
Member

RalfJung commented Jul 24, 2022

Consider the following code:

pub struct G;

pub trait IAmArray{
    const SIZE: usize;
}

impl<T, const N: usize> IAmArray for [T; N]{
    const SIZE: usize = N;
}

impl G{
    pub fn i_am_break_on_different_array_sizes<A, B>(_a: A, _b: B)
        where A: IAmArray,
            B: IAmArray,
    {
        trait CompileAssert{
            const TRIGGER: ();
        }
        impl<A, B> CompileAssert for (A, B)
            where A: IAmArray,
                B: IAmArray,
        {
            const TRIGGER: () = if A::SIZE == B::SIZE {} else {panic!("You must provide arrays of same length")};
        }
        
        let _ = <(A, B) as CompileAssert>::TRIGGER;
    }
}

fn main() {
    G::i_am_break_on_different_array_sizes([0u8;5], [0u32;3]);
}

When I do a check-build of this (rustc --emit=metadata), it works fine. But when I do a full build (rustc --emit=link), it fails:

error[E0080]: evaluation of `<([u8; 5], [u32; 3]) as G::i_am_break_on_different_array_sizes::CompileAssert>::TRIGGER` failed
  --> test.rs:23:64
   |
23 |             const TRIGGER: () = if A::SIZE == B::SIZE {} else {panic!("You must provide arrays of same length")};
   |                                                                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'You must provide arrays of same length', test.rs:23:64
   |
   = note: this error originates in the macro `$crate::panic::panic_2015` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info)

note: the above error was encountered while instantiating `fn G::i_am_break_on_different_array_sizes::<[u8; 5], [u32; 3]>`
  --> test.rs:31:5
   |
31 |     G::i_am_break_on_different_array_sizes([0u8;5], [0u32;3]);
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

This is a post-monomorphization error. It is a long-known problem but I could not find an issue tracking it.

It seems worth tracking as it is rather surprising, so if we can find a way to fix it, that would be great. It also leads to issues with Miri such as rust-lang/miri#2423.

All that said, currently this is intended behavior. This RFC gives some relevant background:

cargo check should catch as many errors as possible, but the emphasis of cargo check is on giving a "fast" answer rather than giving a "complete" answer. If you need a complete answer with all possible errors accounted for then you must use cargo build. The rationale for this is that giving a "complete" answer requires (among other things) doing full monomorphization (since some errors, such as those related to associated consts, can only be caught during monomorphization). Monomorphization is expensive: instead of having to check each function only once, each function now has to be checked once for all choices of generic parameters that the crate needs. Given this performance cost and the fact that errors during monomorphization are fairly rare, cargo check favors speed over completeness.

This is related to #49292 but not the same -- that issue is about lints that arise pre-monomorphization, during MIR passes that are skipped in check builds. This here is about errors that appear even later during compilation.

@RalfJung
Copy link
Member Author

So what would be a way to fix it? The only option I can think of right now is to perform some kind of "fake monomorphization" with --emit=metadata, to ensure that we actually evaluate all the constants that will be evaluated in a proper build, but skipping all the actual generation of LLVM IR.

@mqudsi
Copy link
Contributor

mqudsi commented Jul 25, 2022

Tangential question: is there an official guarantee as to what cargo check is supposed to catch?

@AngelicosPhosphoros
Copy link
Contributor

Tangential question: is there an official guarantee as to what cargo check is supposed to catch?

I think, almost everyone expect it to catch everything as actual compilation would. This is a whole point of it: check compilation errors without generating code.

@RalfJung RalfJung added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-bug Category: This is a bug. labels Aug 26, 2022
@RalfJung RalfJung changed the title 'cargo check' passes but 'cargo build' fails 'cargo check' passes but 'cargo build' fails when there are errors during monomorphization Aug 24, 2023
@RalfJung
Copy link
Member Author

#49292 is a related but different issue -- this one here is about errors due to const-eval running only during monomorphization (which check doesn't run); #49292 is about errors that show up pre-monomorphization but that are emitted by lint passes which are not run during check.

@RalfJung
Copy link
Member Author

To fix this, we have to at least do something like a "mentioned item" monomorphizing traversal of the collector (also see #122568). Fundamentally there's no way to avoid having to touch the same function multiple times, since it might be instantiated with different types implementing a trait and their associated consts may behave differently.

So the regression for check times is likely going to be gigantic. It will be up to the compiler team to decide if they want to take that or not. (RFC 3477 puts this choice into t-compiler jurisdiction. It even mentions not doing any kind of monomorphization during check builds -- for performance reasons -- as a motivation for why we are okay with check builds passing when full builds fail.)

I think this issue will likely remain open at least until we decide to have "slow" and "fast" check builds. But it's still useful to have this as a central tracking location.

bors added a commit to rust-lang-ci/rust that referenced this issue Mar 19, 2024
experiment: run mono-item collection in check build

r? `@ghost`
This is just to get an upper bound to the perf overhead associated with fixing rust-lang#99682. We could be a bit more clever and only perform collection, not partitioning, or even more clever and do "mentioned items" collection rather than full collection (when rust-lang#122568 lands) -- but for now let's just get a ballpark number.
@RalfJung
Copy link
Member Author

RalfJung commented Mar 23, 2024

I did an implementation experiment in #122744 (to be able to measure the regression). Some work needs to happen before we can even do the experiment:

So the information to do this is simply not present because we don't store the MIR of things in rmeta files, or something like that? To to make this approach work I think we'd have to (a) make "mentioned items" traversal work without full MIR (based on Oli's proposal of a separate query that just returns the mentioned items), and then (b) do a mentioned items traversal of everything (rather than the usual used items traversal) in a check build.

@Enselic Enselic added the A-diagnostics Area: Messages for errors, warnings, and lints label Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants