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

Implement return position impl trait / opaque type support #4689

Merged
merged 2 commits into from Jun 5, 2020

Conversation

flodiebold
Copy link
Member

This is working, but I'm not that happy with how the lowering works. We might need an additional representation between TypeRef and Ty where names are resolved and impl Trait bounds are separated out, but things like inference variables don't exist and impl Trait is always represented the same way.

Also note that this doesn't implement correct handling of RPIT inside the function (which involves turning the impl Traits into variables and creating obligations for them). That intermediate representation might help there as well.

@matklad
Copy link
Member

matklad commented Jun 1, 2020

We might need an additional representation between TypeRef and Ty where names are resolved

Makes sense to me. More generally, after we do ItemTree work, it might makes sense to only store resolved things in StructData and such.

Copy link
Member

@matklad matklad left a comment

Choose a reason for hiding this comment

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

LGTM with new query accounted for in memory usage stats

crates/ra_hir_ty/src/display.rs Outdated Show resolved Hide resolved
TypeCtor::OpaqueType(opaque_ty_id) => {
let bounds = match opaque_ty_id {
crate::OpaqueTyId::ReturnTypeImplTrait(func, idx) => {
let datas =
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should switch to data/datum terminlology...

let func = match ctx.resolver.generic_def() {
Some(GenericDefId::FunctionId(f)) => f,
_ => {
// this shouldn't happen
Copy link
Member

Choose a reason for hiding this comment

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

This needs an assert or an explanation why it isn't an assert

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, added a panic instead. I'm sometimes not sure whether to assert or just ignore things like this in cases where there's an easy way to bail out without panicking.

Copy link
Member

Choose a reason for hiding this comment

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

Good question...

I think for compilery things, it's better to panic, but for IDE things, it's probably better to try to soldier on.

I guess, at least debug_assert always makes sense.

crates/ra_hir_ty/src/traits/chalk.rs Show resolved Hide resolved
crates/ra_hir_ty/src/tests/traits.rs Outdated Show resolved Hide resolved
crates/ra_hir_ty/src/db.rs Show resolved Hide resolved
flodiebold and others added 2 commits June 5, 2020 17:08
This is working, but I'm not that happy with how the lowering works. We might
need an additional representation between `TypeRef` and `Ty` where names are
resolved and `impl Trait` bounds are separated out, but things like inference
variables don't exist and `impl Trait` is always represented the same
way.

Also note that this doesn't implement correct handling of RPIT *inside* the
function (which involves turning the `impl Trait`s into variables and creating
obligations for them). That intermediate representation might help there as
well.
@flodiebold
Copy link
Member Author

@matklad, I addressed your comments.

@matklad
Copy link
Member

matklad commented Jun 5, 2020

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 5, 2020

@bors bors bot merged commit 2a3ab7f into rust-lang:master Jun 5, 2020
@lnicola
Copy link
Member

lnicola commented Jun 5, 2020

Expressions of unknown type: 2898 (1%)
Expressions of partially unknown type: 2877 (1%)
Type mismatches: 748

Expressions of unknown type: 2166 (1%)
Expressions of partially unknown type: 2217 (1%)
Type mismatches: 724

🎉

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.

None yet

3 participants