Rust: Tracking issue for RFC 2203, "Constants in array repeat expressions"

[Centril]

This is a tracking issue for the RFC "Constants in array repeat expressions" (rust-lang/rfcs#2203) (feature gate: const_in_array_repeat_expressions).

Steps:

• [x] Implement the RFC (cc @rust-lang/compiler -- can anyone write up mentoring instructions?)
• [ ] Adjust documentation (see instructions on forge)
• [ ] Stabilization PR (see instructions on forge)

Implementation history:

• Initial implementation: https://github.com/rust-lang/rust/pull/61749
• (this is incomplete)

Unresolved questions:

• Should we treat this as an explicit promotion context? That would allow calling arbitrary const fn, but poses the risk of breaking code that would otherwise work fine if we decide to promote when it would not have been necessary. The alternative is to rely on inline const expressions instead (https://github.com/rust-lang/rust/issues/76001).

[F001]

I had an investigation on this issue. I think removing below lines can achieve our goals. But I'm not sure whether it is enough.

https://github.com/rust-lang/rust/blob/1eab9c5a1b85db4ca850114814a7df3e289fcd67/src/librustc_mir/borrow_check/nll/type_check/mod.rs#L1232-L1238

https://github.com/rust-lang/rust/blob/1eab9c5a1b85db4ca850114814a7df3e289fcd67/src/librustc_typeck/check/mod.rs#L4017-L4025

And another question, given that this change should be guarded by a feature gate, how can we suggest user to enable this feature when a compile error occurs?

I'd appreciate it if somebody can write a mentoring instruction.

[eddyb]

See https://github.com/rust-lang/rfcs/pull/2203#issuecomment-360828613 - I don't think we should implement this before the MIR borrowck becomes the default - while it may be possible, it's getting increasingly risky to do such things.
cc @nikomatsakis

[eddyb]

So I just realized that "constant" has been a red herring all along for this problem:
What const-checking has is "value-based reasoning", that is, a value like None::<T> contains no values of T, so T's properties (like Drop or the presence of interior mutability) do not apply to it.

For [expr; n] where we don't know that n <= 1, we've always required typeof(expr): Copy.
The accepted RFC would allow another option which is SomeFormOfConstant(expr).
However, this isn't as general as it could be and it doesn't tie into the Copy condition very well.

One example of a runtime repeated expression we could allow is [Ok::<i32, String>(rand()); n].
Or to expand it a bit, { let x = rand(); [Ok::<i32, String>(x); n] } - this is "obviously fine" because the user could write n of Ok(x), so inline ADT construction is always "inherently copyable".

I propose (a bit late) that we consider a value (dataflow) analysis, Copyable(expr), which works similar to the current value analyses used in const-checking, effectively "recursing" on ADT constructors, and using the following two rules when reaching leaves x of unknown values:

• typeof(x): Copy (the set of types currently allowed in [x; n])
• x is already a constant (by naming a constant or through the pre-existing promotion)

Another advantage of this is that it's not limited to [expr; n]: any copy could be satisfied through it!

cc @nikomatsakis @RalfJung

[RalfJung]

So I just realized that "constant" has been a red herring all along for this problem:
What const-checking has is "value-based reasoning", that is, a value like None:: contains no values of T, so T's properties (like Drop or the presence of interior mutability) do not apply to it.

Trying to recast what you are saying in my terms, we have Copy and "needs drop" as properties of types, but in fact these are properties of values. T: Copy merely says that all values of type T are copy. And a value is Copy if it can be duplicated because it doesn't have any ownership in it. Most of the time we only care about the type-level Copy because that's all the compiler knows, but e.g. the None value of type Option<T> is Copy for any T. Is that what you are saying?

I think that's a remark I already made somewhere else earlier this year but I have no idea where.^^

[eddyb]

Most of the time we only care about the type-level Copy because that's all the compiler knows, but e.g. the None value of type Option<T> is Copy for any T. Is that what you are saying?

Yes, except I only realized it can apply to all copies at the very end of writing my entire comment, so my reasoning may look quite roundabout. I suspect this might not even be the only place where we can generalize a type-based analysis to a value-based one (like const-checking has).

We could even mix this with polymorphism, by putting "refinement types" through generics.
(but that's riskier because of soundness implications that are harder to understand right now)

[nikomatsakis]

@eddyb hmm, interesting thought. So this is basically "doubling down" on the value-based reasoning we use for consts, and applying it here as well? I don't entirely hate that, particularly if we can kind of "reuse" the form of the rule from consts exactly.

[runelabs]

I ran into the following constraints with trying to use _primitive array repeat_ in struct initialization. It seems there is no way to make a RHS expression with the default value None of an Option for [x; n] without implementing Copy or needing _type ascription_.
I read the previous open issues on _type ascription_, but with default() for Option it would be nice to just be able to easily use the default None value for repetition. I am using this for a no_std crate.

enum Wrap { ... }
struct MyStruct<'a> {
  arr: &'a mut [Option<Wrap>]
}
impl<'a> MyStruct<'a> {
  pub fn new(list: &'a mut [Option<Wrap>]) -> Self { Self{ arr: list} } }
}
// works fine, and is very ergonomic
let mut z = MyStruct{ arr: &mut [] }; 
// however the logical follow-up and equally ergonomic &mut [None;N] fails...

// all fine, Copy-trait not needed (*1)
let mut empty1: [Option<Wrap>;11] = Default::default();
let mut a = MyStruct{ arr: &mut empty1 };

// need type ascription, Copy-trait not needed (*2)
let mut empty2 = Default::default():[Option<Wrap>;11];
let mut b = MyStruct::new(&mut empty2);

// need type ascription, Copy-trait for Wrap
let mut empty3 = [Default::default();11]:[Option<Wrap>;11]; // (*3)
let mut empty4 = [Option::default();11]:[Option<Wrap>;11]; // (*4)
let mut empty5 = [None;11]:[Option<Wrap>;11]; // (*5)

// need type ascription, parenthesized expression ... but alas, lifetime fails when (expr)
let mut c = MyStruct{ arr: &mut ([Default::default();11]:[Option<Wrap>;11]) }; // (*6) , also parens expr
let mut d = MyStruct::new( &mut ([Default::default();11]:[Option<Wrap>;11]) ); // (*6b) , also parens expr

// need Copy-trait for Wrap
let mut e = MyStruct{ arr: &mut [Default::default();11] }; // (*7)
let mut f = MyStruct{ arr: &mut [None;11] }; // (*8)
let mut g = MyStruct::new(&mut [None;11]); // (*9)

All the cases from (1) to (9) try to accomplish similar initialization. Ideally, shouldn't they all work similarly?

I really want to use (9), (8) or (7) - and not (1) or (*2) which are not ergonomic.

None seems like a special repetition case not needing Copy. Similarly it is very awkward having arr: [Option<Wrap>;11] and not being able to use [None;11] for initializing it in a structure, but actually having to expand it all like [None,None,None,...]. It also does not feel natural to need _type ascription_ for such basic initialization. Needing to declare const X: T = None; for a [X; N] also does not help in making Rust a more accessible programming language for newcomers. It requires reading up on this special case for Option repetition - instead of what should really just be a simple programming pattern similar to MyStruct{ arr: &mut [] } which is valid, immediately accessible and ergonomic.

[eddyb]

@nikomatsakis Indeed, I think we should double down on value reasoning in favor of ergonomics.
In my view, NLL is doing sort of a similar thing, albeit the analysis is relatively more complex.

---

Needing to declare const X: T = None; for a [X; N] also does not help in making Rust a more accessible programming language for newcomers

This is not required in the accepted RFC, you will be able to just write [None; N] inline.

It requires reading up on this special case for Option repetition -

Option is not special-cased, it just happens that None contains no non-copy data. None would be just as accepted as e.g. Ok(123) (for any Result<_, _> type, including non-copy ones).

As for your ascription questions: you can make these replacements in your code:

• Wrap and even Option<Wrap> -> _ (inference)
  
  
  
  • always try _ first when trying to specify a type (e.g. Vec<_> before Vec<T>)
  
  
  • in let x: _; and expr: _, the _ is redundant (inference works regardless)
  
  
• [expr; n]: [_; n] -> [expr; n] (the type ascription is also redundant here)
• Default::default(): T -> T::default() (or <T>::default if T isn't a path)
  
  
  
  • e.g. <[_; n]>::default()
  
  

Then your code looks like this (click to expand).

enum Wrap { ... }
struct MyStruct<'a> {
  arr: &'a mut [Option<Wrap>]
}
impl<'a> MyStruct<'a> {
  pub fn new(list: &'a mut [Option<Wrap>]) -> Self { Self{ arr: list} } }
}
// works fine, and is very ergonomic
let mut z = MyStruct{ arr: &mut [] }; 
// however the logical follow-up and equally ergonomic &mut [None;N] fails...

// all fine, Copy-trait not needed (*1)
let mut empty1: [_;11] = Default::default();
let mut a = MyStruct{ arr: &mut empty1 };

// need type ascription, Copy-trait not needed (*2)
let mut empty2 = <[_;11]>::default();
let mut b = MyStruct::new(&mut empty2);

// need type ascription, Copy-trait for Wrap
let mut empty3 = [Default::default();11]; // (*3)
let mut empty4 = [Option::default();11]; // (*4)
let mut empty5 = [None;11]; // (*5)

// need type ascription, parenthesized expression ... but alas, lifetime fails when (expr)
let mut c = MyStruct{ arr: &mut [Default::default();11] }; // (*6) , also parens expr
let mut d = MyStruct::new( &mut [Default::default();11] ); // (*6b) , also parens expr

// need Copy-trait for Wrap
let mut e = MyStruct{ arr: &mut [Default::default();11] }; // (*7)
let mut f = MyStruct{ arr: &mut [None;11] }; // (*8)
let mut g = MyStruct::new(&mut [None;11]); // (*9)

Further notes from looking at the result of that:

• [Default::default(); 11] and [Option::default(); 11] are no better than [None; 11]
• <[_; 11]>::default() works even today because it calls Option::default 11 times from the implementation of Default for [T; 11], so it never needs to copy an Option<Wrap>

@runelabs I hope I've clarified a few things and/or resolved most of your concerns.

[runelabs]

@eddyb Thanks, I hadn't thought of <[_; 11]>::default() . That is the _"turbofish"_ notation?
It wasn't immediately intuitive to me as a replacement. The compiler suggested type inference when I was exploring options earlier, which led to looking at _type ascription_ although it seemed unnecessary. As long as the [None;11] will be working with this RFC, I am a happy camper. From the RFC it seemed like it had to be a const declaration beforehand. Good it wasn't so :smile:

[eddyb]

<T>::foo is "type-qualified path" syntax, which T::foo is a shorthand for (if T is not a trait).
Usually only foo::<T> is referred to as turbofish, the former would be more "reverse turbofish".

[eddyb]

Is the S-blocked label the right way to do this? I kind of want a "needs NLL" but not "fixed by NLL" label (cc @nikomatsakis).

[pnkfelix]

@eddyb hmm maybe we could/should just rename "fixed by NLL" to "needs NLL", which is ... strictly more general?

(Or is that label not a true generalization? I'm still musing over the english semantics here. I guess "fixed by NLL" implies that NLL is sufficient but not (necessarily) necessary, while "needs NLL" implies NLL is necessary but not (necessarily) sufficient...)

• I can't believe just wrote "necessarily necessary"

[Centril]

Discussed briefly on Zulip -- would we want to experiment with this on 2018 only to begin with?

[eddyb]

I left a comment elsewhere, I think it would be fine if this were restricted to Rust 2018: https://github.com/rust-lang/rust/issues/54542#issuecomment-478261027

[Centril]

cc @pnkfelix, @eddyb, @oli-obk, and possibly more people: Should we call this unblocked now and start on an implementation? I think it would be good to avoid too much pressure to stabilize uninitialized_array! and do the principled fix (this feature). :)

[eddyb]

Ignoring the more powerful idea in https://github.com/rust-lang/rust/issues/49147#issuecomment-392256983, these are some mentoring notes:
EDIT: some of the details have been updated based on @davidtwco's feedback.

1. remove this check:
   https://github.com/rust-lang/rust/blob/c57ed9d9478dcd12c854a0ef4e83c7f384ade060/src/librustc_typeck/check/mod.rs#L4429-L4435
   (you can probably also remove ObligationCauseCode's RepeatVec variant)
   Note that this won't allow any new code, as NLL borrowck has its own check that would ICE (see 2.)
2. replace this with an error only wrapped in a if let Operand::Move(_) = operand check:
   https://github.com/rust-lang/rust/blob/c57ed9d9478dcd12c854a0ef4e83c7f384ade060/src/librustc_mir/borrow_check/nll/type_check/mod.rs#L1950-L1961
   Note that Operand::Copy is automatically picked by MIR construction when the element type is Copy, and Operand::Constant is effectively copyable anyway, if it can even appear in this position - just to be sure, you can add this test (which currently errors):

// Doesn't implement `Copy`.
struct Foo;
const FOO: Foo = Foo;
fn main() {
    let _five_foos = [FOO; 5];
}

The error should probably look a bit like this:
https://github.com/rust-lang/rust/blob/da9ebc828c982d2ed49396886da85011e1b0a6c0/src/librustc/traits/error_reporting.rs#L1564-L1567
(maybe this is a downgrade because it wouldn't be showing you why it doesn't implement Copy?)
An example of an unconditional error emitted from NLL type-checking:
https://github.com/rust-lang/rust/blob/c57ed9d9478dcd12c854a0ef4e83c7f384ade060/src/librustc_mir/borrow_check/nll/type_check/mod.rs#L1876-L1890
After this step, any errors that became ICEs in step 1 should be back to proper hard errors (on all editions).

1. promote Rvalue::Repeat operands, just like #[rustc_args_required_const] arguments.
   
   
   
   • add a Repeat(Location) variant (or generalize Ref to Rvalue) to enum Candidate:
     
     https://github.com/rust-lang/rust/blob/c57ed9d9478dcd12c854a0ef4e83c7f384ade060/src/librustc_mir/transform/promote_consts.rs#L63-L68
   
   
   • handle Rvalue::Repeat here:
     
     https://github.com/rust-lang/rust/blob/c57ed9d9478dcd12c854a0ef4e83c7f384ade060/src/librustc_mir/transform/qualify_consts.rs#L699
     
     like so:
   
   

ValueSource::Rvalue(&Rvalue::Repeat(operand, _)) => {
    let candidate = Candidate::Repeat(location);
    let not_promotable =
        IsNotPromotable::in_operand(self, operand) ||
        IsNotImplicitlyPromotable::in_operand(self, operand);
    if !not_promotable {
        debug!("qualify_consts: promotion candidate: {:?}",  candidate);
        self.promotion_candidates.push(candidate);
    }
}

• finally, handle the actual promotion of Candidate::Repeat by getting the statement like Candidate::Ref, but replacing the Operand like Candidate::Argument:
  https://github.com/rust-lang/rust/blob/c57ed9d9478dcd12c854a0ef4e83c7f384ade060/src/librustc_mir/transform/promote_consts.rs#L305

[eddyb]

I want to treat Rvalue::Repeat(operand, n) where n > 1 as consuming operand more than once, which would remove the need for the explicit check, but that would degrade error messages, unless we do something similar to loops where we say "moved out on the previous loop iteration".

[davidtwco]

I'll give implementing this a go.

[bstrie]

Another advantage of this is that it's not limited to [expr; n]: any copy could be satisfied through it!

Could this be used to at last return the @ pattern to glory by making things like x @ (y @ (1, 2), 3) possible again? IIRC, advanced uses of @ like this were disabled for 1.0 due to uncertainty regarding copyability.

[eddyb]

@bstrie Irrelevant, IMO. @ was intentionally crippled because of the old borrowck.
With the MIR/NLL borrowck we should be able to just allow it, and any kind of move/borrow conflict between the two sides of @ should be correctly detected.

[RalfJung]

@oli-obk @eddyb I just realized that this feature has one "interesting" consequence... the code in an array repeat expression might now be executed at compile-time, using CTFE rules and restrictions, without an explicit marker or the user explicitly asking for this to happen. We are having lots of pain with promotion because it imposes CTFE rules on code where the user did not ask for CTFE rules, won't we have all the same pain here?

[RalfJung]

Ah, turns out this actually uses the promotion infrastructure, so we the concern that we "silently" execute code under CTFE is taken care of.

Of course, the problem with this is that this is using the promotion infrastructure, which we are trying hard to contain because it's a hazard...

[leonardo-m]

Interesting differences:

#![allow(dead_code)]
fn main() {
    //const ARR1: [Option<Box<u32>>; 100] = [None; 100]; // Error
    const ARR2: [Option<u32>; 100]      = [None; 100];   // OK
    const X: Option<Box<u32>>           = None;
    const ARR3: [Option<Box<u32>>; 100] = [X; 100];      // OK
}

[abonander]

@RalfJung do we _need_ the same promotion logic? It seems to me that we can have two mutually exclusive situations:

• The element type is Copy, thus the expression does not need to be const but is only executed once
• The element type is not Copy thus the expression _must_ be const and thus cannot be observed executing more than once anyway

It seems to me that there should be no surprises here as long as we clearly communicate that the expression should only _appear_ to be executed once, either by the result being copied _or_ the expression not having observable side-effects due to it being const. This communication can mostly take place through compiler errors, I think.

[eddyb]

@abonander Ah, yeah, I forgot that part. So if the expression is Copy do not attempt promotion, meaning only code which needs promotion to compile will evaluate the expression at compile-time, which even if it errors in miri, it couldn't have compiled any other way.

This is in contrast with &expr promotion, which should never be attempted unless we're certain it can't fail, because the code will likely compile without promotion.

[RalfJung]

I am not fully convinced by that argument. It is still very non-obvious to the user if the code needs promotion. In particular, in generic code it could be that if the involved types were fully known, no promotion would be needed, but due to the generic nature of this code promotion happened anyway.

I think we need a clear syntactic marker for promotion, including in array length initializers. Something like const { expr }, or something else specialized for array initializers, but something.

But I agree that at least the part where "promotion broke code that could have compiled otherwise" does not apply here. Is that on its own enough to justify full const-context treatment (same rules as in const items)?

[oli-obk]

Even in generics everything is obvious. Either you have a Copy bound, or you get promotion. I think not being a full const context will confuse more than it will not. We'll just get requests to add more things or at least issues opened asking why things that are obv OK don't work

[abonander]

I think as long as we somehow clearly communicate that the idea is to not have side-effects that are dependent on the length of the array, I think the intuition that the initializer either needs Copy or needs to be a constexpr falls out of that. We can do this easily in error messages:

error[E###]: array initializers may not have length-dependent side-effects
 --> src/main.rs:2:23
  |
2 |     let mut strings = [String::with_capacity(32); 16];
  |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `String` is not `Copy` nor is `String::with_capacity(32)` a const-expression
  |
  = note: the element type must either be copyable or the initializer expression must be const so as to not have length-dependent side-effects

(In this specific case we could recommend using String::new() and a loop calling .reserve(32) instead, although that's a lot of information for a compiler error.)

[abonander]

And I'm pushing this point because I'm _assuming_ that's the reason array repeat initializers cannot use arbitrary expressions because that's what it looks like to me. The reference doesn't specify the motivation.

Does it also/instead have to do with not being able to report panics that occurred in the middle of initialization? That is, if any arbitrary expression was allowed and was run for every element at the array, e.g. if [may_panic(); 8] panics at element 6, we have no way to clearly report that. I imagine it might also have to do with needing to drop only the initialized elements and that being a decently complex operation with nontrivial codegen.

[RalfJung]

@abonander the problem is not code that syntactically wouldn't be allowed in CTFE. The problem is code that goes wrong when doing CTFE -- either because it caused UB, or (more interestingly) because it is "unconst", meaning it would be safe at run-time but is not safe at compile-time.

We will, of course, have a proper diagnostic telling the user what went wrong when const-evaluating their code. And I think given that the alternative is to not even try and compile this, that seems like a reasonable choice. We should just make sure that the error specifically for array initializers specifically mentions that this all only happened because the type is not Copy.

[RalfJung]

Do we have a good name for the code that we run at const-time without the user asking for it? I used to talk about that as "promotion", but I think @eddyb calls other things promoted. I am talking specifically about the code that obeys #[rustc_promotable], the code where if CTFE fails we screwed up because we should have just let that code run at run-time.

"implicitly promoted", maybe? Crucially, the following is a similar mechanism but it not that same kind of code:

const BAR: &'static i32 = &5;
const FOO: Option<&'static i32> = Some(&5);

Relatedly, @eddyb taught me about a mechanism that makes things have 'static lifetime, and that it is the limitations of said mechanism that make this not work:

const BARC: &'static Cell<i32> = &Cell::new(5);
const FOOC: Option<&'static Cell<i32>> = Some(&Cell::new(5));

How is that mechanism called?

[eddyb]

@RalfJung &expr is affected by "implicit promotion (to 'static)" but const X: &T = &expr; (and some amount of nesting) follows the same "temporary scope" rules let x: &T = &expr; does (but for const, the "surrounding scope" is 'static).

What works for const X: &T = &expr; and doesn't when &expr is sufficiently nested in the RHS of the const (e.g. const X: &T = { let tmp = &expr; tmp };) is "needs drop" types.

That is, const EMPTY_BYTES: &Vec<u8> = &Vec::new(); works even if &Vec::new() in general won't be promoted: the difference in "temporary scope" means there was never any intention to drop the Vec, so there's nothing to promote or error on.

But const FOO: &Vec<u8> = id(&Vec::new()); doesn't work because id could be the identity function, but could just as well only read from its argument and then return something unrelated - just like a runtime function could do.

This distinction lets us allow some leaking-to-'static in the language itself (some of that was needed for backcompat, and the rest will be useful as CTFE gets more powerful) while keeping the runtime semantics for the general case (but this is kinda offtopic already).

[RalfJung]

@eddyb points out that the current implementation does not work in static/const bodies. This fails:

#![feature(const_in_array_repeat_expressions)]

struct NotCopy;

static FOO: i32 = {
    let x: [Option<NotCopy>; 5] = [None; 5];
    5
};

Is that worth an issue?

[scottmcm]

I'm concerned about semver implications here. It seems like it would be bad if adding Copy to the type returned from foo could change what happens in things like [foo(); 4].

[cramertj]

@scottmcm FWIW adding Copy to a type can already change significant behaviors and can actually break compilation because of how move inference for closures works.

[josephlr]

As @RalfJung mentioned in https://github.com/rust-lang/rust/issues/77138#issuecomment-698159728, the following code currently doesn't work with this feature flag:

#![feature(const_in_array_repeat_expressions)]
struct T;
impl T {
    const fn new() -> Self {
        Self
    }
}

const fn bar() -> [T; 2] {
    [T::new(); 2]
}

This is due to changes make around promotion rules in const fn (to make them more like fn). To fix this, there will need to be explicit constant promotion rules for array initializers.

[oli-obk]

To fix this, there will need to be explicit constant promotion rules for array initializers.

Note that there's mostly aggreement (I think?) on the decision to not use explicit const promotion, but to just require const { T::new() } blocks

[josephlr]

To fix this, there will need to be explicit constant promotion rules for array initializers.

Note that there's mostly aggreement (I think?) on the decision to _not_ use explicit const promotion, but to just require const { T::new() } blocks

So this means the implementation becomes:

const fn bar() -> [T; 2] {
    const C: T = T::new();
    [C; 2]
}

Is that the desired way to write this? This was my bad missing https://github.com/rust-osdev/x86_64/pull/175 which explains this issue super well.

[RalfJung]

The plan is that eventually, you can write [const { T::new() }; 2]. This is tracked at https://github.com/rust-lang/rust/issues/76001.

Until that is implemented, what you wrote is basically the manual desugaring of the above.

[RalfJung]

Unfortunately that desugaring does not work in a generic function... here's an alternative that works:

trait MakeVec: Sized { const EMPTY: Vec<Self>; }

impl<T> MakeVec for T {
    const EMPTY: Vec<T> = Vec::new();
}

const fn bar<T>() -> [Vec<T>; 2] {
    [<T as MakeVec>::EMPTY; 2]
}

This also shows why we need const blocks to support depending on generic parameters.

[RalfJung]

Turns out that accidentally, part of this feature is already stable.^^ From what I have seen so far, this only affects literal constants in array repeat expressions, which is fairly uncontroversial... but still entirely unintended.

We should at least figure out the code that enabled this to determine what exactly has been stabilized.

[RalfJung]

I think this is the problem:

https://github.com/rust-lang/rust/blob/2c4df6bb343e89c382ae2301505013132af77319/compiler/rustc_mir/src/borrow_check/type_check/mod.rs#L1987

In case of Operand::Copy, no check whatsoever is performed.

[oli-obk]

Operand::Copy is totally fine, because that is only valid for Copy types https://github.com/rust-lang/rust/blob/a1f7ca788d28f8df9ccd161137c3429be1ed9d0e/compiler/rustc_mir/src/transform/validate.rs#L205 the situation here is that we forgot that there's also Operand::Constant. The check should have never looked at the Operand but simply invoked Operand::ty.

I agree that this is a benign accidental stabilization, we should still officially stabilize it. I don't think spending effort on reverting the accidental stabilization is worth it in this case.

[RalfJung]

I agree that this is a benign accidental stabilization, we should still officially stabilize it. I don't think spending effort on reverting the accidental stabilization is worth it in this case.

You mean, make a PR that switches to Operand::ty but allows constants, add tests, and FCP that?

[oli-obk]

There's no need to change the source, it does exactly what we would stabilize. So just a PR that adds tests and then FCP that.

Though I guess the implementation is a bit roundabout, so we could switch to checking the operand ty first and then checking for Operand::Constant if that check fails. But other than readability, that won't change anything.

[RalfJung]

I think the if let should be changed to a match to require explicit handling of all cases.

[nikomatsakis]

+1 to match

[oli-obk]

Marking as E-easy for just the match + tests "stabilization" PR