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

Cycle detected when optimizing MIR on nightly #129811

Closed
chvck opened this issue Aug 31, 2024 · 8 comments · Fixed by #129847
Closed

Cycle detected when optimizing MIR on nightly #129811

chvck opened this issue Aug 31, 2024 · 8 comments · Fixed by #129847
Assignees
Labels
A-mir-opt Area: MIR optimizations C-bug Category: This is a bug. F-async_closure `#![feature(async_closure)]` I-cycle Issue: A query cycle occurred while none was expected

Comments

@chvck
Copy link

chvck commented Aug 31, 2024

Originally posted at https://users.rust-lang.org/t/cycle-detected-when-optimizing-mir-on-nightly/116849/1 and filing this upon request from @bjorn3 .

We're using nightly rust and as of nightly-2024-08-29 (prior to this it compiled fine) we see the following error:

error[E0391]: cycle detected when optimizing MIR for `crudcomponent::<impl at sdk/couchbase-core/src/crudcomponent.rs:38:1: 44:41>::orchestrate_simple_crud::{closure#0}::{closure#0}::{closure#0}::{closure#1}`
   --> sdk/couchbase-core/src/crudcomponent.rs:156:78
    |
156 |           orchestrate_retries(self.retry_manager.clone(), retry_info, async || {
    |  ______________________________________________________________________________^
157 | |             orchestrate_memd_collection_id(
158 | |                 self.collections.clone(),
159 | |                 scope_name,
...   |
188 | |             .await
189 | |         })
    | |_________^
    |
    = note: ...which immediately requires optimizing MIR for `crudcomponent::<impl at sdk/couchbase-core/src/crudcomponent.rs:38:1: 44:41>::orchestrate_simple_crud::{closure#0}::{closure#0}::{closure#0}::{closure#1}` again
    = note: cycle used when computing layout of `{async closure body@sdk/couchbase-core/src/crudcomponent.rs:156:78: 189:10}`
    = note: see https://rustc-dev-guide.rust-lang.org/overview.html#queries and https://rustc-dev-guide.rust-lang.org/query.html for more information

For more information about this error, try `rustc --explain E0391`.
error: could not compile `rscbx_couchbase_core` (lib) due to 1 previous error

Function being flagged can be viewed at couchbase-rs/sdk/couchbase-core/src/crudcomponent.rs at nativex · couchbaselabs/couchbase-rs · GitHub. We're having a bit of a hard time debugging this issue, it's not very clear what this actually means. I've tried looking at the HIR output but it didn't really help and I'm a bit lost on this one as I'm not familiar with MIR.

I expected to see this happen: Compilation succeed.

Instead, this happened: Compilation failed due to MIR cycle.

Meta

rustc --version --verbose:

rustc 1.82.0-nightly (0d634185d 2024-08-29)
binary: rustc
commit-hash: 0d634185dfddefe09047881175f35c65d68dcff1
commit-date: 2024-08-29
host: aarch64-apple-darwin
release: 1.82.0-nightly
LLVM version: 19.1.0

Backtrace contained no extra information.

@chvck chvck added the C-bug Category: This is a bug. label Aug 31, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 31, 2024
@bjorn3 bjorn3 added A-mir-opt Area: MIR optimizations I-cycle Issue: A query cycle occurred while none was expected labels Aug 31, 2024
@matthiaskrgr
Copy link
Member

this kind of looks like it should be an ICE instead? 🤔

@saethlin saethlin added F-async_closure `#![feature(async_closure)]` and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 31, 2024
@compiler-errors compiler-errors self-assigned this Aug 31, 2024
@compiler-errors
Copy link
Member

@chvck: I'll try to put up a fix.

We're having a bit of a hard time debugging this issue [...]

It's due to #128506 (not that you need to understand what that PR does), not your fault.

@compiler-errors
Copy link
Member

For now, though, you'll need to either stop using async || {} closures (since you've got an impl Fn() -> Fut bound, not impl async Fn(); why are you using async || {} closures over just || async {}, since those don't really buy you anything in this case?) or pin your nightly compiler version until I can minimize this and fix it.

@theemathas
Copy link
Contributor

theemathas commented Sep 1, 2024

Minimized:

#![feature(async_closure)]

use std::future::Future;

async fn orchestrate_memd_routing<Fut: Future>(operation: impl Fn() -> Fut) {
    operation().await;
}

pub async fn orchestrate_simple_crud() {
    orchestrate_memd_routing(async || async {}.await).await;
}

To reproduce: cargo +nightly build

(Odd that the "optimization" also happens in debug builds.)

Of note: During the minimization process, rust-analyzer froze multiple times. And a few times, cargo +nightly build also froze.

@compiler-errors
Copy link
Member

@theemathas you're awesome! thanks for the minimization.

@chvck
Copy link
Author

chvck commented Sep 1, 2024

It's due to #128506 (not that you need to understand what that PR does), not your fault.

Thanks for confirming.

For now, though, you'll need to either stop using async || {} closures (since you've got an impl Fn() -> Fut bound, not impl async Fn(); why are you using async || {} closures over just || async {}, since those don't really buy you anything in this case?) or pin your nightly compiler version until I can minimize this and fix it.

We were having a lot of issues with lifetimes when using the async blocks which lead to us using async closures instead. Possibly we should just revisit that...

@theemathas
Copy link
Contributor

@chvck You might be interested in these two links for an explanation on how exactly async closures differ from a closure that returns an async block:

https://blog.rust-lang.org/inside-rust/2024/08/09/async-closures-call-for-testing.html

https://rust-lang.github.io/rfcs/3668-async-closures.html

@chvck
Copy link
Author

chvck commented Sep 2, 2024

Thanks @theemathas I'll take a look at those.

@bors bors closed this as completed in 7b7f2f7 Sep 8, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 8, 2024
Rollup merge of rust-lang#129847 - compiler-errors:async-cycle, r=davidtwco

Do not call query to compute coroutine layout for synthetic body of async closure

There is code in the MIR validator that attempts to prevent query cycles when inlining a coroutine into itself, and will use the coroutine layout directly from the body when it detects that's the same coroutine as the one that's being validated. After rust-lang#128506, this logic didn't take into account the fact that the coroutine def id will differ if it's the "by-move body" of an async closure. This PR implements that.

Fixes rust-lang#129811
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 C-bug Category: This is a bug. F-async_closure `#![feature(async_closure)]` I-cycle Issue: A query cycle occurred while none was expected
Projects
None yet
7 participants