Skip to content

Commit

Permalink
Rollup merge of #111494 - compiler-errors:variant-order, r=petrochenkov
Browse files Browse the repository at this point in the history
Encode `VariantIdx` so we can decode ADT variants in the right order

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

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

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

The specific macro ([`protocol`](https://github.com/dylanmckay/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.
  • Loading branch information
Dylan-DPC committed May 13, 2023
2 parents 770fd73 + ff54c80 commit 8c89601
Show file tree
Hide file tree
Showing 5 changed files with 58 additions and 22 deletions.
57 changes: 36 additions & 21 deletions compiler/rustc_metadata/src/rmeta/decoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -856,7 +856,12 @@ impl<'a, 'tcx> CrateMetadataRef<'a> {
ty::EarlyBinder(&*output)
}

fn get_variant(self, kind: &DefKind, index: DefIndex, parent_did: DefId) -> ty::VariantDef {
fn get_variant(
self,
kind: DefKind,
index: DefIndex,
parent_did: DefId,
) -> (VariantIdx, ty::VariantDef) {
let adt_kind = match kind {
DefKind::Variant => ty::AdtKind::Enum,
DefKind::Struct => ty::AdtKind::Struct,
Expand All @@ -870,22 +875,25 @@ impl<'a, 'tcx> CrateMetadataRef<'a> {
if adt_kind == ty::AdtKind::Enum { Some(self.local_def_id(index)) } else { None };
let ctor = data.ctor.map(|(kind, index)| (kind, self.local_def_id(index)));

ty::VariantDef::new(
self.item_name(index),
variant_did,
ctor,
data.discr,
self.get_associated_item_or_field_def_ids(index)
.map(|did| ty::FieldDef {
did,
name: self.item_name(did.index),
vis: self.get_visibility(did.index),
})
.collect(),
adt_kind,
parent_did,
false,
data.is_non_exhaustive,
(
data.idx,
ty::VariantDef::new(
self.item_name(index),
variant_did,
ctor,
data.discr,
self.get_associated_item_or_field_def_ids(index)
.map(|did| ty::FieldDef {
did,
name: self.item_name(did.index),
vis: self.get_visibility(did.index),
})
.collect(),
adt_kind,
parent_did,
false,
data.is_non_exhaustive,
),
)
}

Expand All @@ -901,7 +909,7 @@ impl<'a, 'tcx> CrateMetadataRef<'a> {
};
let repr = self.root.tables.repr_options.get(self, item_id).unwrap().decode(self);

let variants = if let ty::AdtKind::Enum = adt_kind {
let mut variants: Vec<_> = if let ty::AdtKind::Enum = adt_kind {
self.root
.tables
.module_children_non_reexports
Expand All @@ -912,15 +920,22 @@ impl<'a, 'tcx> CrateMetadataRef<'a> {
let kind = self.def_kind(index);
match kind {
DefKind::Ctor(..) => None,
_ => Some(self.get_variant(&kind, index, did)),
_ => Some(self.get_variant(kind, index, did)),
}
})
.collect()
} else {
std::iter::once(self.get_variant(&kind, item_id, did)).collect()
std::iter::once(self.get_variant(kind, item_id, did)).collect()
};

tcx.mk_adt_def(did, adt_kind, variants, repr)
variants.sort_by_key(|(idx, _)| *idx);

tcx.mk_adt_def(
did,
adt_kind,
variants.into_iter().map(|(_, variant)| variant).collect(),
repr,
)
}

fn get_visibility(self, id: DefIndex) -> Visibility<DefId> {
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_metadata/src/rmeta/encoder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1375,9 +1375,10 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
// Therefore, the loop over variants will encode its fields as the adt's children.
}

for variant in adt_def.variants().iter() {
for (idx, variant) in adt_def.variants().iter_enumerated() {
let data = VariantData {
discr: variant.discr,
idx,
ctor: variant.ctor.map(|(kind, def_id)| (kind, def_id.index)),
is_non_exhaustive: variant.is_field_list_non_exhaustive(),
};
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_metadata/src/rmeta/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ use rustc_span::edition::Edition;
use rustc_span::hygiene::{ExpnIndex, MacroKind};
use rustc_span::symbol::{Ident, Symbol};
use rustc_span::{self, ExpnData, ExpnHash, ExpnId, Span};
use rustc_target::abi::VariantIdx;
use rustc_target::spec::{PanicStrategy, TargetTriple};

use std::marker::PhantomData;
Expand Down Expand Up @@ -430,6 +431,7 @@ define_tables! {

#[derive(TyEncodable, TyDecodable)]
struct VariantData {
idx: VariantIdx,
discr: ty::VariantDiscr,
/// If this is unit or tuple-variant/struct, then this is the index of the ctor id.
ctor: Option<(CtorKind, DefIndex)>,
Expand Down
7 changes: 7 additions & 0 deletions tests/ui/enum-discriminant/auxiliary/discr-foreign-dep.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
#[derive(Default)]
pub enum Foo {
A(u32),
#[default]
B,
C(u32),
}
11 changes: 11 additions & 0 deletions tests/ui/enum-discriminant/discr-foreign.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// aux-build:discr-foreign-dep.rs
// build-pass

extern crate discr_foreign_dep;

fn main() {
match Default::default() {
discr_foreign_dep::Foo::A(_) => {}
_ => {}
}
}

0 comments on commit 8c89601

Please sign in to comment.