Skip to content

Conversation

Nadrieril
Copy link
Member

@Nadrieril Nadrieril commented May 23, 2020

This should resolve #72476 and probably #72467.
This is a bit hacky but that's actually what the code was doing before #71930. I'm essentially reverting e5a2cd5. So despite being hacky, it's been tried and tested (so much so that code relies on it now x)).
Only the third commit does anything interesting.

@rust-highfive
Copy link
Contributor

r? @davidtwco

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 23, 2020
@LeSeulArtichaut
Copy link
Contributor

cc @matthewjasper I think

@Nadrieril
Copy link
Member Author

And cc @varkor who reviewed #71930

Copy link
Member

@davidtwco davidtwco left a comment

Choose a reason for hiding this comment

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

LGTM, probably should have someone more experienced look over it though.

@davidtwco
Copy link
Member

r? @matthewjasper

@mark-i-m
Copy link
Member

@Nadrieril Would it make sense to open an issue about properly fixing this?

@Nadrieril
Copy link
Member Author

I guess yeah. I'm not super motivated to do it though, it would take some work to explain properly what the issue is

@matthewjasper
Copy link
Contributor

The issue is that field.ty(..) doesn't normalize it's output after substituting. For example, in the test the field has type <T as A>::Projection, which after substituting becomes <() as A>::Projection. To get the normalized type you either have to explicitly normalize, or use the result from the type of the subpattern, which typeck has already normalized.

I think that I would prefer normalization to be used here, because it's more consistent with how things have to be done in general.

@Nadrieril
Copy link
Member Author

I made this PR as an emergency fixup because the related issue was high priority. I'd still like to fix this properly, but I don't think adding normalization after field.ty would be enough. There's a bunch of different places that inspect types, and I don't understand the rationale and invariants related to type normalization. It's already the second uncaught problem surfaced by this change so I'd prefer to be safe and merge this first

@Nadrieril
Copy link
Member Author

Since #72476 is P-critical, may I insist a bit to get this merged quickly?

@matthewjasper
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented May 27, 2020

📌 Commit 3e8ba3a has been approved by matthewjasper

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 27, 2020
@Nadrieril
Copy link
Member Author

Thank you 🙏

@wesleywiser
Copy link
Member

Per discussion in the compiler team triage meeting today, I'm going to bump the priority on this since it fixes a P-critical issue.

@bors p=1

@bors
Copy link
Collaborator

bors commented May 28, 2020

⌛ Testing commit 3e8ba3a with merge 7c0b6f8f94538a4818d099aaae778813a4d3303e...

@Dylan-DPC-zz
Copy link

@bors retry yield

@bors
Copy link
Collaborator

bors commented May 28, 2020

⌛ Testing commit 3e8ba3a with merge 052c781a6ae34fbd658d72226577c5cfb6be84b3...

@Dylan-DPC-zz
Copy link

@bors retry yield

@bors
Copy link
Collaborator

bors commented May 29, 2020

⌛ Testing commit 3e8ba3a with merge 9cd3f1c...

@bors
Copy link
Collaborator

bors commented May 29, 2020

☀️ Test successful - checks-azure
Approved by: matthewjasper
Pushing 9cd3f1c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 29, 2020
@bors bors merged commit 9cd3f1c into rust-lang:master May 29, 2020
@Nadrieril Nadrieril deleted the fix-72476 branch November 6, 2020 23:39
@Nadrieril
Copy link
Member Author

The issue is that field.ty(..) doesn't normalize it's output after substituting.

Oh, I just understood this comment x) The error popped up again (in #89393) because I hadn't fixed it properly. I understand how to now.

@Nadrieril Nadrieril added the A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns label Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect non-exhaustive pattern error
10 participants