update promotion docs

[RalfJung]

The promotion documentation was partially outdated (like saying that in const initializers, we wouldn't actually promote) and partially confusing (like the Vec::new example). I hope this helps.

Cc @rust-lang/wg-const-eval

[oli-obk]

const fn bodies are also explicit promotion contexts:

const fn foo() -> i32 {
    42
}

const fn bar() -> &'static i32 {
    &foo()
}

[RalfJung]

But... that's bad? They can be evaluated at runtime! And they need codegen.

[oli-obk]

sure, but you can't do anything bad as the expression must already be const evaluable

[RalfJung]

You can do tons of bad things, like divide by 0 or panic. This breaks everything, it means we cannot make const-eval failures hard errors. Just imagine doing codegen for

const fn evil() { /* do something nasty */ }

const fn bar() {
  if false { &evil() }
}

We have to evaluate the promoted for codegen, but if evaluation fails we cannot cause a hard error as that would mean we broke the build for no good reason.

Since control flow has just gotten stable in const, can we quickly fix this hole and make const fn implicit promotion contexts?

[oli-obk]

Since control flow has just gotten stable in const, can we quickly fix this hole and make const fn implicit promotion contexts?

turning it into implicit promotion contexts is a breaking change that is entirely independent of control flow

[ecstatic-morse]

Oh no. I wish I would have realized this while doing the initial refactoring of const-checking. That logic is here BTW. I can do a crater run to see how many people depend on this.

[RalfJung]

I wish I had read the code more careful to be able to provide such input more timely. No point lamenting over spilled beans, though I wonder if there is something we can do with our process to help with such things, without drowning in paperwork. The MCP mechanism could have helped, assuming that such details of the desired analysis would be spelled out in the MCP. It's much easier for me to provide such feedback based on a high-level but precise design document, than based on the code.

That logic is here BTW

This looks like const fn are not an explicit promotion context, but get some kind of special treatment that is half-way between explicit and implicit contexts? Are there other such "hybrid" cases?

I can do a crater run to see how many people depend on this.

Yes that would be great.

[ecstatic-morse]

Note to self: this has worked for every stable release after 1.35.

[RalfJung]

I have collected my current thoughts on this in a hackmd. Please feel free to expand that with missing information!

[RalfJung]

I opened an issue at rust-lang/rust#75586, so we have a dedicated place to track this problem. I will adjust the text here to reality.

[RalfJung]

When looking at the code, I found something else this document didn't mention yet: promotion of inline assembly arguments. In the future, when landing such things, could you please open an issue to adjust the docs, or (even better) a PR?

Is there anything else that I missed?

[RalfJung]

Here are some more strange things I found -- differences in promotion behavior not captured by the implicit/explicit split, and not documented here:

• There's a const_kind field of type Option<ConstContext> that is undocumented unfortunately, so I have no idea what None means. Looks like this is filled with body_const_context, so None means non-const fn? This seems to be the thing that introduces some extra differences.
• Inside a static mut, &mut [...] is also promoted. Why does that make any sense? Promoteds are consts I thought, they have read-only memory, don't they?
• There's a comment saying &mut [] is promoted only inside functions, but if I understand the code correctly this is actually just done inside non-const fn.
• There's a special case here where I cannot figure out what it does.
• union field accesses are checked depending on the const_kind (not explicit vs implicit as I expected).
• I don't understand this one at all. Consts cannot refer to statics, sure, but promotion isn't the place to enforce that property, or is it?
• validate_rvalue contains two more const_kind.is_none().
• There's something here which looks like a duplicate of the &mut [...] check mentioned above, and this is a duplicate of the &mut [] check mentioned above. Or are they checking different things?
• And finally, this is where we treat const fn like const/static initializers for function calls.

So to conclude, we have an entire suite of differences that has nothing to do with explicit vs implicit, and that was never mentioned in any discussion or documented anywhere 😨 . This mess has dimensions I was not aware of, and I was paying close attention on this topic for years now, just without ever reading the actual code and relying on docs instead. The docs say const/static initializers are explicit promotion contexts, which does not match the code at all -- explicit is `false for them! Something is very wrong with our approach here.

Is there any chance we can ditch const_kind for promotion analysis, and make that all just depend on implicit vs explicit? That is at least a distinction that we have explored in some depth and we have some ideas for what the possible tradeoffs are.

[RalfJung]

I just hope this is not representative of the qualttiy of our design documents for other things like const qualification or pattern matching. If I have to read all that code to verify that the documents make sense, then what is the point of having such documents? (Well I actually read a bunch of const qualification code here and there and generally the latest version of that was quite easy to follow. I don't look at min_const_fn qualification though.^^)

I'm somewhat frustrated, TBH. I realize everyone is doing their best in the amount of time that they have. But also, given the amount of time I have, I just cannot read all that code -- if we cannot make this work based on design documents, I don't see how we can make it work at all. And I fear some of this will be really hard to sort out as it has been stable forever.

[oli-obk]

* Inside a `static mut`, `&mut [...]` is also promoted. Why does that make any sense? Promoteds are consts I thought, they have read-only memory, don't they?

static mut allowed this from long before promotion, which is why we preserved it. We could start throwing future incompat warnings at ppl pushing them to use safe wrappers (see also rust-lang/rust#53639)

* There's a special case [here](https://github.com/rust-lang/rust/blob/860d599d5b11db275a829a3f898560405fa84bbe/src/librustc_mir/transform/promote_consts.rs#L523) where I cannot figure out what it does.

this is a very recent change fixing a regression where a static wasn't able to promote its use of another static.

* I don't understand [this one](https://github.com/rust-lang/rust/blob/860d599d5b11db275a829a3f898560405fa84bbe/src/librustc_mir/transform/promote_consts.rs#L584) at all. Consts cannot refer to statics, sure, but promotion isn't the place to _enforce_ that property, or is it?

we carried this over from the original promotion impl which was also the const checker. We should look into just removing that check.

Is there any chance we can ditch const_kind for promotion analysis, and make that all just depend on implicit vs explicit? That is at least a distinction that we have explored in some depth and we have some ideas for what the possible tradeoffs are.

Some of that mess is actually us trying to contain preexisting messes. We can defintely try to cut down on the variants of ConstKind by cratering things like the static mut stuff. I'm not sure how to remove the *&STATIC hack, since we try to keep mir building as simple as possible. But that's not actually using const_kind, so we can ignore it I guess?

[RalfJung]

static mut allowed this from long before promotion, which is why we preserved it. We could start throwing future incompat warnings at ppl pushing them to use safe wrappers (see also rust-lang/rust#53639)

That doesn't answer the question though -- if promoteds are evaluated like consts, won't this stuff be put into immutable memory? Or is there some other special handling elsewhere to make it mutable?

this is a very recent change fixing a regression where a static wasn't able to promote its use of another static.

rust-lang/rust#74945, I guess.
What about uses of statics elsewhere? Why would we only do this inside another static?
Looks like this is about e.g. &(3, STATIC) or so. Seems reasonable to promote everywhere?

Some of that mess is actually us trying to contain preexisting messes.

Sure. But that doesn't explain why it is not described in documents like this one. The first step to cleaning up a mess is to properly understand the mess, and to me that means describing it in a higher level than code. The point of this document here was not to describe the ideal scenario, but to describe reality.

[RalfJung]

I also just realized that our sanity check for explicit promotion, namely "if it fails to promote, it fails to compile", does not make sense for lifetime extension in const/static initializers. (I am so happy that we have this terminology now. :D ) So... the justification for using relaxed rules has to be a different one (and also the relaxed rules are different, but I am not sure if there is a good justification for that). This is in fact the same observation I already made in #53 -- lifetime extension in const/static initializers is very special.

I see this as yet another argument that the old scheme of not doing lifetime extension via promotion, but simply by just dropping the local, actually made a lot of sense. The only thing that doesn't make sense is that this would be decided by a "promotion" analysis -- the concerns are very different, e.g. interior mutability would not be a problem I think.

[oli-obk]

Or is there some other special handling elsewhere to make it mutable?

I hope we're putting it in mutable memory in the interner, if not, we have an unsoundness and can fix it by just not promoting it anymore

Seems reasonable to promote everywhere?

Yes, and we're doing that, we're just not allowing statics in constants, so we don't ever test this code path.

[RalfJung]

Yes, and we're doing that, we're just not allowing statics in constants, so we don't ever test this code path.

If I read the code correctly, we are not promoting &(3, STATIC) in a fn, as that would not enter the if let Some(hir::ConstContext::Static(..)) = self.const_kind.

EDIT: yeah, this fails to compile:

fn foo() -> &'static (i32, i32) {
    static FOO: i32 = 13;
    &(42, FOO)
}

[RalfJung]

I hope we're putting it in mutable memory in the interner, if not, we have an unsoundness and can fix it by just not promoting it anymore

The generated LLVM IR says it is indeed mutable only when using &mut:

pub static RO_TEST: &[i32] = { let x = &[1,2,3,4]; x };
pub static mut MUT_TEST: &mut [i32] = { let x = &mut [0xFFFF]; x };

generates

@alloc2 = private unnamed_addr constant <{ [16 x i8] }> <{ [16 x i8] c"\01\00\00\00\02\00\00\00\03\00\00\00\04\00\00\00" }>, align 4
@_ZN10playground7RO_TEST17h126819ded3a7c0e9E = constant <{ i8*, [8 x i8] }> <{ i8* getelementptr inbounds (<{ [16 x i8] }>, <{ [16 x i8] }>* @alloc2, i32 0, i32 0, i32 0), [8 x i8] c"\04\00\00\00\00\00\00\00" }>, align 8, !dbg !0
@alloc6 = private unnamed_addr global <{ [4 x i8] }> <{ [4 x i8] c"\FF\FF\00\00" }>, align 4
@_ZN10playground8MUT_TEST17h700ac057062a4b5dE = global <{ i8*, [8 x i8] }> <{ i8* getelementptr inbounds (<{ [4 x i8] }>, <{ [4 x i8] }>* @alloc6, i32 0, i32 0, i32 0), [8 x i8] c"\01\00\00\00\00\00\00\00" }>, align 8, !dbg !13

I just don't understand why, given this line.

[oli-obk]

I just don't understand why, given this line.

That surprised me, too, but I found the reason quickly: https://github.com/rust-lang/rust/blob/7189ca604ad5d9dfff7d0aeef6a42c89d73cbac7/src/librustc_mir/const_eval/eval_queries.rs#L61

[RalfJung]

Not sure how that explains anything, isn't static_mutability().is_some() supposed to answer the question of "is this a static"?

The query docs say

        /// Returns `Some(mutability)` if the node pointed to by `def_id` is a static item.
        query static_mutability(def_id: DefId) -> Option<hir::Mutability> {
            desc { |tcx| "looking up static mutability of `{}`", tcx.def_path_str(def_id) }
        }

[oli-obk]

Yes, but for promoteds we just check their owner and not whether they are promoted unless it's not a static

So promoteds in statics are interned as statics

[RalfJung]

Could you point me to that code?

[RalfJung]

Or is there some other special handling elsewhere to make it mutable?

I hope we're putting it in mutable memory in the interner, if not, we have an unsoundness and can fix it by just not promoting it anymore

Btw, not using promotion for lifetime extension of &mut [...] would also entirely bypass this problem in a backwards-compatible way. ;)

[RalfJung]

Btw, after I raised my frustration earlier I should also say that the promote_consts.rs file actually is fairly pleasant to read, much better than its predecessor(s) -- I mean not in terms of all the mess that's encoded in there, but that's pre-existing, and there are some concrete checks I did not get, but the overall structure makes sense (after I realized how important const_kind is).

[RalfJung]

Yes, but for promoteds we just check their owner and not whether they are promoted unless it's not a static

So promoteds in statics are interned as statics

I only found this code and it seems to require something with a DefIndex (is that the same as a DefId?), and not look into the parent of a promoted.

[oli-obk]

The original thing I linked is what decides the interning mode: https://github.com/rust-lang/rust/blob/43bec40138bab10c08ac916bff2f2f81b2b70669/src/librustc_mir/const_eval/eval_queries.rs#L60-L70 So I'm not sure what else to link. This is the code that decides, and the match is only happening on the DefId, and the DefId of a promoted is the DefId of its owner.

[RalfJung]

the DefId of a promoted is the DefId of its owner.

That is the part which I missed, thanks.

[RalfJung]

I updated the docs now to hopefully reflect reality. Could someone check that?

[RalfJung]

@eddyb calls it "outermost scope" rule. I am not sure why. I will try to find out.

[RalfJung]

I updated these docs to reflect the current situation after the recent PRs that cleaned things up a bit.