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

handle_opaque_type likely mis-uses eq relation #101186

Closed
oli-obk opened this issue Aug 30, 2022 · 4 comments
Closed

handle_opaque_type likely mis-uses eq relation #101186

oli-obk opened this issue Aug 30, 2022 · 4 comments
Assignees
Labels
A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch.

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Aug 30, 2022

I was unable to reproduce the issue in the defining scope (that doesn't mean it's not possible, just that I gave up for now ^^). I found this absolutely useless diagnostic though: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=b80473c86172959763c139535154c3f9

Did some more thinking and I'm wondering now if the did.is_local() check should actually be infcx.opaque_type_origin(did).is_some() so we only generate those generalized inference vars if we're actually able to handle the opaque type. I was kinda hoping to keep that isolated in handle_opaque_type, but that appears to have been a misplaced hope.

One more thing is that we're using eq inside handle_opaque_types when it should probably be using the relation that is calling handle_opaque_types. So maybe we could scrap the entire generalization logic and just always invoke handle_opaque_types by passing the relation as an argument and forwarding to that instead of using eq.

so many possibilities to try out. Let's merge this PR for now and I'm moving this comment to an issue so we can continue there.

Originally posted by @oli-obk in #99928 (comment)

@oli-obk oli-obk self-assigned this Aug 30, 2022
@oli-obk oli-obk added the A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch. label Aug 30, 2022
@compiler-errors
Copy link
Member

Did some more thinking and I'm wondering now if the did.is_local() check should actually be infcx.opaque_type_origin(did).is_some() so we only generate those generalized inference vars if we're actually able to handle the opaque type.

Yeah, I've considered this too.

One more thing is that we're using eq inside handle_opaque_types when it should probably be using the relation that is calling handle_opaque_types.

Do we even need to pass down the relation? We just re-invoke eq just to turn it into an error -- could we just return Err(Sorts(..)) right there?

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 30, 2022

while we could fix that site, the reason we are generalizing at all is the other eq in that file that merges the hidden types. If we avoided the generalization and instead replaced that eq with invoking the relation, we may just get generalization "for free" and thus avoid most of the issues here.

@compiler-errors
Copy link
Member

Ah yeah, totally overlooked the success case and was focusing too much on the error case 😅

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Oct 27, 2022
…r=compiler-errors

Clean up hidden type registration

work on rust-lang#101186

Actually passing down the relation and using it instead of `eq` for the hidden type comparison has *no* effect whatsoever and allows for no further improvements at the call sites. I decided the increased complexity was not worth it and thus did not include that change in this PR.

r? `@compiler-errors`
@oli-obk
Copy link
Contributor Author

oli-obk commented Oct 30, 2022

I believe we've cleaned up all we can here

@oli-obk oli-obk closed this as completed Oct 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-impl-trait Area: `impl Trait`. Universally / existentially quantified anonymous types with static dispatch.
Development

No branches or pull requests

2 participants