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

Recent optimization broke async recursion in release mode #121094

Closed
erickt opened this issue Feb 14, 2024 · 5 comments · Fixed by #121133
Closed

Recent optimization broke async recursion in release mode #121094

erickt opened this issue Feb 14, 2024 · 5 comments · Fixed by #121133
Labels
A-mir-opt Area: MIR optimizations C-bug Category: This is a bug. P-critical Critical priority S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@erickt
Copy link
Contributor

erickt commented Feb 14, 2024

This code compiles in debug mode, but fails in release mode (playground link):

use std::{future::Future, pin::Pin};

pub async fn foo(count: u32) {
    if count == 0 {
        return
    } else {
        let fut: Pin<Box<dyn Future<Output = ()>>> = Box::pin(foo(count - 1));
        fut.await;
    }
}

On release mode, it errors with:

   Compiling playground v0.0.1 (/playground)
error[E0733]: recursion in an async fn requires boxing
 --> src/lib.rs:3:1
  |
3 | pub async fn foo(count: u32) {
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: a recursive `async fn` call must introduce indirection such as `Box::pin` to avoid an infinitely sized future

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

I bisected it down to this patch causing the issue: e132cac. cc @cjgillot.

Meta

rustc --version --verbose:

1.78.0-nightly (2024-02-13 a84bb95a1f65bfe25038)
Backtrace

   Compiling version_check v0.9.4
   Compiling typenum v1.17.0
   Compiling cfg-if v1.0.0
   Compiling cpufeatures v0.2.12
   Compiling generic-array v0.14.7
   Compiling inout v0.1.3
   Compiling crypto-common v0.1.6
   Compiling cipher v0.4.4
   Compiling aes v0.8.4
   Compiling foo v0.1.0 (/usr/local/google/home/etryzelaar/foo)
error[E0733]: recursion in an async fn requires boxing
 --> src/lib.rs:1:1
  |
1 | async fn foo(count: u32) {
  | ^^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: a recursive `async fn` call must introduce indirection such as `Box::pin` to avoid an infinitely sized future

@erickt erickt added the C-bug Category: This is a bug. label Feb 14, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Feb 14, 2024
@erickt erickt changed the title Recent optimization broke async recursion Recent optimization broke async recursion in release mode Feb 14, 2024
@jieyouxu
Copy link
Member

@rustbot label +T-compiler +A-mir-opt +S-has-mcve -needs-triage

@rustbot rustbot added A-mir-opt Area: MIR optimizations S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Feb 14, 2024
@tmandry
Copy link
Member

tmandry commented Feb 14, 2024

Shouldn't this be given a priority?

@rustbot label +P-critical

cc #117206
cc @tmiasko

@rustbot rustbot added the P-critical Critical priority label Feb 14, 2024
@JakobDegen
Copy link
Contributor

This fails to compile with either debug or release mode on stable. That means this isn't a regression, but rather an accidental stabilization. Probably still P-critical though

@erickt
Copy link
Contributor Author

erickt commented Feb 14, 2024

@JakobDegen - good catch, I updated the example to use the dyn trait as suggested by the stable compiler, which passes both debug and release, but fails with nightly release.

@compiler-errors
Copy link
Member

compiler-errors commented Feb 14, 2024

@JakobDegen: This is not an accidental stabilization. This was stabilized in #117703. This is a legitimate misoptimization.

erickt added a commit to erickt/rust that referenced this issue Feb 15, 2024
cc @cjgillot @tmiasko

This revert is based on the following report of a regression caused by this PR:

rust-lang#121094

In accordance with the compiler team [revert policy], PRs that cause meaningful
regressions should be reverted and re-landed once the regression has been fixed
(and a regression test has been added, where appropriate).
[revert policy]: https://forge.rust-lang.org/compiler/reviews.html#reverts

Fear not! Regressions happen. Please rest assured that this does not
represent a negative judgment of your contribution or ability to contribute
positively to Rust in the future. We simply want to prioritize keeping existing
use cases working, and keep the compiler more stable for everyone.

r? compiler
@bors bors closed this as completed in a447249 Feb 15, 2024
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. P-critical Critical priority S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue 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.

6 participants