Use explicit promotion for constants in repeat expressions

[oli-obk]

As per #49147 (comment)

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

cc @rust-lang/wg-const-eval

[oli-obk]

r? @RalfJung I guess?

[RalfJung]

Uh, I don't actually understand most of the code you are touching here -- the glue code that connects const-eval with the rest of the compiler. I could rubber-stamp it as it looks simple enough, but maybe we can think of someone who can do a meaningful review? @eddyb? (They have a lot on their plate though.)

However, taking a step back... this is actually a pretty big decision, I feel. Last time we talked, it was still the case that rustc first checked if the repeat expression could be promoted, and only if that failed, it checked the Copy trait. Has that changed since then? Is there a way to test this? It is crucial that we check the Copy trait first, to maintain the principle that with explicit promotion rules, failure to promote must imply failure to compile. I wonder if there is a delay_span_bug we can put somewhere to enshrine this principle in code?

And even with that principle upheld, there is still the issue that it might be hard to predict whether this code runs at run-time or at compile-time (because that depends on whether the Copy bound is satisfied). At some point there should be a T-lang FCP to decide between these two options:

• What the PR implements: the absence of the Copy bound is sufficient to trigger compile-time evaluation.
• An alternative: demand some explicit marker to request compile-time evaluation, such as [const { $expr }; N].

That doesn't have to happen here, it could be part of a stabilization FCP, but it should come up explicitly.

[oli-obk]

An alternative: demand some explicit marker to request compile-time evaluation, such as [const { $expr }; N].

I am working on a prototype for this since a few days, it seems like something we want independently of this PR

[oli-obk]

It was still the case that rustc first checked if the repeat expression could be promoted, and only if that failed, it checked the Copy trait. Has that changed since then?

This is exactly what this PR does (and what it was doing with implicit promo. If the type is !Copy, we mark the repeat element as a promotion candidate.

Is there a way to test this?

I believe we are already testing it, but I can add comments to those tests so if some day in the future something changes there, ppl know what we want to happen.

It is crucial that we check the Copy trait first, to maintain the principle that with explicit promotion rules, failure to promote must imply failure to compile. I wonder if there is a delay_span_bug we can put somewhere to enshrine this principle in code?

We can probably make the codegen code around Rvalue::Repeat ICE in case its element is a non-item constant whose type is Copy.

[RalfJung]

This is exactly what this PR does

This PR doesn't change the order in which Copy-check and promotion-check are done, does it? Where is that order determined?

Back then I was first told Copy would be checked first but later @eddyb said that was a mistake and actually promotion was checked first.

We can probably make the codegen code around Rvalue::Repeat ICE in case its element is a non-item constant whose type is Copy.

I feel it would be more direct and more local to have a delay_span_bug when promotion fails and Validator::explicit is true.

[oli-obk]

I feel it would be more direct and more local to have a delay_span_bug when promotion fails and Validator::explicit is true.

Oh that makes a lot of sense. Let's do that (or just report the tailored error right there)

[oli-obk]

r? @eddyb

I split the PR into commits where each commit passes tests. The last commit (the only one actually changing any behavior) is now super small.

[eddyb]

r=me on the changes themselves, but I don't know if we want to land the last commit without some process? I don't remember what the status of this was, did we decide already?

[oli-obk]

@Centril does this need an FCP or is an FCP for stabilization at some point enough?

[RalfJung]

Could you add a comment explaining why this check is absolutely important?
I want to be sure someone doesn't just remove it when it seems convenient.^^

The explicit field's doc comment already says a few things, so here's a proposal:

// If we use explicit validation, we carry the risk of turning a legitimate run-time
// operation into a failing compile-time operation. Make sure that does not happen
// by asserting that there is no possible run-time behavior here in case promotion fails.

[oli-obk]

What actually happens is not that we turn it into a failing compile-time op, it's that we don't promote at all, thus keeping the runtime operation, which happened to be the right thing in the cases that the assertion caught.

[RalfJung]

keeping the runtime operation

We do that because we don't even try. But the branch here is for after we tried, so we cannot be in the case where we want to keep the runtime operation.

[oli-obk]

Yes we are, this checks whether we can promote and if not, we remove it from the list. Then we process this list in promote_candidates, which can only fail to promote without ICEing in case of nested promotion candidates, where promoting one candidate eliminated the other candidate because the other candidate is part of the already promoted one.

[RalfJung]

I am entirely confused now.

Going back a step -- did you want to say above that my proposed comment was incorrect, and suggest a better one? I don't understand what is is you are trying to say here.

[oli-obk]

Yes, I believe your comment is incorrect

// If we use explicit validation, we expect promotion to succeed.
// If an explicit promotion candidate were to get "unpromoted" here,
// that means we keep an operation that we expect to happen at compile-time
// at runtime. In case of repeat elements, this would just cause an error
// later in compilation (due to multiple moves out of a `!Copy` value), but
// for `rustc_arg_required_const` function arguments, where a constant is
// required, ending up with a runtime value would likely ICE the compiler
// during codegen, or silently cause undocumented and undesired LLVM features
// to be used.

[oli-obk]

@RalfJung What do you think about the above comment?

[RalfJung]

I think we are having different views of the same thing here.

Re: your comment, I am not worried about the case where, if promotion analysis here fails, we ICE the compiler later. I am worried about the case where we do not ICE later. Because that means that, no matter whether promotion succeeds or fails, we produce a program, but the programs may do different things because we use the less conservative, "explicit promotion" rules. That is what I think this comment should explain.

[RalfJung]

(This is the delay_span_bug that I meant in my other comment, that we should add nevertheless.)

[Centril]

@Centril does this need an FCP or is an FCP for stabilization at some point enough?

The PR feels too much in-flux for me to say one way or another. A summary with the rationale, pros, and cons would be helpful.

[oli-obk]

This PR changes our behaviour in repeat expressions, turning the repeat element into a full (anonymous) constant if the type is !Copy and the repeat count is larger than 1. This means that you can write things like [Vec::new(); 42] and don't have to create an intermediate constant yourself. Before this PR you had to write

constFOO:Vec<i32> = Vec::new();[FOO;42]

because the repeat element was using the rules for promoteds instead of those for constants.

@scottmcm worries that

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].

