Support async recursive calls (as long as they have indirection) by compiler-errors · Pull Request #117703 · rust-lang/rust (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

Before #101692, we stored coroutine witness types directly inside of the coroutine. That means that a coroutine could not contain itself (as a witness field) without creating a cycle in the type representation of the coroutine, which we detected with the OpaqueTypeExpander, which is used to detect cycles when expanding opaque types after that are inferred to contain themselves.

After -Zdrop-tracking-mir was stabilized, we no longer store these generator witness fields directly, but instead behind a def-id based query. That means there is no technical obstacle in the compiler preventing coroutines from containing themselves per se, other than the fact that for a coroutine to have a non-infinite layout, it must contain itself wrapped in a layer of allocation indirection (like a Box).

This means that it should be valid for this code to work:

async fn async_fibonacci(i: u32) -> u32 { if i == 0 || i == 1 { i } else { Box::pin(async_fibonacci(i - 1)).await + Box::pin(async_fibonacci(i - 2)).await } }

Whereas previously, you'd need to coerce the future to Pin<Box<dyn Future<Output = ...>> before awaiting it, to prevent the async's desugared coroutine from containing itself across as await point.

This PR does two things:

Only report an error if an opaque expansion cycle is detected not through coroutine witness fields.
- Instead, if we find an opaque cycle through coroutine witness fields, we compute the layout of the coroutine. If that results in a cycle error, we report it as a recursive async fn.
Reworks the way we report layout errors having to do with coroutines, to make up for the diagnostic regressions introduced by (1.). We actually do even better now, pointing out the call sites of the recursion!

Xuanwo, Rexagon, c410-f3r, orzogc, ilyaaay, and schneiderfelipe reacted with thumbs up emoji ebkalderon reacted with hooray emoji lcnr, slanterns, Kobzol, Dirbaio, Jules-Bertholet, WaffleLapkin, tmandry, kamulos, kornelski, piegamesde, and 24 more reacted with heart emoji

rustbot added A-query-system

Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html)

S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

T-rustdoc

Relevant to the rustdoc team, which will review and decide on the PR/issue.

labels

Nov 8, 2023

This comment has been minimized.

bors added a commit to rust-lang-ci/rust that referenced this pull request

Nov 8, 2023

…try>

Support async recursive calls (as long as they have indirection)

TL;DR: This code should work

async fn foo() {
  Box::pin(foo()).await;
}

r? @ghost while I write up a description, etc.

This comment has been minimized.

bors added S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

Nov 8, 2023

This comment has been minimized.

bors added a commit to rust-lang-ci/rust that referenced this pull request

Nov 12, 2023

…try>

Support async recursive calls (as long as they have indirection)

TL;DR: This code should work

async fn foo() {
  Box::pin(foo()).await;
}

r? @ghost while I write up a description, etc.

This comment has been minimized.

bors added a commit to rust-lang-ci/rust that referenced this pull request

Nov 12, 2023

…try>

Support async recursive calls (as long as they have indirection)

Before rust-lang#101692, we stored coroutine witness types directly inside of the coroutine. That means that a coroutine could not contain itself (as a witness field) without creating a cycle in the type representation of the coroutine, which we detected with the OpaqueTypeExpander, which is used to detect cycles when expanding opaque types after that are inferred to contain themselves.

This means that it should be valid for this code to work:

async fn async_fibonacci(i: u32) -> u32 {
    if i == 0 || i == 1 {
        i
    } else {
        Box::pin(async_fibonacci(i - 1)).await
          + Box::pin(async_fibonacci(i - 2)).await
    }
}

This PR does two things:

Remove the behavior from OpaqueTypeExpander where it intentionally fetches and walks through the coroutine's witness fields. This was kept around after -Zdrop-tracking-mir was stabilized so we would not be introducing new stable behavior, and to preserve the much better diagnostics of async recursion compared to a layout error.
Reworks the way we report layout errors having to do with coroutines, to make up for the diagnostic regressions introduced by (1.). We actually do even better now, pointing out the call sites of the recursion!

This comment has been minimized.

☀️ Try build successful - checks-actions
Build commit: ab5373f (ab5373fe664830d5d63e2d9d47289c64a6d946c8)

This comment has been minimized.

📌 Commit 9a75603 has been approved by lcnr

It is now in the queue for this repository.

bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Jan 8, 2024

bors mentioned this pull request

Jan 9, 2024

Finished benchmarking commit (dc64103): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	0.2%	[0.2%, 0.2%]	1
Improvements ✅ (primary)	-0.3%	[-0.4%, -0.3%]	3
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.3%	[-0.4%, -0.3%]	3

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean	range	count
Regressions ❌ (primary)	2.1%	[1.6%, 2.7%]	4
Regressions ❌ (secondary)	1.3%	[0.8%, 2.0%]	5
Improvements ✅ (primary)	-0.5%	[-0.5%, -0.5%]	1
Improvements ✅ (secondary)	-2.4%	[-2.8%, -2.1%]	3
All ❌✅ (primary)	1.6%	[-0.5%, 2.7%]	5

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 669.826s -> 669.888s (0.01%)
Artifact size: 308.53 MiB -> 308.54 MiB (0.00%)

this is weird, it looks like an inverse blip occurred on the preceding PR (Rollup of 7 pull requests #119760), where tt-muncher check incr-unchanged had a single point with -0.2% instruction-count, and then it preceding to "return to normal" on the succeeding PRs.
(Its harder for me to explain away "inverse blips" ...)
but at the same time, this does not seem like a significant regression by our usual metrics.
marking as triaged.

@rustbot label: +perf-regression-triaged

compiler-errors added relnotes

Marks issues that should be documented in the release notes of the next release.

and removed S-waiting-on-fcp

Status: PR is in FCP and is awaiting for FCP to complete.

labels

Jan 16, 2024

lcnr mentioned this pull request

Feb 9, 2024

lcnr mentioned this pull request

Mar 11, 2024

wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request

Mar 29, 2024