Skip to content

Conversation

@lowr
Copy link
Contributor

@lowr lowr commented Sep 9, 2022

This PR implements basic type inference for generator and yield expressions.

Things not included in this PR:

  • Generator upvars and generator witnesses are not implemented. They are only used to determine auto trait impls, so basic type inference should be fine without them, but method resolutions with auto trait bounds may not be resolved correctly.

Open questions:

  • I haven't (yet) implemented HirDisplay for TyKind::Generator, so generator types are just shown as "{{generator}}" (in tests, inlay hints, hovers, etc.), which is not really nice. How should we show them?
  • I added moderate amount of stuffs to minicore. I especially didn't want to add impl<T> Deref for &T and impl<T> Deref for &mut T exclusively for tests for generators; should I move them into the test fixtures or can they be placed in minicore?

cc #4309

@lowr
Copy link
Contributor Author

lowr commented Sep 9, 2022

I deliberately did not link #4309 for automatic close because there are still some features missing as mentioned in the PR description. However I think this PR enables type inference in most of the real-world use cases and we might want to close #4309 and create a new issue for the missing features (only if this PR is merged, of course).

@Veykril
Copy link
Member

Veykril commented Sep 12, 2022

I added moderate amount of stuffs to minicore. I especially didn't want to add impl Deref for &T and impl Deref for &mut T exclusively for tests for generators; should I move them into the test fixtures or can they be placed in minicore?

I think those are some key implementations so adding them to minicore seems fine to me

I haven't (yet) implemented HirDisplay for TyKind::Generator, so generator types are just shown as "{{generator}}" (in tests, inlay hints, hovers, etc.), which is not really nice. How should we show them?

Probably just like closures but maybe with gen prefixed or something along those lines?

/// closures, but currently this is the only field that will change there,
/// so it doesn't make sense.
return_ty: Ty,
resume_yield_tys: Option<(Ty, Ty)>,
Copy link
Member

Choose a reason for hiding this comment

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

Comment might be nice about which one the yield and return type is

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added doc comment

@lowr
Copy link
Contributor Author

lowr commented Sep 12, 2022

I haven't (yet) implemented HirDisplay for TyKind::Generator, so generator types are just shown as "{{generator}}" (in tests, inlay hints, hovers, etc.), which is not really nice. How should we show them?

Probably just like closures but maybe with gen prefixed or something along those lines?

I'd love to reuse the closure syntax, but I'm not sure how we can encode yield type of a generator. Another option I thought of was impl Generator<ResumeType, Yield = YieldType, Return = ReturnType>, it's clear but too long to show imo (maybe it's suitable for DisplayTarget::Test though).

@lowr lowr force-pushed the feat/inference-for-generator branch from 3ee75f9 to 4b5a66e Compare September 12, 2022 17:45
@Veykril
Copy link
Member

Veykril commented Sep 13, 2022

I'd love to reuse the closure syntax, but I'm not sure how we can encode yield type of a generator. Another option I thought of was impl Generator<ResumeType, Yield = YieldType, Return = ReturnType>, it's clear but too long to show imo (maybe it's suitable for DisplayTarget::Test though).

We can probably just make something up here like gen<YieldTy> |ResumeType| -> ReturnType or yield<YieldTy> |ResumeType| -> ReturnType?

@lowr
Copy link
Contributor Author

lowr commented Sep 21, 2022

Sorry for the delay! I decided to go for the initial implementation like so: |ResumeType| yields YieldType -> ReturnType.

This is now ready for (another?) full review.

note to self: some parts of generator substitution handling need to be adjusted when we change the generic parameter ordering.

@Veykril
Copy link
Member

Veykril commented Sep 26, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Sep 26, 2022

📌 Commit 9ede5f0 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Sep 26, 2022

⌛ Testing commit 9ede5f0 with merge 1f92965...

@bors
Copy link
Contributor

bors commented Sep 26, 2022

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 1f92965 to master...

@bors bors merged commit 1f92965 into rust-lang:master Sep 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants