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

Encode VariantIdx so we can decode ADT variants in the right order #111494

Merged
merged 1 commit into from
May 13, 2023

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented May 12, 2023

As far as I can tell, we don't guarantee anything about the ordering of DefIds and module children...

The code that motivated this PR (#111483) looks something like:

#[derive(Protocol)]
pub enum Data {
    #[protocol(discriminator(0x00))]
    Disconnect(Disconnect),
    EncryptionRequest,
    /* more variants... */
}

The specific macro (protocol) doesn't really matter, but as far as I can tell (from calls to build_reduced_graph), the presence of that #[protocol(..)] helper attribute causes the def-id of the Disconnect enum variant to be collected after its siblings, and it shows up after the other variants in module_children.

When we decode the variants for Data in a child crate (an example test, in this case), this means that the Disconnect variant is moved to the end of the variants list, and all of the other variants now have incorrect relative discriminant data, causing the ICE.

This PR fixes this by sorting manually by variant index after they are decoded. I guess there are alternative ways of fixing this, such as not reusing module_children_non_reexports to encode the order-sensitive ADT variants, or to do some sorting in rustc_resolve... but none of those seemed particularly satisfying either.

I really struggled to create a reproduction here -- it required at least 3 crates, one of which is a proc macro, and then some code to actually compute discriminants in the child crate... Needless to say, I failed to repro this in a test, but I can confirm that it fixes the regression in #111483. Test exists now.

r? @petrochenkov but feel free to reassign. Again, sorry for no test, but I hope the explanation at least suggests why a fix like this is likely necessary. Feedback is welcome.

@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 May 12, 2023
@compiler-errors compiler-errors added the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 12, 2023
@petrochenkov
Copy link
Contributor

it required at least 3 crates, one of which is a proc macro

I think you could put #[rustfmt::skip] on a variant to delay its "materialization" into a DefId, then you won't need a proc macro.

@petrochenkov
Copy link
Contributor

r=me, preferably with a test #111494 (comment).

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 12, 2023
@compiler-errors
Copy link
Member Author

Took a lot of work, but I found a test that actually reproduced the issue. It was surprisingly hard (and really order-dependent) to load the wrong ADT from the crate metadata...

@bors r=petrochenkov

@bors
Copy link
Contributor

bors commented May 13, 2023

📌 Commit ff54c80 has been approved by petrochenkov

It is now in the queue for this repository.

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 13, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request May 13, 2023
Rollup of 6 pull requests

Successful merges:

 - rust-lang#110454 (Require impl Trait in associated types to appear in method signatures)
 - rust-lang#111096 (Add support for `cfg(overflow_checks)`)
 - rust-lang#111451 (Note user-facing types of coercion failure)
 - rust-lang#111469 (Fix data race in llvm source code coverage)
 - rust-lang#111494 (Encode `VariantIdx` so we can decode ADT variants in the right order)
 - rust-lang#111499 (asm: loongarch64: Drop efiapi)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8c89601 into rust-lang:master May 13, 2023
@rustbot rustbot added this to the 1.71.0 milestone May 13, 2023
@compiler-errors compiler-errors deleted the variant-order branch May 14, 2023 20:53
@apiraino
Copy link
Contributor

Beta backport approved as per compiler team on Zulip

@rustbot label +beta-accepted

@rustbot rustbot added the beta-accepted Accepted for backporting to the compiler in the beta channel. label May 18, 2023
@cuviper cuviper mentioned this pull request May 20, 2023
@cuviper cuviper modified the milestones: 1.71.0, 1.70.0 May 20, 2023
@cuviper cuviper removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 20, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request May 20, 2023
[beta] backport

- debuginfo: split method declaration and definition rust-lang#111167
- Encode VariantIdx so we can decode ADT variants in the right order rust-lang#111494
- Simplify find_width_of_character_at_span. rust-lang#111560

r? cuviper
bors added a commit to rust-lang-ci/rust that referenced this pull request May 22, 2023
…trieb

Add extra debug assertions for equality for Adt/Variant/FieldDef

Would've made it easier to both catch and test rust-lang#111494. Maybe not worth it, since it does mean that the compiler is doing extra work when debug-assertions are enabled, but also that's what debug assertions are for :^)

This is a revival of rust-lang#111523 because I think I pushed an empty branch and bors got a bit too excited it closed the PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. 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
Development

Successfully merging this pull request may close these issues.

6 participants