-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Improve async-await/generator obligation errors in some cases #70679
Conversation
src/librustc_trait_selection/traits/error_reporting/suggestions.rs
Outdated
Show resolved
Hide resolved
src/librustc_trait_selection/traits/error_reporting/suggestions.rs
Outdated
Show resolved
Hide resolved
src/librustc_trait_selection/traits/error_reporting/suggestions.rs
Outdated
Show resolved
Hide resolved
src/librustc_trait_selection/traits/error_reporting/suggestions.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good. Left some thoughts.
src/librustc_trait_selection/traits/error_reporting/suggestions.rs
Outdated
Show resolved
Hide resolved
@nikomatsakis I updated the wording, what do you think? It's a little repetitive in some places, but by and large I think it's for the best. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these are looking much better, yes.
= help: the trait `std::marker::Sync` is not implemented for `std::cell::RefCell<i32>` | ||
= note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<std::cell::RefCell<i32>>` | ||
= note: required because it appears within the type `[generator@$DIR/issue-68112.rs:38:5: 41:6 t:std::sync::Arc<std::cell::RefCell<i32>> {()}]` | ||
= note: required because it appears within the type `impl std::ops::Generator` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this backtrace is confusing, but that's a separate issue I think =)
@@ -5,14 +5,14 @@ LL | fn spawn<T: Send>(_: T) {} | |||
| ----- ---- required by this bound in `spawn` | |||
... | |||
LL | spawn(async { | |||
| ^^^^^ future is not `Send` | |||
| ^^^^^ future created by async block is not `Send` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems good to me
@bors r+ |
📌 Commit 889cfe1 has been approved by |
Improve async-await/generator obligation errors in some cases Fixes rust-lang#68112. This change is best read one commit at a time (I add a test at the beginning and update it in each change after). The `test2` function is a case I found while writing the test that we don't handle with this code yet. I don't attempt to fix it in this PR, but it's a good candidate for future work. r? @davidtwco, @nikomatsakis
🔒 Merge conflict This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again. How do I rebase?Assuming
You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial. Please avoid the "Resolve conflicts" button on GitHub. It uses Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Error message
|
☔ The latest upstream changes (presumably #69707) made this pull request unmergeable. Please resolve the merge conflicts. |
I personally find this clearer.
@bors r=nikomatsakis |
📌 Commit 4326d95 has been approved by |
☀️ Test successful - checks-azure |
Fixes #68112.
This change is best read one commit at a time (I add a test at the beginning and update it in each change after).
The
test2
function is a case I found while writing the test that we don't handle with this code yet. I don't attempt to fix it in this PR, but it's a good candidate for future work.r? @davidtwco, @nikomatsakis