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.

Sign up for GitHub

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

Jump to bottom

Suggest desugaring to return-position impl Future when an async fn in trait fails an auto trait bound #115864

Merged
merged 2 commits into from Sep 21, 2023
Merged

Suggest desugaring to return-position impl Future when an async fn in trait fails an auto trait bound #115864

merged 2 commits into from Sep 21, 2023

Conversation

compiler-errors
Copy link
Member

First commit allows us to store the span of the async keyword in HIR.

Second commit implements a suggestion to desugar an async fn to a return-position impl Future in trait to slightly improve the Send situation being discussed in #115822.

This suggestion is only made when #![feature(return_type_notation)] is not enabled -- if it is, we should instead suggest an appropriate where-clause bound.

@rustbot
Copy link
Collaborator

rustbot commented Sep 15, 2023

r? @jackh726

(rustbot has picked a reviewer for you, use r? to override)

@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. labels Sep 15, 2023
@compiler-errors
Copy link
Member Author

Because of the changes to HIR

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 15, 2023
@bors
Copy link
Contributor

bors commented Sep 15, 2023

⌛ Trying commit fe864be with merge 2fea758...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 15, 2023
@bors
Auto merge of rust-lang#115864 - compiler-errors:rpitit-sugg, r=<try>
2fea758 
Suggest desugaring to return-position `impl Future` when an `async fn` in trait fails an auto trait bound

First commit allows us to store the span of the `async` keyword in HIR.

Second commit implements a suggestion to desugar an `async fn` to a return-position `impl Future` in trait to slightly improve the `Send` situation being discussed in rust-lang#115822.

This suggestion is only made when `#![feature(return_type_notation)]` is not enabled -- if it is, we should instead suggest an appropriate where-clause bound.
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@compiler-errors
Copy link
Member Author

Of course because I'm developing on aarch64, I didn't see any of the type size changes.

@rustbot author

@rustbot rustbot 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 Sep 15, 2023
@bors
Copy link
Contributor

bors commented Sep 15, 2023

💔 Test failed - checks-actions

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@compiler-errors
Copy link
Member Author

@bors try @rust-timer queue

@bors
Copy link
Contributor

bors commented Sep 15, 2023

⌛ Trying commit 733a447 with merge e9015d5...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 15, 2023
@bors
Auto merge of rust-lang#115864 - compiler-errors:rpitit-sugg, r=<try>
e9015d5 
Suggest desugaring to return-position `impl Future` when an `async fn` in trait fails an auto trait bound

First commit allows us to store the span of the `async` keyword in HIR.

Second commit implements a suggestion to desugar an `async fn` to a return-position `impl Future` in trait to slightly improve the `Send` situation being discussed in rust-lang#115822.

This suggestion is only made when `#![feature(return_type_notation)]` is not enabled -- if it is, we should instead suggest an appropriate where-clause bound.
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Sep 15, 2023

💔 Test failed - checks-actions

estebank
estebank reviewed Sep 15, 2023
View reviewed changes
compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs Outdated
Comment on lines 4064 to 4153
// ... which is a local function
let Some(fn_def_id) = fn_def_id.as_local() else {
return;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we maybe mention that the problem lies in the trait definition so they know they have to bother the crate maintainer? Particularly thinking of the case of workspaces where the crate isn't local, but still under the dev's control.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I can do that. I can at least point to the async fn definition if we have a span for that.

@rustbot
Copy link
Collaborator

rustbot commented Sep 15, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Some changes occurred in src/librustdoc/clean/types.rs

cc @camelid

@compiler-errors
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Sep 15, 2023

⌛ Trying commit fae8c23 with merge bb4d374...

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 15, 2023
@bors
Auto merge of rust-lang#115864 - compiler-errors:rpitit-sugg, r=<try>
bb4d374 
Suggest desugaring to return-position `impl Future` when an `async fn` in trait fails an auto trait bound

First commit allows us to store the span of the `async` keyword in HIR.

Second commit implements a suggestion to desugar an `async fn` to a return-position `impl Future` in trait to slightly improve the `Send` situation being discussed in rust-lang#115822.

This suggestion is only made when `#![feature(return_type_notation)]` is not enabled -- if it is, we should instead suggest an appropriate where-clause bound.
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Sep 15, 2023

☀️ Try build successful - checks-actions
Build commit: bb4d374 (bb4d3742a7a4a72dc4a9082e10e3bf2a65e5d8ff)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (bb4d374): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

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

Max RSS (memory usage)

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

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: 633.484s -> 632.201s (-0.20%)
Artifact size: 318.18 MiB -> 318.17 MiB (-0.00%)

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 15, 2023
@compiler-errors
Copy link
Member Author

r? @estebank

@rustbot rustbot assigned estebank and unassigned jackh726 Sep 21, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot
Copy link
Collaborator

rustbot commented Sep 21, 2023

Changes to the size of AST and/or HIR nodes.

cc @nnethercote

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 21, 2023

📌 Commit 9072415 has been approved by estebank

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 21, 2023
@bors
Copy link
Contributor

bors commented Sep 21, 2023

⌛ Testing commit 9072415 with merge b3aa8e7...

@bors
Copy link
Contributor

bors commented Sep 21, 2023

☀️ Test successful - checks-actions
Approved by: estebank
Pushing b3aa8e7 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 21, 2023
@bors bors merged commit b3aa8e7 into rust-lang:master Sep 21, 2023
12 checks passed
@rustbot rustbot added this to the 1.74.0 milestone Sep 21, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b3aa8e7): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

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.0% [2.0%, 2.0%] 1
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) 2.0% [2.0%, 2.0%] 1

Cycles

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)		 - - 0
Regressions ❌
(secondary)		 3.5% [3.5%, 3.5%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 632.84s -> 634.138s (0.21%)
Artifact size: 317.62 MiB -> 317.56 MiB (-0.02%)

@nnethercote
Copy link
Contributor

Of course because I'm developing on aarch64, I didn't see any of the type size changes.

It might be worth changing how that condition works, so that it hits for Mac M1 users.

@compiler-errors
Copy link
Member Author

@nnethercote: unfortunately, can't do it the "simple" way by just enabling the static_assert_size checks on aarch64, since the sizes of items change between x86_64 and aarch64 -- @JakobDegen told me it was probably due to types containing u128s. and I think that maintaining two sets of type sizes is a lot of work 😅

@nnethercote
Copy link
Contributor

I wonder what fraction of the existing size assertions have different results on aarch64 vs x86-64.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estebank estebank estebank approved these changes

Assignees

@estebank estebank

Labels
merged-by-bors This PR was explicitly merged by bors. 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.
Projects
None yet
Milestone
1.74.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@compiler-errors @rustbot @rust-timer @bors @rust-log-analyzer @estebank @nnethercote @jackh726