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

fix: Fix return type of async closures. #12958

Merged
merged 3 commits into from Mar 15, 2023
Merged

Conversation

zachs18
Copy link
Contributor

@zachs18 zachs18 commented Aug 7, 2022

May fix #12957

Comment on lines 241 to 247
// Use the first type parameter as the output type of future.
// existential type AsyncBlockImplTrait<InnerType>: Future<Output = InnerType>
let impl_trait_id = crate::ImplTraitId::AsyncBlockTypeImplTrait(self.owner, *body);
let opaque_ty_id = self.db.intern_impl_trait_id(impl_trait_id).into();
dbg!(&ret_ty);
TyKind::OpaqueType(opaque_ty_id, Substitution::from1(Interner, ret_ty))
.intern(Interner)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was mostly copied from the Expr::Async arm on line 152 above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This uses AsyncBlockTypeImplTrait but isn't an async block.
Should a new variant be added to ImplTraitId? Or maybe AsyncBlockTypeImplTrait should be renamed?

@zachs18 zachs18 marked this pull request as ready for review August 7, 2022 02:21
@zachs18
Copy link
Contributor Author

zachs18 commented Aug 7, 2022

Actually, this doesn't yet fix the issue, it gives impl Future<Output = {unknown}>, not impl Future<Output = u8> as it should.
image
image

(I also forgot to tidy, oops)

@zachs18 zachs18 marked this pull request as draft August 7, 2022 02:31
@zachs18 zachs18 force-pushed the async_closure branch 2 times, most recently from 20d3b5a to 2a6bdd1 Compare August 7, 2022 04:20
Comment on lines +91 to +93
let f = async move |x: i32| x + 42;
f;
// ^ |i32| -> impl Future<Output = i32>
Copy link
Contributor Author

@zachs18 zachs18 Aug 7, 2022

Choose a reason for hiding this comment

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

This probably shouldn't need x: i32, but otherwise it deduces the type as |i32| -> impl Future<Output = {unknown}>. It's not an issue with this PR though, and it also happens with non-async closures.
image
image

@zachs18 zachs18 marked this pull request as ready for review August 7, 2022 05:57
@zachs18
Copy link
Contributor Author

zachs18 commented Aug 7, 2022

It now gives correct hints for the example in #12957.

image

(The {unknown} at the end of test_params is not an issue with this PR, as it also occurs for non-async closures, and on the master branch).

@bors
Copy link
Collaborator

bors commented Sep 1, 2022

☔ The latest upstream changes (presumably #13165) made this pull request unmergeable. Please resolve the merge conflicts.

@zachs18 zachs18 force-pushed the async_closure branch 2 times, most recently from 2afae76 to f9e8c6e Compare September 6, 2022 23:55
@bors
Copy link
Collaborator

bors commented Sep 26, 2022

☔ The latest upstream changes (presumably #13209) made this pull request unmergeable. Please resolve the merge conflicts.

@Veykril Veykril added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 3, 2022
@Veykril
Copy link
Member

Veykril commented Jan 9, 2023

Can someone knowing the type system side of r-a have a look at this?

@Veykril
Copy link
Member

Veykril commented Jan 17, 2023

There landed some changes revolving the closure inference code (we have landed support for generators), can you update this PR to make use of the same machinery? That will probably tell if your current approach works out fine as well.

@Veykril Veykril 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 Jan 19, 2023
@zachs18
Copy link
Contributor Author

zachs18 commented Jan 20, 2023

Yes, I should have time this weekend to look into that.

@zachs18
Copy link
Contributor Author

zachs18 commented Jan 23, 2023

I rebased, but I didn't change to use the new machinery yet. I should be able to look into that sometime this week.

@bors
Copy link
Collaborator

bors commented Mar 3, 2023

☔ The latest upstream changes (presumably #14222) made this pull request unmergeable. Please resolve the merge conflicts.

@Veykril
Copy link
Member

Veykril commented Mar 15, 2023

I went ahead and moved this to the new machinery, I hope you don't mind :)

@Veykril
Copy link
Member

Veykril commented Mar 15, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 15, 2023

📌 Commit 3bf07a5 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Mar 15, 2023

⌛ Testing commit 3bf07a5 with merge 8330f8e...

@bors
Copy link
Collaborator

bors commented Mar 15, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 8330f8e to master...

@bors bors merged commit 8330f8e into rust-lang:master Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect type hints for async closures (nightly feature async_closure).
3 participants