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

Tighten fn_decl_span for async blocks #127058

Merged
merged 1 commit into from
Jun 28, 2024

Conversation

compiler-errors
Copy link
Member

Tightens the span of async {} blocks in diagnostics, and subsequently async closures and async fns, by actually setting the fn_decl_span correctly. This is kinda a follow-up on #125078, but it fixes the problem in a more general way.

I think the diagnostics are significantly improved, since we no longer have a bunch of overlapping spans. I'll point out one caveat where I think the diagnostic may get a bit more confusing, but where I don't think it matters.

r? @estebank or @oli-obk or someone else on wg-diag or compiler i dont really care lol

@rustbot rustbot added 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. WG-trait-system-refactor The Rustc Trait System Refactor Initiative labels Jun 27, 2024
LL | | game_loop(Arc::clone(&room_ref))
| | -------- `room_ref` is borrowed here
LL | | });
| |_____^ may outlive borrowed value `room_ref`
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the only place that the diagnostic arguably got worse. I kinda don't think it matters, though, because the primary message still says:

async block may outlive the current function, but it borrows room_ref, which is owned by the current function

So it's still clear that the problem is with an async block.

If you care enough about this for it to get fixed, then I suggest we change the label to state what may outlive the borrowed value, i.e.:

LL |     let gameloop_handle = spawn(async {
   |                                 ^^^^^ async block may outlive borrowed value `room_ref`

@rust-log-analyzer

This comment has been minimized.

@rustbot
Copy link
Collaborator

rustbot commented Jun 27, 2024

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/tools/rustfmt

cc @rust-lang/rustfmt

@oli-obk
Copy link
Contributor

oli-obk commented Jun 27, 2024

r? @oli-obk

bors r plus

@bors rollup

@bors
Copy link
Contributor

bors commented Jun 27, 2024

📌 Commit 789ee88 has been approved by oli-obk

It is now in the queue for this repository.

@rustbot rustbot assigned oli-obk and unassigned estebank Jun 27, 2024
@bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Jun 27, 2024
@@ -8,7 +8,7 @@ LL | let f: F = async { 1 };
= help: add `#![feature(const_async_blocks)]` to the crate attributes to enable
= note: this compiler was built on YYYY-MM-DD; consider upgrading it if it is out of date

error[E0271]: expected `{async block@$DIR/issue-78722.rs:10:13: 10:21}` to be a future that resolves to `u8`, but it resolves to `()`
error[E0271]: expected `{async block@$DIR/issue-78722.rs:10:13: 10:18}` to be a future that resolves to `u8`, but it resolves to `()`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: we should special case the wording here to be something like

Suggested change
error[E0271]: expected `{async block@$DIR/issue-78722.rs:10:13: 10:18}` to be a future that resolves to `u8`, but it resolves to `()`
error[E0271]: expected async block to be a future that resolves to `u8`, but it resolves to `()`

Comment on lines +1 to +7
error[E0277]: `{gen block@$DIR/gen_block_is_no_future.rs:5:5: 5:8}` is not a future
--> $DIR/gen_block_is_no_future.rs:4:13
|
LL | fn foo() -> impl std::future::Future {
| ^^^^^^^^^^^^^^^^^^^^^^^^ `{gen block@$DIR/gen_block_is_no_future.rs:5:5: 5:21}` is not a future
| ^^^^^^^^^^^^^^^^^^^^^^^^ `{gen block@$DIR/gen_block_is_no_future.rs:5:5: 5:8}` is not a future
|
= help: the trait `Future` is not implemented for `{gen block@$DIR/gen_block_is_no_future.rs:5:5: 5:21}`
= help: the trait `Future` is not implemented for `{gen block@$DIR/gen_block_is_no_future.rs:5:5: 5:8}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to figure out how to point at the gen/async block in E0277s because the error as is isn't as clear as it should be: we point at the span that introduced the obligation, not where the obligation wasn't fulfilled.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a general problem with opaques.

we point at the span that introduced the obligation

This is the span that introduces the obligation. You're thinking about the span that introduces the expression that assigns the hidden type of the opaque. Determining what that span is is difficult to impossible without hacks that I would rather not try to land, because they will almost certainly regress with the new solver.

@@ -13,7 +13,7 @@ note: captured value is not `Send` because `&` references cannot be sent unless
|
LL | let x = x;
| ^ has type `&T` which is not `Send`, because `T` is not `Sync`
= note: required for the cast from `Pin<Box<{async block@$DIR/issue-86507.rs:18:17: 20:18}>>` to `Pin<Box<(dyn Future<Output = ()> + Send + 'async_trait)>>`
= note: required for the cast from `Pin<Box<{async block@$DIR/issue-86507.rs:18:17: 18:27}>>` to `Pin<Box<(dyn Future<Output = ()> + Send + 'async_trait)>>`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going through all of there errors, I wonder if mentioning the end of the span is necessary in messages...

Comment on lines +16 to -23
LL | (async move || {
| - return type of async closure is &'1 i32
...
LL | let y = &*x;
| --- `*x` is borrowed here
LL | *x += 1;
| ^^^^^^^ `*x` is assigned to here but it was already borrowed
LL | y
| - returning this value requires that `*x` is borrowed for `'1`
LL | })()
| - return type of async closure is &'1 i32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<3

Comment on lines +28 to +33
LL | needs_async_fn(async || {
| -------------- ^^^^^^^^
| | |
| | cannot borrow as mutable
| | in this closure
| expects `Fn` instead of `FnMut`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should now remove the "in this closure" label, as it is no longer relevant?

jhpratt added a commit to jhpratt/rust that referenced this pull request Jun 28, 2024
…s, r=oli-obk

Tighten `fn_decl_span` for async blocks

Tightens the span of `async {}` blocks in diagnostics, and subsequently async closures and async fns, by actually setting the `fn_decl_span` correctly. This is kinda a follow-up on rust-lang#125078, but it fixes the problem in a more general way.

I think the diagnostics are significantly improved, since we no longer have a bunch of overlapping spans. I'll point out one caveat where I think the diagnostic may get a bit more confusing, but where I don't think it matters.

r? `@estebank` or `@oli-obk` or someone else on wg-diag or compiler i dont really care lol
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 28, 2024
…s, r=oli-obk

Tighten `fn_decl_span` for async blocks

Tightens the span of `async {}` blocks in diagnostics, and subsequently async closures and async fns, by actually setting the `fn_decl_span` correctly. This is kinda a follow-up on rust-lang#125078, but it fixes the problem in a more general way.

I think the diagnostics are significantly improved, since we no longer have a bunch of overlapping spans. I'll point out one caveat where I think the diagnostic may get a bit more confusing, but where I don't think it matters.

r? ``@estebank`` or ``@oli-obk`` or someone else on wg-diag or compiler i dont really care lol
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 28, 2024
…s, r=oli-obk

Tighten `fn_decl_span` for async blocks

Tightens the span of `async {}` blocks in diagnostics, and subsequently async closures and async fns, by actually setting the `fn_decl_span` correctly. This is kinda a follow-up on rust-lang#125078, but it fixes the problem in a more general way.

I think the diagnostics are significantly improved, since we no longer have a bunch of overlapping spans. I'll point out one caveat where I think the diagnostic may get a bit more confusing, but where I don't think it matters.

r? ```@estebank``` or ```@oli-obk``` or someone else on wg-diag or compiler i dont really care lol
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 28, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#124741 (patchable-function-entry: Add unstable compiler flag and attribute)
 - rust-lang#126470 (make cargo submodule optional)
 - rust-lang#126701 (ignore `llvm::Lld` if lld is not enabled)
 - rust-lang#126956 (core: avoid `extern type`s in formatting infrastructure)
 - rust-lang#126970 (Simplify `str::clone_into`)
 - rust-lang#127058 (Tighten `fn_decl_span` for async blocks)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 28, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#124741 (patchable-function-entry: Add unstable compiler flag and attribute)
 - rust-lang#126470 (make cargo submodule optional)
 - rust-lang#126956 (core: avoid `extern type`s in formatting infrastructure)
 - rust-lang#126970 (Simplify `str::clone_into`)
 - rust-lang#127022 (Support fetching `Attribute` of items.)
 - rust-lang#127058 (Tighten `fn_decl_span` for async blocks)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 89a0cfe into rust-lang:master Jun 28, 2024
6 checks passed
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 28, 2024
Rollup merge of rust-lang#127058 - compiler-errors:tighten-async-spans, r=oli-obk

Tighten `fn_decl_span` for async blocks

Tightens the span of `async {}` blocks in diagnostics, and subsequently async closures and async fns, by actually setting the `fn_decl_span` correctly. This is kinda a follow-up on rust-lang#125078, but it fixes the problem in a more general way.

I think the diagnostics are significantly improved, since we no longer have a bunch of overlapping spans. I'll point out one caveat where I think the diagnostic may get a bit more confusing, but where I don't think it matters.

r? ````@estebank```` or ````@oli-obk```` or someone else on wg-diag or compiler i dont really care lol
@rustbot rustbot added this to the 1.81.0 milestone Jun 28, 2024
flip1995 pushed a commit to flip1995/rust that referenced this pull request Jul 11, 2024
…s, r=oli-obk

Tighten `fn_decl_span` for async blocks

Tightens the span of `async {}` blocks in diagnostics, and subsequently async closures and async fns, by actually setting the `fn_decl_span` correctly. This is kinda a follow-up on rust-lang#125078, but it fixes the problem in a more general way.

I think the diagnostics are significantly improved, since we no longer have a bunch of overlapping spans. I'll point out one caveat where I think the diagnostic may get a bit more confusing, but where I don't think it matters.

r? ````@estebank```` or ````@oli-obk```` or someone else on wg-diag or compiler i dont really care lol
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants