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

rustc/rustc_mir: Implement RFC 2203. #61749

Merged
merged 5 commits into from
Jul 19, 2019

Conversation

davidtwco
Copy link
Member

This PR implements RFC 2203, allowing constants in array repeat
expressions. Part of #49147.

r? @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 11, 2019
src/librustc/mir/mod.rs Outdated Show resolved Hide resolved
@davidtwco davidtwco force-pushed the rfc-2203-const-array-repeat-exprs branch from 0ec77d9 to fd8fc14 Compare June 11, 2019 21:32
@rust-highfive

This comment has been minimized.

.span_label(span, &format!(
"the trait `{}` is not implemented for `{}`",
copy_path, ty,
))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @nikomatsakis @matthewjasper This is not a proper trait error, so it would miss the "backtrace" that one might get otherwise - any suggestions on how to fix that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've started re-using some existing functions for diagnostics in this PR. It appears to work correctly but it might not produce the full "backtrace" that you want.

@eddyb
Copy link
Member

eddyb commented Jun 12, 2019

To expand on #61749 (comment), this example:

#[derive(Copy, Clone)]
struct Foo<T>(T);
fn main() {
    [Foo(String::new()); 4];
}

Currently errors with:

error[E0277]: the trait bound `std::string::String: std::marker::Copy` is not satisfied
 --> src/main.rs:4:5
  |
4 |     [Foo(String::new()); 4];
  |     ^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::marker::Copy` is not implemented for `std::string::String`
  |
  = note: required because of the requirements on the impl of `std::marker::Copy` for `Foo<std::string::String>`
  = note: the `Copy` trait is required because the repeated element will be copied

The "required because of the requirements on the impl of ..." part would now be missing, I think.
There might be a relatively easy way to produce a proper trait error, even from the NLL type_check.
Either way, it would be good to have tests like this to properly compare the effect.

@davidtwco
Copy link
Member Author

Either way, it would be good to have tests like this to properly compare the effect.

I've added this example as a test.

@eddyb
Copy link
Member

eddyb commented Jun 12, 2019

@davidtwco Sadly, without adding a test like this (or a more complex one) on master first, it's hard to see the changes in diagnostics.

@rust-highfive

This comment has been minimized.

@davidtwco
Copy link
Member Author

@eddyb I've ran that example with another working directory (at most 24 hours old behind master) and this is the current error:

error[E0277]: the trait bound `std::string::String: std::marker::Copy` is not satisfied
 --> ../rust0/src/test/ui/consts/rfc-2203-const-array-repeat-exprs/trait-error.rs:8:5
  |
8 |     [Foo(String::new()); 4];
  |     ^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::marker::Copy` is not implemented for `std::string::String`
  |
  = note: required because of the requirements on the impl of `std::marker::Copy` for `Foo<std::string::String>`
  = note: the `Copy` trait is required because the repeated element will be copied

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.

@davidtwco davidtwco force-pushed the rfc-2203-const-array-repeat-exprs branch from 3c7ee48 to 99ebd9a Compare June 12, 2019 09:48
@rust-highfive

This comment has been minimized.

@davidtwco davidtwco force-pushed the rfc-2203-const-array-repeat-exprs branch 2 times, most recently from 55546ac to fbf884b Compare June 12, 2019 12:52
@davidtwco davidtwco force-pushed the rfc-2203-const-array-repeat-exprs branch from fbf884b to 72ec9d7 Compare June 12, 2019 16:06
@bors

This comment has been minimized.

@davidtwco davidtwco force-pushed the rfc-2203-const-array-repeat-exprs branch from b9cae48 to 9d0ce71 Compare June 17, 2019 17:49
@bors

This comment has been minimized.

@davidtwco davidtwco force-pushed the rfc-2203-const-array-repeat-exprs branch from 9d0ce71 to 2c10ded Compare June 19, 2019 17:32
@bors

This comment has been minimized.

@davidtwco davidtwco force-pushed the rfc-2203-const-array-repeat-exprs branch from 2c10ded to 4a185c0 Compare June 23, 2019 18:28
@eddyb
Copy link
Member

eddyb commented Jun 25, 2019

What is this blocked on, if anything?

@davidtwco
Copy link
Member Author

I'm not aware of any outstanding concerns, only that it isn't a proper trait error with a backtrace being emitted.

@bors

This comment has been minimized.

@davidtwco davidtwco force-pushed the rfc-2203-const-array-repeat-exprs branch from 4a185c0 to a357743 Compare June 25, 2019 19:23
@bors

This comment has been minimized.

@davidtwco davidtwco force-pushed the rfc-2203-const-array-repeat-exprs branch from 8168d22 to 4b1bc2d Compare July 7, 2019 19:23
@eddyb
Copy link
Member

eddyb commented Jul 11, 2019

r? @matthewjasper or @nikomatsakis - We need a way to report proper trait errors, with their cause/origin backtraces.

@rust-highfive rust-highfive assigned matthewjasper and unassigned eddyb Jul 11, 2019
@eddyb
Copy link
Member

eddyb commented Jul 18, 2019

@davidtwco I'm sorry for blocking this, I don't think the diagnostic regressions are worth the wait.
Especially given situations like #53491 (comment).

@@ -3,11 +3,12 @@

fn f<T: Copy, const N: usize>(x: T) -> [T; N] {
[x; {N}]
//~^ ERROR array lengths can't depend on generic parameters
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh I don't remember this error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh it's for [x; n] expressions, not types. Makes sense (although it should use a ty::Const instead of hardcoding an integer, cc @varkor @oli-obk ).

@eddyb
Copy link
Member

eddyb commented Jul 18, 2019

@bors r+ p=2 rollup=never

@bors
Copy link
Contributor

bors commented Jul 18, 2019

📌 Commit 4b1bc2d has been approved by eddyb

@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 Jul 18, 2019
@bors
Copy link
Contributor

bors commented Jul 18, 2019

⌛ Testing commit 4b1bc2d with merge a336998...

bors added a commit that referenced this pull request Jul 18, 2019
…=eddyb

rustc/rustc_mir: Implement RFC 2203.

This PR implements RFC 2203, allowing constants in array repeat
expressions. Part of #49147.

r? @eddyb
@bors
Copy link
Contributor

bors commented Jul 19, 2019

☀️ Test successful - checks-azure
Approved by: eddyb
Pushing a336998 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 19, 2019
@bors bors merged commit 4b1bc2d into rust-lang:master Jul 19, 2019
@davidtwco davidtwco deleted the rfc-2203-const-array-repeat-exprs branch July 19, 2019 11:43
|
= help: the following implementations were found:
<std::option::Option<T> as std::marker::Copy>
= note: the `Copy` trait is required because the repeated element will be copied
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't mention the feature gate :(

)) = self.body[bb].statements[stmt_idx].kind {
promoted_temps.insert(index);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized, this is unnecessary, only Candidate::Ref cares (it removes StorageDead from the temp that would need to be 'static).

_,
box Rvalue::Ref(_, _, Place::Base(PlaceBase::Local(index)))
) = self.body[bb].statements[stmt_idx].kind {
promoted_temps.insert(index);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oli-obk Wow this is wrong?! const FOO: &i32 = { let x = &(5, false).0; x }; fails because of it, lol.

Centril added a commit to Centril/rust that referenced this pull request Oct 28, 2019
…error, r=ecstatic-morse

suggest `const_in_array_repeat_expression` flag

This PR adds a suggestion to add the `#![feature(const_in_array_repeat_expression)]` attribute to the crate when a promotable expression is used in a repeat expression and the feature gate is not enabled.

Unfortunately, this ended up being a little bit more complex than I anticipated, which may not have been worth it given that this would all be removed when the feature is stabilized. However, with rust-lang#65732 and rust-lang#65737 being open, and the feature gate having not been being suggested to potential users, the feature might not be stabilized in a while, so maybe this is worth landing.

cc @Centril (addresses [this comment](rust-lang#61749 (comment)))
r? @ecstatic-morse (opened issues related to RFC 2203 recently)
let arr: [Option<String>; 2] = [None::<String>; 2];
//~^ ERROR the trait bound `std::option::Option<std::string::String>: std::marker::Copy` is not satisfied [E0277]
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out this test was not thorough enough, and some other cases sneaked past the feature gate.

bors added a commit to rust-lang-ci/rust that referenced this pull request 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

7 participants