-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
async/await: improve not-send errors, part 2 #65345
async/await: improve not-send errors, part 2 #65345
Conversation
src/test/ui/async-await/issue-64130-non-send-future-diags.stderr
Outdated
Show resolved
Hide resolved
cba981a
to
15ebeab
Compare
This comment has been minimized.
This comment has been minimized.
964759e
to
6d16511
Compare
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.
Updated the PR (with help from @nikomatsakis) so that it now applies to more cases.
let ty = ty.builtin_deref(false).map(|ty_and_mut| ty_and_mut.ty).unwrap_or(ty); | ||
let target_ty = target_ty.builtin_deref(false) | ||
.map(|ty_and_mut| ty_and_mut.ty) | ||
.unwrap_or(target_ty); |
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 is hack that I wasn't able to find a better solution for.
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.
What problem is it solving, exactly? I guess that we sometimes capture things "by reference"?
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 wasn’t able to find a way to check for equality of types that were ty::Ref
, nothing seemed to work. There are some messages in the Zulip thread detailing what I tried when I was last looking at this. Nope, misremembered.
LL | fut().await; | ||
| ^^^^^^^^^^^ await occurs here, with `x` maybe used later | ||
LL | } | ||
| - `x` is later dropped here |
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 diagnostic is misleading.
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.
Hmm, because of the drop(x)
that comes before? I suppose that's true, though it'd ordinarily be quite helpful. Perhaps we can just weaken the language to "x may be dropped here"?
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.
Yeah, I think we could adjust the wording to better explain why the drop
call isn’t enough to avoid the type being held over the await point.
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 consider this a bit of a bug, also -- it'd be nice if we were more precise around particularly this case)
@davidtwco should this still be considered "draft"? |
Probably not, I’ve unmarked it. |
This comment has been minimized.
This comment has been minimized.
473d24e
to
6c18dd6
Compare
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.
uh I had some comments I apparently never posted :(
}; | ||
|
||
// Look at the last interior type to get a span for the `.await`. | ||
let await_span = tables.generator_interior_types.iter().map(|i| i.span).last().unwrap(); |
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.
huh, so we just take the last span from the table? seems a bit arbitrary. Ideally, it would be tied to the value we are citing..?
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.
but I guess that's pre-existing
|
||
// Get the tables from the infcx if the generator is the function we are | ||
// currently type-checking; otherwise, get them by performing a query. | ||
// This is needed to avoid cycles. |
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 wonder if we want a helper function for this
let ty = ty.builtin_deref(false).map(|ty_and_mut| ty_and_mut.ty).unwrap_or(ty); | ||
let target_ty = target_ty.builtin_deref(false) | ||
.map(|ty_and_mut| ty_and_mut.ty) | ||
.unwrap_or(target_ty); |
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.
What problem is it solving, exactly? I guess that we sometimes capture things "by reference"?
Ping from triage: |
Ping from triage, any updates? @nikomatsakis |
ping from triage: |
Signed-off-by: David Wood <[email protected]>
This commit corrects the diagnostic note for `async move {}` so that `await` is mentioned, rather than `yield`. Signed-off-by: David Wood <[email protected]>
6c18dd6
to
f0b5114
Compare
The reason we were invoking `builtin_deref` was to enable comparisons when the type was `&T`. For the reasons outlined in the comment, those comparisons failed because the regions disagreed.
@bors r+ (Tests pass locally) |
📌 Commit 5cd9f22 has been approved by |
…provements, r=nikomatsakis async/await: improve not-send errors, part 2 Part of #64130. Fixes #65667. This PR improves the errors introduced in #64895 so that they have specialized messages for `Send` and `Sync`. r? @nikomatsakis
☀️ Test successful - checks-azure |
Part of #64130. Fixes #65667.
This PR improves the errors introduced in #64895 so that they have specialized messages for
Send
andSync
.r? @nikomatsakis