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

Make type_of for RPITITs GAT on traits return a ty::Error if no default provided #113461

Closed
wants to merge 1 commit into from

Conversation

spastorino
Copy link
Member

Fixes #113434

This one is interesting because it ICEs on both master and in the new RPITIT impl.

I think the code is self-explanatory but we want to avoid delaying a bug and calling opaque::find_opaque_ty_constraints_for_rpit(tcx, def_id, owner) when we already know we can't build a type.

r? @compiler-errors

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 7, 2023
@compiler-errors
Copy link
Member

Why exactly do we even try to call type_of on the RPITIT in the first place?

@spastorino
Copy link
Member Author

Why exactly do we even try to call type_of on the RPITIT in the first place?

Something interesting is that ...

#![feature(return_position_impl_trait_in_trait)]

struct Wrapper<G: Send>();

trait Foo {
    fn bar() -> Wrapper<impl Send>;
}

fn main() {}

This example doesn't ICE, so I guess doesn't call type_of

@spastorino
Copy link
Member Author

spastorino commented Jul 7, 2023

Why exactly do we even try to call type_of on the RPITIT in the first place?

So from what I see we're trying to check Foo::bar is WF.

This is the process we follow with the RPITIT def_id

check_associated_item
assumed_wf_types_and_report_errors
iterates over assumed_wf_types results and calls deeply_normalize
select_where_possible
select
process_obligations
process_trait_obligation
poly_select
confirm_candidate
confirm_auto_impl_candidate
constituent_types_for_ty
type_of

This process seems reasonable to me but I'm not sure if we should change something and what exactly. So we select an auto impl candidate because of impl Sized, then need to call constituent_types_for_ty which calls type_of for opaques. Unless we want to do something special here https://github.com/spastorino/rust/blob/ca94899db9ec4fe27bcda746bfda537402aff666/compiler/rustc_trait_selection/src/traits/select/mod.rs#L2352 I'm not sure what to do exactly.

@spastorino
Copy link
Member Author

Another interesting thing is that this example:

#![feature(return_position_impl_trait_in_trait)]

struct Wrapper<G: Sized>(G);

trait Foo {
    fn bar() -> Wrapper<impl Send>;
}

fn main() {}

Doesn't produce any issue. We need the impl trait to be an impl auto trait in return position in order to reproduce this.

@compiler-errors
Copy link
Member

I don't think that's enough of an explanation for why this is happening. All you've provided is the call stack of the ICE and a couple of examples that don't ICE 😅

doesn't explain why we're registering this obligation and why we're actually getting the opaque that has no hidden type.

This at least needs a bit more of an explanation so I can be convinced there's some better place to put this fix, without having to actually investigate it myself...

@rustbot author

@rustbot rustbot 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 Jul 10, 2023
@spastorino
Copy link
Member Author

Closing in favor of #113741

@spastorino spastorino closed this Jul 16, 2023
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Jul 31, 2023
…-missing-opaque, r=spastorino

Don't install default projection bound for return-position `impl Trait` in trait methods with no body

This ensures that we never try to project to an opaque type in a trait method that has no body to infer its hidden type, which means we never later call `type_of` on that opaque. This is because opaque types try to reveal their hidden type when proving auto traits.

I thought about this a lot, and I think this is a fix that's less likely to introduce other strange downstream ICEs than rust-lang#113461.

r? `@spastorino`
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Jul 31, 2023
…-missing-opaque, r=spastorino

Don't install default projection bound for return-position `impl Trait` in trait methods with no body

This ensures that we never try to project to an opaque type in a trait method that has no body to infer its hidden type, which means we never later call `type_of` on that opaque. This is because opaque types try to reveal their hidden type when proving auto traits.

I thought about this a lot, and I think this is a fix that's less likely to introduce other strange downstream ICEs than rust-lang#113461.

Fixes rust-lang#113434

r? `@spastorino`
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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ICE: tried to get type of this RPITIT with no definition
3 participants