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

type certainty: clear DefId when an expression's type changes to non-adt #12591

Merged
merged 1 commit into from Apr 4, 2024

Conversation

y21
Copy link
Member

@y21 y21 commented Mar 29, 2024

Fixes #12585

The root cause of the ICE in the linked issue was in the expression one.x, in the array literal.

The type of one is the One struct: an adt with a DefId, so its certainty is Certain(def_id_of_one). However, the field access .x can then change the type (to i32 here) and that should update that DefId accordingly. It does do that correctly when one.x would be another adt with a DefId:

if let Some(def_id) = adt_def_id(expr_ty) {
certainty.with_def_id(def_id)

but when it isn't an adt and there is no def id (which is the case in the linked issue: one.x is an i32), it keeps the DefId of One, even though that's the wrong type (which would then lead to a contradiction later when joining Certaintys):

} else {
certainty

In particular, in the linked issue, from_array([one.x, two.x]) would try to join the Certainty of the two array elements, which should have been [Certain(None), Certain(None)], because i32s have no DefId, but instead it was [Certain(One), Certain(Two)], because the DefId wasn't cleared from when it was visiting one and two. This is the "contradiction" that could be seen in the ICE message

... so this changes it to clear the DefId when it isn't an adt.

cc @smoelius you implemented this initially in #11135, does this change make sense to you?

changelog: none

@rustbot
Copy link
Collaborator

rustbot commented Mar 29, 2024

r? @Jarcho

rustbot has assigned @Jarcho.
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 the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 29, 2024
@smoelius
Copy link
Contributor

smoelius commented Apr 1, 2024

cc @smoelius you implemented this initially in #11135, does this change make sense to you?

It does. Thanks very much for fixing this, @y21.

@Jarcho
Copy link
Contributor

Jarcho commented Apr 4, 2024

Thank you. @bors r+

@bors
Copy link
Collaborator

bors commented Apr 4, 2024

📌 Commit 9f5d31e has been approved by Jarcho

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Apr 4, 2024

⌛ Testing commit 9f5d31e with merge 8253040...

@bors
Copy link
Collaborator

bors commented Apr 4, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing 8253040 to master...

@bors bors merged commit 8253040 into rust-lang:master Apr 4, 2024
5 checks passed
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.

debug_assert ICE in type_certainty contradiction detection
5 participants