source: #49147 (comment)

Additionally on the tracking issue @RalfJung worried (#49147 (comment)) that it may be suprising to users as to what code is accepted and what isn't, since the Copy or const rule is a first in Rust afaik

This change is forward compatible with #49147 (comment) which suggests to permit [Ok::<i32, String>(x); N], even though the expression is neither Copy nor const evaluable, but it's basically a value constructor taking only Copy values.

[RalfJung]

So could this lead to a problem if we later figure out that actually, the constant is1, and then we compile the program after a failed attempt to explicitly promote? (That would trigger the delayed_span_bug now.)

[oli-obk]

the alternative is to fail to compile (!Copy types treat generic lengths as > 1, so if it later happens to be 1 that's irrelevant). While we could permit that, we'd be adding a new source of post monomorphization errors.

[RalfJung]

I mean I am fine with this as long as it is guaranteed that this cannot ICE due to the safety net delay_span_bug this PR also adds.
I want to be sure that we do not draw ourselves into a corner where, or backwards compatibility reasons, we have to remove that safety net to fix an ICE.

[Centril]

We discussed this at Thursday's language team meeting (2020-04-09). Due to nervousness regarding extending promotion to more places, the rough consensus of those present at the meeting were that we should first consider something like const { ... } or const $expr (@Centril prefers the former, @nikomatsakis prefers the latter) which has been floated (and I believe @oli-obk has an experimental branch for it) before moving towards accepting e.g., [Vec::new(); size]. The suggested const { ... } may be introduced experimentally, with a tracking issue, but we'll want an RFC for it (perhaps for stabilization?).

We'll also add a checkbox to the tracking issue #49147 to make sure that we consider whether [const { ... }; size] has become too much of a pain. But even so, we could possibly relax the rules after stabilizing that feature.

[RalfJung]

@oli-obk at least the delay_span_bug in this PR is something we want though, IMO... could you submit a follow-up with that part?

[RalfJung]

#71198 is that follow-up. Thanks!