enum variant support #89745

zhamlin · 2021-10-10T19:49:30Z

Proof of concept for enum variant types.

rust-highfive · 2021-10-10T19:49:33Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

rust-highfive · 2021-10-10T19:49:33Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @michaelwoerister (or someone else) soon.

Please see the contribution instructions for more information.

oli-obk · 2021-10-11T08:02:48Z

r? @oli-obk

oli-obk

First of all, this is amazing. I thought an MVP would be significantly more involved, but you even got surface syntax working from the start.

I left a lot of comments, I do realize this is a draft and as a first iteration the copy paste is expected to get things running quickly. Please let me know how nit-picky you want the review to be. I'm also available on zulip for chatting if you prefer, just open a thread in the compiler team stream.

One general thing is that I think we should also change TyLayout::for_variant to return a ty::Variant in the type field instead of just returning the original type.

oli-obk · 2021-10-11T07:57:58Z

compiler/rustc_borrowck/src/type_check/mod.rs

+                ty::Variant(ty, _) => match ty.kind() {
+                    ty::Adt(adt_def, _substs) if adt_def.is_enum() => {
+                        if index.as_usize() >= adt_def.variants.len() {
+                            PlaceTy::from_ty(span_mirbug_and_err!(
+                                self,
+                                place,
+                                "cast to variant #{:?} but enum only has {:?}",
+                                index,
+                                adt_def.variants.len()
+                            ))
+                        } else {
+                            PlaceTy { ty: *ty, variant_index: Some(index) }
+                        }
+                    }
+                    _ => bug!("unexpected type: {:?}", ty.kind()),
+                },


is downcasting a variant type an operation we would ever encounter?

I think for now this arm can just mir-bug if index != var_index where var_index is the second field of ty::Variant.

that would allow re-downcasting to the same variant, but nothing else

oli-obk · 2021-10-11T08:00:58Z

compiler/rustc_borrowck/src/type_check/mod.rs

