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 TyAlias in projected_ty #15288

Merged
merged 2 commits into from
Jul 15, 2023
Merged

Handle TyAlias in projected_ty #15288

merged 2 commits into from
Jul 15, 2023

Conversation

alibektas
Copy link
Member

First of all I still have no idea how MIR works but #15143 has been an issue that constantly made RA crash so I have been looking for a way to make RA stop panicking. I have zero claims that what I want to merge has any sense or is correct 😄 but there isn't any more panicking. Even if it is wrong may this be at least a step towards resolving this issue.
As is customary this PR fixes #15143

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 15, 2023
@@ -151,6 +151,7 @@ impl<V, T> ProjectionElem<V, T> {
TyKind::Adt(_, subst) => {
db.field_types(f.parent)[f.local_id].clone().substitute(Interner, subst)
}
TyKind::Alias(al) => al.clone().intern(Interner),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TyKind::Alias(al) => al.clone().intern(Interner),
TyKind::Alias(inner) => al.clone().intern(Interner),

@@ -167,6 +168,7 @@ impl<V, T> ProjectionElem<V, T> {
TyKind::Error.intern(Interner)
}),
TyKind::Closure(id, subst) => closure_field(*id, subst, *f),
TyKind::Alias(al) => al.clone().intern(Interner),
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TyKind::Alias(al) => al.clone().intern(Interner),
TyKind::Alias(inner) => al.clone().intern(Interner),

@lnicola
Copy link
Member

lnicola commented Jul 15, 2023

I'm not completely sure this is correct, but it can't be worse than crashing, right? :-). Can you also find a place to add a test?

@alibektas
Copy link
Member Author

I'm not completely sure this is correct, but it can't be worse than crashing, right? :-). Can you also find a place to add a test?

This was my exact line of thought 😄 . I don't seem to understand why the enclosing function is invoked and I do not see a testing infrastructure that isolates mir from the rest. I would be happy to do it if you could maybe guide me ?

Copy link
Member

@lnicola lnicola left a comment

Choose a reason for hiding this comment

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

Fair enough, I actually don't have a minimization for this.

CC @HKalbasi maybe you know a way to test this?

@lnicola
Copy link
Member

lnicola commented Jul 15, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Jul 15, 2023

📌 Commit f8f19c4 has been approved by lnicola

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jul 15, 2023

⌛ Testing commit f8f19c4 with merge 996e054...

@bors
Copy link
Collaborator

bors commented Jul 15, 2023

☀️ Test successful - checks-actions
Approved by: lnicola
Pushing 996e054 to master...

@bors bors merged commit 996e054 into rust-lang:master Jul 15, 2023
9 checks passed
@HKalbasi
Copy link
Member

I don't think this PR makes any sense. The projected_ty should return the type of place after the applied projection. That is, if the base ty is (i32, u64) and the projection is .1, the result should be u64. This PR doesn't do this, if the goal is to silence the problem, we can just comment out the never! lines.

But what is the problem here? In this code (which hits the never! so it can be used as a unit test):

trait Tr {
    type Ty;
}

struct A;

impl Tr for A {
    type Ty = (u32, i64);
}

struct B<T: Tr> {
    f: <T as Tr>::Ty,
}

fn x(b: B<A>) {
    let f = b.f.0; // here
}

b.f.0 will become a place in mir and it tries to recursively compute its type. But the result of b.f is <A as Tr>::Ty, a TyKind::Alias, and it expects something with projection .0 to have tuple type, so it fails. To solve this problem, we should either:

  • Normalize the base type
  • Store the type inside projection and retire the projection_ty function

@lnicola
Copy link
Member

lnicola commented Jul 15, 2023

Oh, sorry, let's revert this and tell people to use a binary build, if possible.

EDIT: I can't find the revert button, will to it tomorrow.

bors added a commit that referenced this pull request Jul 16, 2023
Revert "Handle TyAlias in projected_ty"

Reverts #15288
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"internal error: entered unreachable code: Only adt has field"
5 participants