Skip to content

MIR inlining introduces trait solver overflows #106147

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

Open
JakobDegen opened this issue Dec 26, 2022 · 5 comments
Open

MIR inlining introduces trait solver overflows #106147

JakobDegen opened this issue Dec 26, 2022 · 5 comments
Labels
A-mir-opt Area: MIR optimizations A-mir-opt-inlining Area: MIR inlining C-bug Category: This is a bug. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@JakobDegen
Copy link
Contributor

Code: (playground)

#![recursion_limit = "1"]

trait A {
    fn f();
}

trait B {}
impl<T: B> A for T {
    fn f() {}
}

trait C {}
impl<T: C> B for T {}

fn _foo<T: B>() {
    <T as A>::f();
}

This produces

error[E0275]: overflow evaluating the requirement `T: B`
  |
  = help: consider increasing the recursion limit by adding a `#![recursion_limit = "2"]` attribute to your crate (`test`)
note: required for `T` to implement `A`
 --> test.rs:8:12
  |
8 | impl<T: B> A for T {
  |            ^     ^

error: aborting due to previous error

For more information about this error, try `rustc --explain E0275`.

If and only if MIR inlining is turned on. I'm not sure why, but that seems like a bug.

Meta

rustc --version --verbose:

rustc 1.68.0-nightly (8dfb33954 2022-12-25)
binary: rustc
commit-hash: 8dfb3395411555e93399671d0c41a4ac9ed62b95
commit-date: 2022-12-25
host: x86_64-unknown-linux-gnu
release: 1.68.0-nightly
LLVM version: 15.0.6

Reproduced by compiling the above code with each of:

$ rustc +nightly --crate-type lib -Zinline-mir=no test.rs
$ rustc +nightly --crate-type lib -Zinline-mir=yes test.rs

This regressed when MIR inlining was turned on in stable, but since this is just an overflow regression I'm not going to add the label.

@rustbot label +T-compiler +A-mir-opt +A-mir-opt-inlining

@JakobDegen JakobDegen added the C-bug Category: This is a bug. label Dec 26, 2022
@rustbot rustbot added A-mir-opt Area: MIR optimizations A-mir-opt-inlining Area: MIR inlining T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 26, 2022
@compiler-errors
Copy link
Member

compiler-errors commented Dec 26, 2022

This is only an overflow with a ridiculously low recursion limit, right? Most code that makes its way to codegen also fails to compile with #[recursion_limit = 1], so I'm not sure what the issue here is.

That's just a consequence of the fact MIR inlining calls resolve_instance, so it has to solve more trait obligations than we otherwise would have with polymorphic library code.

@JakobDegen
Copy link
Contributor Author

This is only an overflow with a ridiculously low recursion limit, right?

This example, yes, but with larger examples this should easily reproduce for any recursion limit.

That's just a consequence of the fact MIR inlining calls resolve_instance, so it has to solve more trait obligations than we otherwise would have with polymorphic library code.

Sure, but enabling optimizations should really never cause new errors to be produced

@compiler-errors
Copy link
Member

compiler-errors commented Dec 26, 2022

I'm not convinced that this is a bug, or at least, I'm pretty sure this is not a bug worth solving. Random lints and even modifications to typechecking can cause us to change the obligations we end up needing to validate during the compilation process. Even the choice of compilation phase (check/build, library/bin) can change what we end up solving.

At a more meta-level, I don't think we consider the minimum successfully compileable recursion depth to be something that is stable across releases -- it's kind of a compiler detail 😓 we only really expose this detail because people hit the upper limit too much with their deep monomorphizations. I dug up this comment that seems to echo that sentiment #79909 (comment) (I think, maybe @Mark-Simulacrum can correct me if I'm taking his words incorrectly), but couldn't really find anything else that sets this in stone, so take my words with a grain of salt.

In any case, I'd prefer a more realistic example to demonstrate the issue in practice... Or perhaps some background as to why you stumbled upon this?

@JakobDegen
Copy link
Contributor Author

I don't think we consider the minimum successfully compileable recursion depth to be something that is stable across releases

I agree, but that is not the issue here. Changes to whether or not this overflows between compiler versions are completely fine - the thing I'm claiming is a bug is specifically that -Zinline-mir=yes is causing a change.

Or perhaps some background as to why you stumbled upon this?

No practical example, just working on some stuff that happened to get me to think about this interaction

@saethlin
Copy link
Member

It might be interesting to keep an eye on this. The MIR inliner has some HUGE restrictions on it right now with the default settings, it is restricted based on both the caller and the callee. These restrictions are pretty artificial and conservative, and we should lift them at some point by fixing our inline cost estimation. I would not be surprised if we see a lot more recursion in the inliner when we get to that.

But for this issue: Is there anything else we could do that wouldn't count against the recursion limit?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-mir-opt Area: MIR optimizations A-mir-opt-inlining Area: MIR inlining 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

No branches or pull requests

4 participants