@@ -744,6 +744,22 @@ impl<'a, 'b, 'tcx> TypeVerifier<'a, 'b, 'tcx> {
                        PlaceTy { ty: base_ty, variant_index: Some(index) }


Ideally we'd remove the variant_index field and replace ty here with the variant type

oli-obk · 2021-10-11T08:03:12Z

compiler/rustc_borrowck/src/type_check/mod.rs

            PlaceTy { ty, variant_index: Some(variant_index) } => match *ty.kind() {
                ty::Adt(adt_def, substs) => (&adt_def.variants[variant_index], substs),
+                ty::Variant(ty, _) => match ty.kind() {
+                    ty::Adt(adt_def, substs) => {
+                        (adt_def.variants.get(variant_index).expect(""), *substs)
+                    }
+                    _ => bug!("unexpected type: {:?}", ty.kind()),
+                },


if we get rid of variant_index as mentioned earlier, then we can grab the variant index from the ty::Variant

If we remove variant_index how would we handle the Adt match arm above?

If the above comment is implemented, then we should never encounter that arm, it should always be ty::Variant if we would have had a variant index in the PlaceTy

oli-obk · 2021-10-11T08:42:49Z

compiler/rustc_const_eval/src/interpret/operand.rs

+                    ty::Variant(ty, _) => match ty.kind() {
+                        ty::Adt(adt, _) => adt.discriminants(*self.tcx).find(|(_, var)| var.val == discr_bits),
+                        _ => bug!("unexpected type: {:?}", ty.kind()),
+                    }


Suggested change

ty::Variant(ty, _) => match ty.kind() {

ty::Adt(adt, _) => adt.discriminants(*self.tcx).find(|(_, var)| var.val == discr_bits),

_ => bug!("unexpected type: {:?}", ty.kind()),

}

ty::Variant(_ty, idx) => idx,

This should return (VariantIdx, Discr) not just the index so this wouldn't work?

ah, yea, that makes sense. we should probably still assert somewhere that the index matches the expected one here

oli-obk · 2021-10-11T08:47:18Z

compiler/rustc_const_eval/src/interpret/validity.rs

@@ -219,6 +219,10 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
                if tag_field == field {
                    return match layout.ty.kind() {
                        ty::Adt(def, ..) if def.is_enum() => PathElem::EnumTag,
+                        ty::Variant(ty, ..) => match ty.kind() {


Instead of any of the changes in this function, we could do a

if let ty::Variant(ty, ..) = *layout.ty.kind() { return self.aggregate_field_path_elem(self.ecx.layout_of(ty), field), }

at the start

This is in a Variants::Multiple match arm; we should never see ty::Variant here I think.

oli-obk · 2021-10-11T09:49:45Z

compiler/rustc_typeck/src/check/expr.rs

@@ -308,7 +373,20 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                self.check_expr_closure(expr, capture, &decl, body_id, gen, expected)
            }
            ExprKind::Block(body, _) => self.check_block_with_expected(&body, expected),
-            ExprKind::Call(callee, args) => self.check_call(expr, &callee, args, expected),


Same here, we can probably make the type of tuple variant constructors be fn(args) -> Enum::Variant instead of fn(args) -> Enum

oli-obk · 2021-10-11T09:51:36Z

compiler/rustc_typeck/src/check/expr.rs

@@ -1685,6 +1764,45 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
        while let Some((base_t, _)) = autoderef.next() {
            debug!("base_t: {:?}", base_t);
            match base_t.kind() {
+                ty::Variant(ty, idx) => match ty.kind() {


I think as a first prototype we could start without this, and require let Foo::Bar { x, y } = ... in order to get the fields out

oli-obk · 2021-10-11T09:53:01Z

src/test/ui/enum-variant-types/invalid_variant.stderr

+   = note: expected enum variant `Foo::Variant1`
+              found enum variant `Foo::Variant2`


oli-obk · 2021-10-11T09:54:03Z

src/test/ui/enum-variant-types/variant_projection.rs

+}
+
+fn main() {
+    let f: Foo::Variant1 = Foo::Variant1(3, "test".to_string());


will this work without f having a type specified?

oli-obk · 2021-10-11T09:55:51Z

src/test/ui/enum-variant-types/fn_call.rs

+}
+
+fn main() {
+    let f: Foo::Variant2 = Foo::Variant2(9);


Will the following also work with the current impl?

let y = Foo::Variant2(9); foo(y); fn foo(z: Foo::Variant2) {}

Not yet, currently everything must have the type explicitly set.

Ah, ok. so that's coupled with the ExpectedTy situation I commented on earlier in this PR.

If you want you can also commit failing tests like this and others you come up with, then we can see the test changes as you touch the logic

jackh726

Oli beat me to the review here, but I'm just as excited :)

Just one comment for now!

jackh726 · 2021-10-11T13:04:32Z

compiler/rustc_middle/src/ty/sty.rs

@@ -195,6 +195,9 @@ pub enum TyKind<'tcx> {
    /// A type variable used during type checking.
    Infer(InferTy),

+    /// An enum (TyKind::Adt) and its variant
+    Variant(Ty<'tcx>, VariantIdx),


So, is the Ty here ever going to be anything other than an Adt? If not, maybe we should just have (&'tcx AdtDef, SubstsRef<'tcx>, VariantIdx)?

I personally think it should some day allow more than just enums. E.g. if we allowed multiple variant indices at some point, we could also think about ranged integers. As long as there are some assertions right now limiting it to adts, that's fine with me, we don't need to check everywhere though. I mean, not all Adts make sense here either, so it's not like limiting to Adts will get us freedom from misuse by construction. E.g. structs don't really make sense inside a ty::Variant right now.

And we should def re-use it for generators, if just for keeping the special cases and repetition down.

Using Adt could cut down a lot of the matches that need to get the underlying AdtDef for the enum this is a variant of.

I wouldn't complicate the code now for extremely hypothetical extensions such as ranged integers.

RalfJung · 2021-10-11T21:12:41Z

compiler/rustc_const_eval/src/interpret/validity.rs

@@ -272,6 +276,20 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
                }
            }

+            ty::Variant(ty, ..) => match ty.kind() {
+                ty::Adt(def, ..) if def.is_enum() => {


This seems to be a very common pattern to get the adtdef of the enum for a ty::Variant; should maybe ty_adt_def have support for ty::Variant so it could be used instead? That would also avoid all the rightwards drift.

RalfJung · 2021-10-11T21:14:03Z

compiler/rustc_const_eval/src/interpret/validity.rs

@@ -567,6 +585,17 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
                self.check_safe_pointer(value, "box")?;
                Ok(true)
            }
+            ty::Variant(ty, _) => match ty.kind() {
+                ty::Adt(def, _) => {
+                    if def.is_box() {


Box is not an enum, this should be impossible. I think this can be handled together with the Adt fallback case below.

RalfJung · 2021-10-11T21:15:13Z

compiler/rustc_const_eval/src/interpret/validity.rs

@@ -729,6 +758,10 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
    ) -> InterpResult<'tcx> {
        let name = match old_op.layout.ty.kind() {
            ty::Adt(adt, _) => PathElem::Variant(adt.variants[variant_id].ident.name),
+            ty::Variant(ty, ..) => match ty.kind() {


Is this reachable? We should only reach this function for types with a multi-variant layout, which ty::Variant is not.

rust-log-analyzer · 2021-10-12T14:08:24Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

    Checking tracing-tree v0.1.9
    Checking rustdoc-json-types v0.1.0 (/checkout/src/rustdoc-json-types)
    Checking tera v1.10.0
    Checking rustdoc v0.0.0 (/checkout/src/librustdoc)
error[E0004]: non-exhaustive patterns: `Variant(_, _)` not covered
    --> src/librustdoc/clean/mod.rs:1398:15
     |
1398 |         match *ty.kind() {
     |               ^^^^^^^^^^ pattern `Variant(_, _)` not covered
    ::: /checkout/compiler/rustc_middle/src/ty/sty.rs:199:5
     |
     |
199  |     Variant(Ty<'tcx>, VariantIdx),
     |
     = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms
     = note: the matched value is of type `rustc_middle::ty::TyKind`


error[E0004]: non-exhaustive patterns: `Variant(_, _)` not covered
    |
    |
539 |         Some(match *self.cx.tcx.type_of(ty_id).kind() {
    |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ pattern `Variant(_, _)` not covered
   ::: /checkout/compiler/rustc_middle/src/ty/sty.rs:199:5
    |
    |
199 |     Variant(Ty<'tcx>, VariantIdx),
    |
    = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms
    = note: the matched value is of type `rustc_middle::ty::TyKind`

RalfJung · 2021-10-12T15:44:17Z

compiler/rustc_const_eval/src/interpret/validity.rs

@@ -756,15 +729,12 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValueVisitor<'mir, 'tcx, M>
        variant_id: VariantIdx,
        new_op: &OpTy<'tcx, M::PointerTag>,
    ) -> InterpResult<'tcx> {
-        let name = match old_op.layout.ty.kind() {
+        let ty = old_op.layout.ty.strip_variant_type();


Variant types don't have variants, so something seems wrong if we see a variant type here.

RalfJung · 2021-10-12T15:46:50Z

compiler/rustc_const_eval/src/interpret/validity.rs

@@ -217,12 +217,8 @@ impl<'rt, 'mir, 'tcx: 'mir, M: Machine<'mir, 'tcx>> ValidityVisitor<'rt, 'mir, '
        match layout.variants {
            Variants::Multiple { tag_field, .. } => {
                if tag_field == field {
-                    return match layout.ty.kind() {
+                    return match layout.ty.strip_variant_type().kind() {


Variant types should never have Variants::Multiple layout so this should be unreachable.

Generally I am not very happy with strip_variant_type, from a conceptual perspective this rarely seems like the right thing to do -- but maybe my intuition is leading me astray here.

strip_variant_type is the correct thing wherever we don't actually handle variant things on generators or adts right now, so here it is indeed wrong, we should not ever encounter adt/generator anymore, but for that Layout::for_variant needs to be fixed to return ty::Variant.

layout is the outer layout of the enum here, why should we not encounter adt/generators here?

It's the other match below in the same function where we should not encounter them any more.

oli-obk · 2021-10-12T16:23:04Z

So... a general point about this PR. It's a huge amount of work to implement this all at once.

In order to get this mergeable (and reviewable) in small steps, how about the following plan:

a new PR that avoids any user visible changes in the first round, and just has the PlaceTy::variant_index changes. This by itself should be fairly self-contained, as you can likely only touch some PlaceTy users (codegen, miri) and avoid going all in in the compiler
a second PR makes Layout::for_variant return ty::Variant and fix up the (presumably larger) fallout that occurs in miri and codegen
add user visible effects behind a feature gate with some simple tests, get that merged
more PRs follow that flesh out the feature with more tests (which are likely obtainable by temporarily deactivating the feature gate and seeing how libcore explodes).

zhamlin · 2021-10-13T22:26:57Z

Thanks for taking the time to lay out the next steps, and looking over this PR.

I'd be more than happy to start on 1, but it does seem like this is skipping a few steps that are outlined here.
I don't want to put much too time into this without making sure this is something needed/wanted.

oli-obk · 2021-10-13T22:39:02Z

1 and 2 are invisible to users. They are essentially cleanups of the compiler's logic.

For 3 we'd at least need an accepted MCP, from the zulip discussion it sounded like there was nothing speaking against it, so maybe just asking for a "second" to be able to experiment is all that is needed at this stage?

zhamlin · 2021-10-13T22:52:45Z

Would zulip be the best place to discuss this going forward?
I have some questions before I can get started on 1

oli-obk · 2021-10-13T22:55:36Z

Yea a zulip thread in the T-compiler stream is probably the best place to discuss this

zhamlin · 2021-10-13T23:03:49Z

I'll start a thread and tag you in it, whats your zulip name?

oli-obk · 2021-10-14T04:18:48Z

@oli

bors · 2021-10-16T09:37:26Z

☔ The latest upstream changes (presumably #89939) made this pull request unmergeable. Please resolve the merge conflicts.

rust-highfive assigned michaelwoerister Oct 10, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 10, 2021

partial enum variant type support

43f9467

zhamlin force-pushed the enum-variant-types branch from a5f1cef to 43f9467 Compare October 10, 2021 19:51

zhamlin mentioned this pull request Oct 10, 2021

Enum Variant Types rust-lang/lang-team#122

Closed

This comment has been minimized.

Sign in to view

rust-highfive assigned oli-obk and unassigned michaelwoerister Oct 11, 2021

oli-obk requested changes Oct 11, 2021

View reviewed changes

jackh726 reviewed Oct 11, 2021

View reviewed changes

oli-obk mentioned this pull request Oct 11, 2021

change PR to "waiting on author" after "changes requested" review rust-lang/triagebot#1373

Closed

RalfJung reviewed Oct 11, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

clean up handling of variant type

ccf569f

zhamlin force-pushed the enum-variant-types branch from 14a70a9 to ccf569f Compare October 12, 2021 14:03

RalfJung reviewed Oct 12, 2021

View reviewed changes

zhamlin mentioned this pull request Oct 26, 2021

Update PlaceTy to use ty::Variant #90330

Closed

zhamlin closed this Nov 19, 2021

		@@ -744,6 +744,22 @@ impl<'a, 'b, 'tcx> TypeVerifier<'a, 'b, 'tcx> {
		PlaceTy { ty: base_ty, variant_index: Some(index) }

		= note: expected enum variant `Foo::Variant1`
		found enum variant `Foo::Variant2`

enum variant support #89745

enum variant support #89745

Conversation

zhamlin commented Oct 10, 2021

rust-highfive commented Oct 10, 2021

rust-highfive commented Oct 10, 2021

This comment has been minimized.

oli-obk commented Oct 11, 2021

oli-obk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackh726 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

rust-log-analyzer commented Oct 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oli-obk commented Oct 12, 2021

zhamlin commented Oct 13, 2021

oli-obk commented Oct 13, 2021

zhamlin commented Oct 13, 2021

oli-obk commented Oct 13, 2021

zhamlin commented Oct 13, 2021

oli-obk commented Oct 14, 2021

bors commented Oct 16, 2021