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.

Sign up for GitHub

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

Jump to bottom

Fix non-existent-field ICE for generic fields. #81716

Merged
merged 1 commit into from
Feb 3, 2021
Merged

Fix non-existent-field ICE for generic fields. #81716

merged 1 commit into from
Feb 3, 2021

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Feb 3, 2021

I mentioned this ICE in a chat and it took about 3 milliseconds before @eddyb found the problem and said this change would fix it. :)

This also changes one the field types in the related test to one that triggered the ICE.

Fixes #81627.
Fixes #81672.
Fixes #81709.

Cc #81480 @b-naber @estebank.

@rust-highfive
Copy link
Collaborator

r? @estebank

(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 Feb 3, 2021
@m-ou-se m-ou-se added I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 3, 2021
eddyb
eddyb reviewed Feb 3, 2021
View reviewed changes
compiler/rustc_typeck/src/check/expr.rs
@@ -1974,7 +1974,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

field_path.push(candidate_field.ident.normalize_to_macros_2_0());
let field_ty = candidate_field.ty(self.tcx, subst);
if let Some((nested_fields, _)) = self.get_field_candidates(span, &field_ty) {
if let Some((nested_fields, subst)) = self.get_field_candidates(span, &field_ty) {
Copy link
Member

Choose a reason for hiding this comment

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

Random note about something I noticed, that I don't think is worth touching in this PR: this function used subst instead of substs, and while we do use the name subst, it's for the method (where it means "substitute") - the variables of type Substs ("substitutions") should be called substs.

@eddyb
Copy link
Member

eddyb commented Feb 3, 2021

@bors r+ (what are the chances this gets into nightly?)

@bors
Copy link
Contributor

bors commented Feb 3, 2021

📌 Commit 68cc12a has been approved by eddyb

@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 Feb 3, 2021
@camelid
Copy link
Member

camelid commented Feb 3, 2021

@eddyb you can do @bors p=1 which will make it much more likely it will be in the nightly.

@m-ou-se
Copy link
Member Author

m-ou-se commented Feb 3, 2021

Right now, p=1 would not do much, looking at the bors queue:

image

@camelid
Copy link
Member

camelid commented Feb 3, 2021

Since we've already gone several reports of this, I'll just do it myself :)

@bors p=1

@camelid
Copy link
Member

camelid commented Feb 3, 2021

Right now, p=1 would not do much, looking at the bors queue:

image

Well, it means that it will definitely get in by tomorrow's nightly :)

I think it has about 6 hours to get merged, so might not get in.

@eddyb
Copy link
Member

eddyb commented Feb 3, 2021

@eddyb you can do @bors p=1 which will make it much more likely it will be in the nightly.

Yeah but I didn't even look at the queue, not know enough about what's going on right now to risk messing with priorities.

@camelid
Copy link
Member

camelid commented Feb 3, 2021 •
edited
Loading

bors seems to prioritize within a priority level based on when it was approved, so it shouldn't mess with anything that's already prioritized. But, if you want to you can undo my priority bump, I'm fine either way.

@m-ou-se m-ou-se mentioned this pull request Feb 3, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 3, 2021
@bors
Auto merge of rust-lang#81718 - m-ou-se:rollup-3ftbymt, r=m-ou-se
120b2a7 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#80394 (make const_err a future incompat lint)
 - rust-lang#81532 (Remove incorrect `delay_span_bug`)
 - rust-lang#81692 (Update clippy)
 - rust-lang#81715 (Reduce tab formatting assertions to debug only)
 - rust-lang#81716 (Fix non-existent-field ICE for generic fields.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@b-naber
Copy link
Contributor

b-naber commented Feb 3, 2021

Thanks for fixing this and sorry for introducing the bug. I still don't understand how this change fixes the bug, can anybody explain this, please?

@camelid
Copy link
Member

camelid commented Feb 3, 2021 •
edited
Loading

Thanks for fixing this and sorry for introducing the bug.

That's okay, everyone introduces them :)

I still don't understand how this change fixes the bug, can anybody explain this, please?

My understanding (though I may be totally off-base) is that previously the code was checking for nested fields but in the subst from the parent scope. So when checking the .baz in foo.bar.baz, it's using the subst for foo rather than for foo.bar. It gets confusing because there's shadowing of the subst variable.

@b-naber
Copy link
Contributor

b-naber commented Feb 3, 2021

Oh yes, I see. Thanks.

@bors bors merged commit 4617418 into rust-lang:master Feb 3, 2021
@rustbot rustbot added this to the 1.51.0 milestone Feb 3, 2021
@m-ou-se m-ou-se deleted the fix-ice branch July 24, 2021 08:14
@Enselic Enselic mentioned this pull request Sep 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddyb eddyb eddyb approved these changes

Assignees

@estebank estebank

Labels
I-ICE Issue: The compiler panicked, giving an Internal Compilation Error (ICE) ❄️ S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.51.0
8 participants
@m-ou-se @rust-highfive @eddyb @bors @camelid @b-naber @estebank @rustbot