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

test more variants of enum-int-casting #61702

Merged
merged 1 commit into from
Jun 18, 2019
Merged

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Jun 9, 2019

As I learned in #61673 (comment), there is a code path we are not testing yet. Looks like enum-int-casting with and without an intermediate let-binding is totally different.

EDIT: The reason for this is to get rid of the cycle in definitions such as:

enum Foo {
    A = 0,
    B = Foo::A as isize + 2,
}

This has historically been supported, so a hack adding special treatment to Enum::Variant as _ was added to keep supporting it.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(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 Jun 9, 2019
@RalfJung
Copy link
Member Author

RalfJung commented Jun 9, 2019

I looked at the MIR for something like Ordering::Less as i32 and... WTF:

fn main() {
    let _val = std::cmp::Ordering::Less as i32;
}

has the following MIR:

fn  main() -> () {
    let mut _0: ();                      // return place
    scope 1 {
    }
    scope 2 {
        let _1: i32;                     // "_val" in scope 2 at src/main.rs:14:9: 14:13
    }
    let mut _2: isize;
    let mut _3: (isize, bool);

    bb0: {
        StorageLive(_1);                 // bb0[0]: scope 0 at src/main.rs:14:9: 14:13
        StorageLive(_2);                 // bb0[1]: scope 0 at src/main.rs:14:16: 14:47
        _3 = CheckedAdd(const Unevaluated(DefId(2/1:7942 ~ core[e194]::cmp[0]::Ordering[0]::Less[0]::{{constant}}[0]), []) : isize, const 0isize); // bb0[2]: scope 0 at src/main.rs:14:16: 14:47
                                         // ty::Const
                                         // + ty: isize
                                         // + val: Unevaluated(DefId(2/1:7942 ~ core[e194]::cmp[0]::Ordering[0]::Less[0]::{{constant}}[0]), [])
                                         // mir::Constant
                                         // + span: src/main.rs:14:16: 14:47
                                         // + ty: isize
                                         // + literal: Const { ty: isize, val: Unevaluated(DefId(2/1:7942 ~ core[e194]::cmp[0]::Ordering[0]::Less[0]::{{constant}}[0]), []) }
                                         // ty::Const
                                         // + ty: isize
                                         // + val: Scalar(Bits { size: 8, bits: 0 })
                                         // mir::Constant
                                         // + span: src/main.rs:14:16: 14:47
                                         // + ty: isize
                                         // + literal: Const { ty: isize, val: Scalar(Bits { size: 8, bits: 0 }) }
        assert(!move (_3.1: bool), "attempt to add with overflow") -> bb1; // bb0[3]: scope 0 at src/main.rs:14:16: 14:47
    }

    bb1: {
        _2 = move (_3.0: isize);         // bb1[0]: scope 0 at src/main.rs:14:16: 14:47
        _1 = move _2 as i32 (Misc);      // bb1[1]: scope 0 at src/main.rs:14:16: 14:47
        StorageDead(_2);                 // bb1[2]: scope 0 at src/main.rs:14:46: 14:47
        StorageDead(_1);                 // bb1[3]: scope 0 at src/main.rs:15:1: 15:2
        return;                          // bb1[4]: scope 0 at src/main.rs:15:2: 15:2
    }
}

What is even going on? Why is there an addition? Of 0? Checked for overflow?!? I find it unlikely that adding 0 will make anything overflow.
And why does core[e194]::cmp[0]::Ordering[0]::Less[0]::{{constant}}[0] have type isize? There's not even an enum-int cast here, just a cast from isize to i32.

For comparison, the MIR with an intermediate let-binding:

fn main() {
    let val = std::cmp::Ordering::Less;
    let _val = val as i32;
}
fn  main() -> () {
    let mut _0: ();                      // return place
    scope 1 {
        scope 3 {
        }
        scope 4 {
            let _2: i32;                 // "_val" in scope 4 at src/main.rs:3:9: 3:13
        }
    }
    scope 2 {
        let _1: std::cmp::Ordering;      // "val" in scope 2 at src/main.rs:2:9: 2:12
    }
    let mut _3: std::cmp::Ordering;

    bb0: {
        StorageLive(_1);                 // bb0[0]: scope 0 at src/main.rs:2:9: 2:12
        discriminant(_1) = 0;            // bb0[1]: scope 0 at src/main.rs:2:15: 2:39
        StorageLive(_2);                 // bb0[2]: scope 1 at src/main.rs:3:9: 3:13
        StorageLive(_3);                 // bb0[3]: scope 1 at src/main.rs:3:16: 3:19
        _3 = _1;                         // bb0[4]: scope 1 at src/main.rs:3:16: 3:19
        _2 = move _3 as i32 (Misc);      // bb0[5]: scope 1 at src/main.rs:3:16: 3:26
        StorageDead(_3);                 // bb0[6]: scope 1 at src/main.rs:3:25: 3:26
        StorageDead(_2);                 // bb0[7]: scope 1 at src/main.rs:4:1: 4:2
        StorageDead(_1);                 // bb0[8]: scope 0 at src/main.rs:4:1: 4:2
        return;                          // bb0[9]: scope 0 at src/main.rs:4:2: 4:2
    }
}

That's what I expected to begin with.

Cc @eddyb

(This is unrelated to the PR by the way, I am just not sure what would be a better place to discuss this.)

@eddyb
Copy link
Member

eddyb commented Jun 10, 2019

@RalfJung Heh, @oli-obk knows why because IIRC he did all of this.

Let's take it one by one: isize is the default type of the "explicit discriminant" expressions (i.e. after = ...) for enum variants, unless a #[repr] attribute is specified.
The addition with 0 is probably a missing special-case (or, really, we could be doing const-folding).

Now, why is Enum::Variant as IntegerType different from casting a runtime value of type Enum?
Well, that's because Enum's variant discriminants can use this sort of cast to get the discriminants of other expressions, but you can't compute the value the runtime way without miri knowing the layout of Enum (which depends on the values of the discriminants).

So, that weird MIR you see, doesn't depend on Ordering being an i8, and so could be used as part of the definition of Ordering itself. I guess we could be const-folding that on the spot, if miri allows it?
I'm not entirely sure why we leave an Unevaluated in, but if we change anything, we'd need to crater it.

@RalfJung
Copy link
Member Author

RalfJung commented Jun 10, 2019

Let's take it one by one: isize is the default type of the "explicit discriminant" expressions (i.e. after = ...) for enum variants, unless a #[repr] attribute is specified.

So core[e194]::cmp[0]::Ordering[0]::Less[0]::{{constant}}[0] is not supposed to be Less, it is supposed to be "the discriminant expression for Less"? That makes a bit more sense then.

The addition with 0 is probably a missing special-case (or, really, we could be doing const-folding).

I was more wondering where it comes from, but now I guess this is from the "counting up" that happens where the next variant gets N+1 if it does not have an explicit discriminator expression?

Now, why is Enum::Variant as IntegerType different from casting a runtime value of type Enum?
Well, that's because Enum's variant discriminants can use this sort of cast to get the discriminants of other expressions, but you can't compute the value the runtime way without miri knowing the layout of Enum (which depends on the values of the discriminants).

I don't understand. Who can get which discriminants of which other expressions but not at run-time...?

Why is it better to refer to the discriminant expression here instead of doing something like what happens with an intermediate let-binding? The code generated with the let-binding is more readable and it needs fewer basic blocks. Having a special case for "immediate cast to int" makes testing harder. This almost caused an incorrect assertion to sneak into the miri engine that neither the rustc CTFE test suite nor the Miri test suite found, because nobody writes these tests in let-expanded way and nobody expects that to hit a totally different code path. Is there any argument for this funny special case translation?

I could understand if some sort of const propagation would kick in later and replace statically-known casts with constant stuff, but that is not what is happening here. rustc.main.001-000.SimplifyCfg-initial.before.mir already has this addition thing going on. Looks to me like an attempt to do some optimizations during MIR construction already? IMO that's generally not a good idea.

@eddyb
Copy link
Member

eddyb commented Jun 10, 2019

enum Foo {
    A = 0,
    B = Foo::A as isize + 2,
}

This will cycle error if you do the runtime thing, because the runtime thing requires knowing the layout, and knowing the layout requires knowing the discriminants.

@RalfJung
Copy link
Member Author

RalfJung commented Jun 10, 2019

But we still cycle error for this:

enum Foo {
    A = 0,
    B = {let v = Foo::A; v as isize + 2},
}

or this:

const fn do_something(x: Foo) -> isize { ... }

enum Foo {
    A = 0,
    B = do_something(Foo::A),
}

Seems like an awful hack for a partial solution... and even supporting this very special case seems way not worth the cost in incurs. This affects all MIR generated anywhere!

@eddyb
Copy link
Member

eddyb commented Jun 10, 2019

@RalfJung Backwards compatibility is certainly worth it, and there was no let at the time miri replaced the old const-evaluator. Maybe a different solution can be picked, but like I said, it needs to pass crater.

@RalfJung
Copy link
Member Author

Well I'd be in favor of deprecating cyclic enums...

But in the interrim, can we do something like only apply this weird special case when translating an enum discriminant expression?

@RalfJung
Copy link
Member Author

Can you point me to the code implementing this? I did a grep for "enum" and "cast" in librustc_mir/build but found nothing.

@eddyb
Copy link
Member

eddyb commented Jun 10, 2019

// check whether this is casting an enum variant discriminant
// to prevent cycles, we refer to the discriminant initializer
// which is always an integer and thus doesn't need to know the
// enum's layout (or its tag type) to compute it during const eval
// Example:
// enum Foo {
// A,
// B = A as isize + 4,
// }
// The correct solution would be to add symbolic computations to miri,
// so we wouldn't have to compute and store the actual value
let var = if let hir::ExprKind::Path(ref qpath) = source.node {
let res = cx.tables().qpath_res(qpath, source.hir_id);
cx
.tables()
.node_type(source.hir_id)
.ty_adt_def()
.and_then(|adt_def| {
match res {
Res::Def(
DefKind::Ctor(CtorOf::Variant, CtorKind::Const),
variant_ctor_id,
) => {
let idx = adt_def.variant_index_with_ctor_id(variant_ctor_id);
let (d, o) = adt_def.discriminant_def_for_variant(idx);
use rustc::ty::util::IntTypeExt;
let ty = adt_def.repr.discr_type();
let ty = ty.to_ty(cx.tcx());
Some((d, o, ty))
}
_ => None,
}
})
} else {
None
};
let source = if let Some((did, offset, var_ty)) = var {
let mk_const = |literal| Expr {
temp_lifetime,
ty: var_ty,
span: expr.span,
kind: ExprKind::Literal {
literal,
user_ty: None
},
}.to_ref();
let offset = mk_const(ty::Const::from_bits(
cx.tcx,
offset as u128,
cx.param_env.and(var_ty),
));
match did {
Some(did) => {
// in case we are offsetting from a computed discriminant
// and not the beginning of discriminants (which is always `0`)
let substs = InternalSubsts::identity_for_item(cx.tcx(), did);
let lhs = mk_const(cx.tcx().mk_const(ty::Const {
val: ConstValue::Unevaluated(did, substs),
ty: var_ty,
}));
let bin = ExprKind::Binary {
op: BinOp::Add,
lhs,
rhs: offset,
};
Expr {
temp_lifetime,
ty: var_ty,
span: expr.span,
kind: bin,
}.to_ref()
},
None => offset,
}

@RalfJung
Copy link
Member Author

Okay, let's see in #61723 what crater finds, and let's move the discussion there.

Until then, this PR helps to at least get us some basic test coverage for "both kinds" of enum-int-casts.

@RalfJung
Copy link
Member Author

r? @oli-obk

@oli-obk
Copy link
Contributor

oli-obk commented Jun 17, 2019

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 17, 2019

📌 Commit 2e9931f has been approved by oli-obk

@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 Jun 17, 2019
Centril added a commit to Centril/rust that referenced this pull request Jun 17, 2019
test more variants of enum-int-casting

As I learned in rust-lang#61673 (comment), there is a code path we are not testing yet. Looks like enum-int-casting with and without an intermediate let-binding is totally different.

EDIT: The reason for this is to get rid of the cycle in definitions such as:
```rust
enum Foo {
    A = 0,
    B = Foo::A as isize + 2,
}
```
This has historically been supported, so a hack adding special treatment to `Enum::Variant as _` was added to keep supporting it.
bors added a commit that referenced this pull request Jun 17, 2019
Rollup of 5 pull requests

Successful merges:

 - #61702 (test more variants of enum-int-casting)
 - #61836 (Replace some uses of NodeId with HirId)
 - #61885 (Help LLVM better optimize slice::Iter(Mut)::len)
 - #61893 (make `Weak::ptr_eq`s into methods)
 - #61908 (don't ICE on large files)

Failed merges:

r? @ghost
@bors bors merged commit 2e9931f into rust-lang:master Jun 18, 2019
@RalfJung RalfJung deleted the const-enum-cast branch June 21, 2019 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants