update promotion docs - Githubissues

RalfJung commented 4 years ago

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

RalfJung commented 4 years ago

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 commented 4 years ago

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 :fearful: . 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 commented 4 years ago

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 commented 4 years ago

* 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 https://github.com/rust-lang/rust/issues/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 commented 4 years ago

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.

https://github.com/rust-lang/rust/pull/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 commented 4 years ago

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 https://github.com/rust-lang/const-eval/issues/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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Could you point me to that code?

RalfJung commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

That is the part which I missed, thanks.

RalfJung commented 4 years ago

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

RalfJung commented 4 years ago

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

rust-lang / const-eval

update promotion docs #54