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

Tracking issue for RFC 2203, "Constants in array repeat expressions" #49147

Closed
Centril opened this issue Mar 18, 2018 · 77 comments
Closed

Tracking issue for RFC 2203, "Constants in array repeat expressions" #49147

Centril opened this issue Mar 18, 2018 · 77 comments

Comments

@Centril
Copy link
Contributor

@Centril Centril commented Mar 18, 2018

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:

Implementation history:

  • Initial implementation: #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 (#76001).
  • Should we maybe not perform any promotion at all, and instead require a named const, literal, or explicit const {} block for the repeat expression?
@F001
Copy link
Contributor

@F001 F001 commented May 4, 2018

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.

Rvalue::Repeat(operand, len) => if *len > 1 {
let operand_ty = operand.ty(mir, tcx);
let trait_ref = ty::TraitRef {
def_id: tcx.lang_items().copy_trait().unwrap(),
substs: tcx.mk_substs_trait(operand_ty, &[]),
};

if let Ok(count) = count {
let zero_or_one = count.val.to_raw_bits().map_or(false, |count| count <= 1);
if !zero_or_one {
// For [foo, ..n] where n > 1, `foo` must have
// Copy type:
let lang_item = self.tcx.require_lang_item(lang_items::CopyTraitLangItem);
self.require_type_meets(t, expr.span, traits::RepeatVec, lang_item);
}
}

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
Copy link
Member

@eddyb eddyb commented May 4, 2018

See rust-lang/rfcs#2203 (comment) - 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
Copy link
Member

@eddyb eddyb commented May 26, 2018

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
Copy link
Member

@RalfJung RalfJung commented May 26, 2018

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
Copy link
Member

@eddyb eddyb commented May 27, 2018

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
Copy link
Contributor

@nikomatsakis nikomatsakis commented May 28, 2018

@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
Copy link

@runelabs runelabs commented May 29, 2018

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
Copy link
Member

@eddyb eddyb commented May 29, 2018

@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
Copy link

@runelabs runelabs commented May 29, 2018

@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 😄

@eddyb
Copy link
Member

@eddyb eddyb commented May 29, 2018

<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
Copy link
Member

@eddyb eddyb commented Aug 24, 2018

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
Copy link
Member

@pnkfelix pnkfelix commented Dec 21, 2018

@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
Copy link
Contributor Author

@Centril Centril commented Dec 21, 2018

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

kennytm added a commit to kennytm/rust that referenced this issue Feb 15, 2019
split MaybeUninit into several features, expand docs a bit

This splits the `maybe_uninit` feature gate into several:

* `maybe_uninit` for what we will hopefully stabilize soon-ish.
* `maybe_uninit_ref` for creating references into `MaybeUninit`, for which the rules are not yet clear.
* `maybe_uninit_slice` for handling slices of `MaybeUninit`, which needs more API design work.
* `maybe_uninit_array` for creating arrays of `MaybeUninit` using a macro (because we don't have rust-lang#49147 yet).

Is that an okay thing to do? The goal is to help people avoid APIs we do not want to stabilize yet. I used this to make sure rustc itself does not use `get_ref` and `get_mut`.

I also extended the docs to advise against uninitialized integers -- again this is something for which the rules are still being discussed.
kennytm added a commit to kennytm/rust that referenced this issue Feb 16, 2019
split MaybeUninit into several features, expand docs a bit

This splits the `maybe_uninit` feature gate into several:

* `maybe_uninit` for what we will hopefully stabilize soon-ish.
* `maybe_uninit_ref` for creating references into `MaybeUninit`, for which the rules are not yet clear.
* `maybe_uninit_slice` for handling slices of `MaybeUninit`, which needs more API design work.
* `maybe_uninit_array` for creating arrays of `MaybeUninit` using a macro (because we don't have rust-lang#49147 yet).

Is that an okay thing to do? The goal is to help people avoid APIs we do not want to stabilize yet. I used this to make sure rustc itself does not use `get_ref` and `get_mut`.

I also extended the docs to advise against uninitialized integers -- again this is something for which the rules are still being discussed.
@eddyb
Copy link
Member

@eddyb eddyb commented Apr 7, 2019

I left a comment elsewhere, I think it would be fine if this were restricted to Rust 2018: #54542 (comment)

@Centril
Copy link
Contributor Author

@Centril Centril commented May 21, 2019

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). :)

@oli-obk oli-obk removed the S-blocked label May 21, 2019
@oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Dec 1, 2020

Oh... I didn't think about that. That's weird, but I found out where it happens:

if self.qualif_local::<qualifs::NeedsDrop>(place.local) {
only runs for the lifetime extension. The same with interior mutability and derefs... and mutable references!?!

EDIT: Ah, good, mutable references are definitely still prevented: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=fd4d892e52203f0c5c262f38e0b4e88f

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Dec 1, 2020

Mutable references and interior mutability are also checked here:

Rvalue::Ref(_, kind, place) => {

And deref here:

ProjectionElem::Deref => {

Lifetime extension is special in that, if I understood correctly, given &EXPR.proj1.proj2, it actually promotes EXPR and then performs runtime projections. That's why it has its own look at the involved projections.


But NeedsDrop is not checked anywhere else. I am wondering, does it need to? Array repeat expressions use the const by-value, so except possibly for [expr; 0], the same drops happen at the same time whether or not promotion is involved. Still, this is in direct contradiction to our docs which say that things that need dropping are never promoted (implicitly or otherwise).

@oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Dec 1, 2020

But NeedsDrop is not checked anywhere else. I am wondering, does it need to? Array repeat expressions use the const by-value, so except possibly for [expr; 0], the same drops happen at the same time whether or not promotion is involved. Still, this is in direct contradiction to our docs which say that things that need dropping are never promoted (implicitly or otherwise).

Right, we'd have to adjust the docs to reflect the reality of this feature gate, if we still want this feature at all. We would have fewer special cases if we never implemented it and just suggested const blocks instead (which will automatically work, since the promotion code will just see a single constant, just like with explicit constants)

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Dec 1, 2020

Still, it seems strange that validate_operand or validate_local does not check NeedsDrop.

@oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Dec 1, 2020

It must be checking drops somewhere, otherwise https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=7438680b64dbd44dd0988fbba1888d2d would not error. The drop check I linked is just for the final expression

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Dec 1, 2020

That errors because drop is a function call that does not get promoted.

bors added a commit to rust-lang-ci/rust that referenced this issue Dec 20, 2020
Acknowledge that `[CONST; N]` is stable

When `const_in_array_repeat_expressions` (RFC 2203) got unstably implemented as part of rust-lang#61749, accidentally, the special case of repeating a *constant* got stabilized immediately. That is why the following code works on stable:

```rust
const EMPTY: Vec<i32> = Vec::new();

pub const fn bar() -> [Vec<i32>; 2] {
    [EMPTY; 2]
}

fn main() {
    let x = bar();
}
```

In contrast, if we had written `[expr; 2]` for some expression that is not *literally* a constant but could be evaluated at compile-time (e.g. `(EMPTY,).0`), this would have failed.

We could take back this stabilization as it was clearly accidental. However, I propose we instead just officially accept this and stabilize a small subset of RFC 2203, while leaving the more complex case of general expressions that could be evaluated at compile-time unstable. Making that case work well is pretty much blocked on inline `const` expressions (to avoid relying too much on [implicit promotion](https://github.com/rust-lang/const-eval/blob/master/promotion.md)), so it could take a bit until it comes to full fruition. `[CONST; N]` is an uncontroversial subset of this feature that has no semantic ambiguities, does not rely on promotion, and basically provides the full expressive power of RFC 2203 but without the convenience (people have to define constants to repeat them, possibly using associated consts if generics are involved).

Well, I said "no semantic ambiguities", that is only almost true... the one point I am not sure about is `[CONST; 0]`. There are two possible behaviors here: either this is equivalent to `let x = CONST; [x; 0]`, or it is a NOP (if we argue that the constant is never actually instantiated). The difference between the two is that if `CONST` has a destructor, it should run in the former case (but currently doesn't, due to rust-lang#74836); but should not run if it is considered a NOP. For regular `[x; 0]` there seems to be consensus on running drop (there isn't really an alternative); any opinions for the `CONST` special case? Should this instantiate the const only to immediately run its destructors? That seems somewhat silly to me. After all, the `let`-expansion does *not* work in general, for `N > 1`.

Cc `@rust-lang/lang` `@rust-lang/wg-const-eval`
Cc rust-lang#49147
bors added a commit to rust-lang-ci/rust that referenced this issue Dec 21, 2020
Acknowledge that `[CONST; N]` is stable

When `const_in_array_repeat_expressions` (RFC 2203) got unstably implemented as part of rust-lang#61749, accidentally, the special case of repeating a *constant* got stabilized immediately. That is why the following code works on stable:

```rust
const EMPTY: Vec<i32> = Vec::new();

pub const fn bar() -> [Vec<i32>; 2] {
    [EMPTY; 2]
}

fn main() {
    let x = bar();
}
```

In contrast, if we had written `[expr; 2]` for some expression that is not *literally* a constant but could be evaluated at compile-time (e.g. `(EMPTY,).0`), this would have failed.

We could take back this stabilization as it was clearly accidental. However, I propose we instead just officially accept this and stabilize a small subset of RFC 2203, while leaving the more complex case of general expressions that could be evaluated at compile-time unstable. Making that case work well is pretty much blocked on inline `const` expressions (to avoid relying too much on [implicit promotion](https://github.com/rust-lang/const-eval/blob/master/promotion.md)), so it could take a bit until it comes to full fruition. `[CONST; N]` is an uncontroversial subset of this feature that has no semantic ambiguities, does not rely on promotion, and basically provides the full expressive power of RFC 2203 but without the convenience (people have to define constants to repeat them, possibly using associated consts if generics are involved).

Well, I said "no semantic ambiguities", that is only almost true... the one point I am not sure about is `[CONST; 0]`. There are two possible behaviors here: either this is equivalent to `let x = CONST; [x; 0]`, or it is a NOP (if we argue that the constant is never actually instantiated). The difference between the two is that if `CONST` has a destructor, it should run in the former case (but currently doesn't, due to rust-lang#74836); but should not run if it is considered a NOP. For regular `[x; 0]` there seems to be consensus on running drop (there isn't really an alternative); any opinions for the `CONST` special case? Should this instantiate the const only to immediately run its destructors? That seems somewhat silly to me. After all, the `let`-expansion does *not* work in general, for `N > 1`.

Cc `@rust-lang/lang` `@rust-lang/wg-const-eval`
Cc rust-lang#49147
@RalfJung
Copy link
Member

@RalfJung RalfJung commented Dec 28, 2020

@oli-obk so, what do we do about the dropping thing? [expr; 0] behaves (or rather, should behave) differently depending on whether expr got promoted or not when the final value of expr needs dropping. So I think something in promotion analysis needs to reject things that need dropping. But I have no idea where the best place is to put that check -- in the Candidate::Repeat arm, or should we rather have it in validate_local or so?

@oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Dec 28, 2020

I'm still unsure we should do this at all (as mentioned earlier). I would rather make a decision on that first before putting in more effort

@RalfJung
Copy link
Member

@RalfJung RalfJung commented Dec 28, 2020

You mean, supporting only Operand::Const, i.e., const literals and const blocks, and no promotion at all? Yeah that would of course also work.

@oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Dec 28, 2020

Yes, that is what I mean. It would simplify a lot, and iirc that was always on the table.

@tmiasko
Copy link
Contributor

@tmiasko tmiasko commented Jan 24, 2021

Not checking for drop has an additional effect that enabling this feature gate changes the semantics of programs that already compile successfully (or is a separate bug?):

//#![feature(const_in_array_repeat_expressions)]

struct S(&'static str);

impl Drop for S {
    fn drop(&mut self) {
        println!("{}", self.0);
    }
}

fn main() {
    // S is dropped or not depending on the feature gate.
    [&S("a"); 4];
}
@RalfJung
Copy link
Member

@RalfJung RalfJung commented Jan 24, 2021

@tmiasko ouch...

I am more and more convinced we should do what @oli-obk suggested: remove const_in_array_repeat_expressions and the corresponding code in promotion, and rely on const blocks instead.

@tesuji
Copy link
Contributor

@tesuji tesuji commented Feb 6, 2021

The feature const_in_array_repeat_expressions was removed in #80404.
Should this issue be closed or be replaced with something else ?

@oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Feb 8, 2021

Ah good catch :D yea, this issue should be closed.

We removed this feature without replacement as the feature was extremely buggy and problematic and in the future we'll be able to use the inline_const feature to get the same behaviour albeit at a slight increase in verbosity.

@varkor varkor closed this Feb 8, 2021
@SimonSapin
Copy link
Contributor

@SimonSapin SimonSapin commented Feb 8, 2021

Should the accepted RFC be marked as "retracted" or something?

@scottmcm
Copy link
Member

@scottmcm scottmcm commented Feb 8, 2021

@SimonSapin Good call. I'll make a quick RFCs-PR and bring it up in the lang meeting tomorrow.

@bluss
Copy link
Member

@bluss bluss commented Mar 23, 2021

MaybeUninit::uninit_array's docs still link to this as a "future feature", seems like that mention should be removed

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Mar 30, 2021
update comment at MaybeUninit::uninit_array

rust-lang#49147 is closed; this now instead needs inline const expressions (rust-lang#76001).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet