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

only use generic info when ty var belong it in orphan check #132904

Closed
wants to merge 1 commit into from

Conversation

bvanjoi
Copy link
Contributor

@bvanjoi bvanjoi commented Nov 11, 2024

Fixes #132826

This issue is caused by incorrect usage of a type variable. I will use the following example to illustrate:

// foregin.rs
pub trait ForeignTrait {}
pub struct ForeignType<T>(pub T);

// lib.rs
extern crate foregin;
use foreign_crate::{ForeignType, ForeignTrait};

trait MyTrait {
    type Item;
}

impl<M> MyTrait for ForeignType<M> { // `impl#0`
    type Item = ForeignType<M>;
}

impl<K> ForeignTrait for <ForeignType<K> as MyTrait>::Item {} // `impl#1`

When we check the orphan rule for impl#1, we create two type variables: ty_var(K) and ty_var(M), which come from impl#1 and impl#0 respectively. After normalizing <ForeignType<K> as MyTrait>::Item, it returns ty_var(M), which is then used to find the generics information of impl#1, causing a panic due to it not being found.

So naturally, I think there are three ways to solve this issue:

  • Try to instantiate ty_var(M) into ty_var(K) since their relationship is similar to that of a parameter and an argument. And then we can use ty_var(K) as the correct key for the generic.
  • During type folding, find the correct type variable among all those that have an equality relation with ty_var(M). The pseudocode might be:
fn fold_ty() {
  //...
  let vid = [vid, all_eq_relations_of(vid)]
          .find(|ty| 
              generics.param_def_id_to_index.contains(ty.origin.param_def_id))
          .unwrap();
   // ....
}
  • The final approach, as shown in this PR, is to use the generics information only if the type variable belongs to this set of generics. I chose this method because the above methods are too complex, and this approach can easily resolve the issue.

r? @fmease

@rustbot
Copy link
Collaborator

rustbot commented Nov 11, 2024

Could not assign reviewer from: fmease.
User(s) fmease are either the PR author, already assigned, or on vacation, and there are no other candidates.
Use r? to specify someone else to assign.

@rustbot
Copy link
Collaborator

rustbot commented Nov 11, 2024

r? @davidtwco

rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Nov 11, 2024
@fmease fmease assigned fmease and unassigned davidtwco Nov 11, 2024
@bors
Copy link
Contributor

bors commented Mar 21, 2025

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

@fmease
Copy link
Member

fmease commented Mar 21, 2025

Thanks for trying to this fix and sorry for not having responded until now. TyVarFolder as written by me was definitely quite buggy. Making it not panic if it encounters a different type param would've only papered over all of its flaws and it likely would've remained buggy regardless.

Errs has submitted an alternative approach, #138727, which has since been merged. I'll close this PR now. I'm sorry.

@fmease fmease closed this Mar 21, 2025
@fmease fmease removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Using a trait's Associated Type in a different trait's impl declaration crashes
5 participants