E0492: borrow of an interior mutable value may end up in the final value during const eval when no inner mutability is involved

[sarah-quinones]

This code works on stable, but not latest nightly

#[derive(Copy, Clone)]
pub struct S<T>(T);

#[doc(hidden)]
pub trait DynTrait {
    type VTable: Copy + 'static;
    const VTABLE: &'static Self::VTable;
}

impl<T: DynTrait> DynTrait for S<T> {
    type VTable = T::VTable;
    const VTABLE: &'static Self::VTable = &{ *T::VTABLE };
}

Error:

error[E0492]: constants cannot refer to interior mutable data
  --> src/lib.rs:12:43
   |
12 |     const VTABLE: &'static Self::VTable = &{ *T::VTABLE };
   |                                           ^^^^^^^^^^^^^^^ this borrow of an interior mutable value may end up in the final value

rustc --version --verbose:

rustc 1.78.0-nightly (6672c16af 2024-02-17)
binary: rustc
commit-hash: 6672c16afcd4db8acdf08a6984fd4107bf07632c
commit-date: 2024-02-17
host: x86_64-unknown-linux-gnu
release: 1.78.0-nightly
LLVM version: 18.1.0

[RalfJung]

This is probably a regression introduced by #120932.

However, the error is right: this is a reference to Self::VTable, and the impl is generic, so for all this code knows, Self::VTable might have interior mutability.

#120932 fixed a bug in our interior-mutablity-checking code. I think what this actually shows is that the code never should have been accepted in the first place. But we may need a crater run to figure out how bad this regression is. Is there a way to retroactively do a crater run for #120932?

Cc @oli-obk

[sarah-quinones]

is there a trait bound or something similar that can be added to tell the compiler that there won't be any internal mutability?

[RalfJung]

is there a trait bound or something similar that can be added to tell the compiler that there won't be any internal mutability?

Unfortunately not on stable -- see #60715.

[RalfJung]

Interestingly I think there is no soundness issue on stable here: in &{ *T::VTABLE }, T::VTABLE is itself a const, and consts can never point to anything interior mutable without const_refs_to_static, and therefore just saying that every *place expression in a const has no interior mutability is sound. And with statics we can't write generic code, so we'd need a concrete type that is both Copy and !Freeze, which does not exist. So by sheer luck there is no soundness issue here.

---

@sarah-ek is there a reason why you wrote &{ *T::VTABLE } instead of &*T::VTABLE? The extra curly braces make a big difference here. With curly braces, this means "please create a new temporary memory allocation storing a copy of *T::VTABLE, and then create a reference to that". Without curly braces, this means "please create a reference to the existing allocation that T::VTABLE points to". Creating temporary allocations with interior mutability is something we try to prevent, but due to a bug the code you showed slipped through those checks.

Without curly braces, the code still works fine on nightly with the fixed checks:

#[derive(Copy, Clone)]
pub struct S<T>(T);

#[doc(hidden)]
pub trait DynTrait {
    type VTable: Copy + 'static;
    const VTABLE: &'static Self::VTable;
}

impl<T: DynTrait> DynTrait for S<T> {
    type VTable = T::VTable;
    const VTABLE: &'static Self::VTable = &*T::VTABLE;
}

[sarah-quinones]

this is a simplification of a different scenario. the actual code is here https://github.com/sarah-ek/equator/blob/444beec918bb83e6c9e400a1849dcf015795a7b0/equator/src/lib.rs#L581-L584

the reason i want to borrow is so i can force the promotion to static when used as a part of my own assert! macro

[RalfJung]

But promotion to static is exactly what we don't want to do when there might be interior mutability. So that kind of code is definitely not intended to work.

I don't know how constrained you are with the types you can use here; would something like this work for you?

impl<Lhs: DynDebug, Rhs: DynDebug> DynDebug for AndExpr<Lhs, Rhs> {
    type VTable = (&'static Lhs::VTable, &'static Rhs::VTable);
    const VTABLE: &'static Self::VTable = &(Lhs::VTABLE, Rhs::VTABLE);
}

[sarah-quinones]

yeah, i ended up going with that

[RalfJung]

Okay, glad to hear you have a work-around.

We still need to decide what to do with this accidental regression though. It is IMO a bugfix in our analysis, even though the stars align to make it not obviously unsound -- though it's also possible that I missed something in my analysis above.

Let's see what Oli says.

[oli-obk]

I think the right thing would be to error out as TooGeneric if we encounter such a constant.

Or just not intern generic constants at all, but error out as TooGeneric before interning and validation in order to still get the const eval coverage of the rest.

Evaluating generic constants is best effort anyway, and repeated from scratch for every monomorphization, at which point we'll only error if there is an actual mutable allocation there

[RalfJung]

I think the right thing would be to error out as TooGeneric if we encounter such a constant.

This is not an interpreter error, it is a const-checking error (i.e., it is raised in compiler/rustc_const_eval/src/transform/check_consts/check.rs). Those can't stop with "too generic", I think?

[oli-obk]

Oh 🤦 I was wondering how we got that error wording from interning.

I'll need to think about this some more.

[juntyr]

I just stumbled across the same issue in my weekly CI. In my nightly crate, I am at one point building a HList that will be layout-equivalent to an array. The following code now fails (this is somewhat simplified to collapse helper traits):

pub trait ComputeSet {
    type TyHList: 'static + Copy;
    const TYS: &'static Self::TyHList;
}

impl<H2: TypeLayout, T: ComputeSet> ComputeSet for Cons<H2, T> {
    type TyHList = Cons<&'static crate::TypeLayoutInfo<'static>, T::TyHList>;

    const TYS: &'static Self::TyHList = &Cons {
        head: &H2::TYPE_LAYOUT,
        tail: *T::TYS,
    };
}

#[repr(C)]
#[derive(Copy, Clone)]
pub struct Cons<H, T> {
    head: H,
    tail: T,
}

pub unsafe trait TypeLayout: Sized {
    const TYPE_LAYOUT: TypeLayoutInfo<'static>;
}

#[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
pub struct TypeLayoutInfo<'a> {
    ...
}

Error:

error[E0492]: constants cannot refer to interior mutable data
   --> src/typeset.rs:163:45
    |
163 |           const TYS: &'static Self::TyHList = &Cons {
    |  _____________________________________________^
164 | |             head: &H2::TYPE_LAYOUT,
165 | |             tail: *T::TYS,
166 | |         };
    | |_________^ this borrow of an interior mutable value may end up in the final value

For more information about this error, try `rustc --explain E0492`.
error: could not compile `const-type-layout` (lib) due to 1 previous error

The complete version can be found here:
https://github.com/juntyr/const-type-layout/blob/990a562c62a44dcc0c96e5ebf9790ce4352d6de5/src/typeset.rs#L152-L161

rustc --version --verbose:

rustc 1.78.0-nightly (2bf78d12d 2024-02-18)
binary: rustc
commit-hash: 2bf78d12d33ae02d10010309a0d85dd04e7cff72
commit-date: 2024-02-18
host: x86_64-unknown-linux-gnu
release: 1.78.0-nightly
LLVM version: 18.1.0

[apiraino]

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-medium

[RalfJung]

Crater results are in. The equator crate (the crate of the OP) is the only regression across all of crates.io.

So I am inclined to classify this as a bugfix -- we shouldn't have allowed such temporaries with potentially interior mutable data to be creates in the first place. We should prioritize letting people write generic code while preserving the information that a type has no interior mutability, i.e., letting people use Freeze bounds on stable.

[oli-obk]

@rust-lang/lang We accidentally fixed a bug (by cleaning up const checking code in a way that inherently forbade the bug). This bug has been found to have a single regression. The issue is already being worked around.

The bug fix is that we now correctly forbid using associated constants that have generic parameters (either themselves whenever we get GACs, or from their trait), if those constants are used behind references. This was a hole in our checks for preventing interior mutability behind references in constants. These are not stable yet, and if the constant is generic, we cannot know whether it has interior mutability and need to reject it.

[traviscross]

@rustbot labels -I-lang-nominated

As reflected above, we discussed this in lang triage today. It's now in FCP, so let's unnominate.

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[p-avital]

OHHH NO... I missed the comment period: this being stabilized before core::marker::Freeze has killed stabby's ABI-stable vtables :(

[nikomatsakis]

Can you elaborate Pierre?

…

On Thu, May 2, 2024, at 6:57 PM, Pierre Avital wrote: OHHH NO... I missed the comment period: this being stabilized before `core::marker::Freeze` has killed `stabby`'s ABI-stable vtables :( — Reply to this email directly, view it on GitHub <#121250 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABF4ZQ6AKHBXB2626N7UILZAJWANAVCNFSM6AAAAABDNYA6XOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAOJRGA3DAMBVGQ>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[p-avital]

I've elaborated quite a bit on this thread.

TL;DR:

• stabby uses this "bug" to use associated consts as a form of static (since we can't have generic statics), allowing its vtables to exist at no-runtime cost.
  • The use is sound, since the vtables are always just a bunch of function pointers with no interior mutability.
• I was warned about this rule breaking stabby back in March. I addressed it by auto-detecting nightly and adding core::marker::Freeze bounds where relevant (proving the soundness of the use).
  • I've since been told this was a bad practice as it messes with crater, but I was expecting (and have since tested) that beta runs would find stabby.
    • crater ended up missing stabby because it ran on 2.0.1 instead of the latest 4.0.1, and 2.0.1 fails to compile on 1.77 onwards due to the alignment change of u128 on x86 (which is part of what stabby is designed to do, it's an ABI-as-library after all).
    • crater did catch one other crate using the same trick as stabby to also generate vtables in a sound way.
      • I'm arguably a bit salty that this was ignored, as no unsound uses of the bug have been caught by crater, but I can understand wanting to fix that before unsound uses crop up.
  • I arguably trusted too much in crater, as I thought I'd left a comment about this somewhere, but it's clearly not in this thread.
• I have since added a "hotfix" in stabby to allow it to continue working on 1.78 and above with runtime overhead and too much unsafe for my taste:
  • previous versions and nightly keep on using the previous technique
  • 1.78 onwards without nightly leak the vtables to the heap and register them in a global lock-free set.
    • Naive performance testing by a user shows that this adds about 75ns to the construction time of stabby's version of a trait object when there's only one candidate vtable. I'm currently writing my own benchmarks to see how this scales.
  • My plan is to revert to the previous technique as soon as core::marker::Freeze becomes stable

[RalfJung]

crater did catch one other crate using the same trick as stabby to also generate vtables in a sound way.

That other use had a trivial work-around, though. That's why we felt it was fine to go ahead with the breakage.

But yeah the fact that crater tested an old stabby is unfortunate. I still don't know why that happened, but that's on us and we should figure out how to avoid it.

I've since been told this was a bad practice as it messes with crater

Not just with crater. It violates the basic principle that nightly features are opt-in. See #120804, #124339.

[p-avital]

That other use had a trivial work-around, though. That's why we felt it was fine to go ahead with the breakage.

Riiight, I originally thought of doing vtables like that, but decided against it due to the number of indirections this would add to function calls.

I guess I'll add it as an option, as right now my workaround does require allocations. I'll let users pick whatever runs best for them through a cfg flag.

But yeah the fact that crater tested an old stabby is unfortunate. I still don't know why that happened, but that's on us and we should figure out how to avoid it.

I guess it was in an effort to reduce the (probably huge) resources a crater run takes by reusing the results from a previous run?

Not just with crater. It violates the basic principle that nightly features are opt-in. See #120804, #124339.

Yeah, but arguably, this detection was entirely to address a breaking change from stable (two wrongs to make a right :p), not the usual "this cool nightly feature makes my crate better so I want it on if it's around".

It was very much a way to expand the set of compiler versions stabby could run on, with the expectation that if the nightly feature changed in some way, that would require its own solution in its own time.

Since stabby's position as a "std-alternative" means it's expected to get very deeply embedded in a dependency tree, I didn't want a project to go out of their way to accommodate stabby just because one of their dependencies used it. I also didn't want potential stabby dependents to themselves need to accommodate this breaking change in nightly.

That's also why some of stabby's tuning handles are cfgs instead of features: we're not talking addition with these, just selecting a strategy which should be in the hands of the final dependent.

[nikomatsakis]

Thanks for elaborating on what stabby is doing! Helpful. If we can zoom out from the specific mechanism, the goal you are looking for is creating generic, constant vtables? That's interesting because it's the same thing that the Rust-for-Linux project would like to do. (I'm not sure if they require generics per se.) They describe this as depending on ``const_mut_refs` or `const_refs_to_static`, is that something you could theoretically use as well?` (see Rust-for-Linux/linux#2)

[p-avital]

They describe this as depending on const_mut_refs or const_refs_to_static, is that something you could theoretically use as well? (see Rust-for-Linux/linux#2)

I'm not so sure:

• const_refs_to_static allows references to statics in consts, but really, if I could have generic statics, my issues would probably be fixed already, since I'm using reference-typed associated consts as a way to emulate associated statics.
• const_mut_ref seems out of scope to me for this feature, although I remember some spots where I could use them elsewhere :)

Support for associated/generic statics would address my issue entirely, and would probably have many cool uses, but that seems far away still.

Alternatively, stabilizing core::marker::Freeze would also solve all of my problems instantly.

Of note: I've just realized the workaround equator took would still be problematic to stabby, as it would introduce a separate (incompatible) version of the representation of trait objects. I still think allowing trait objects to run without libc (which is required for my current 1.78 workaround) is valuable enough that I'll try to implement it (working out a few kinks right now) and find ways to advertise the compromises (including ecosystem split) that come with each representation.

[RalfJung]

Generic statics are pretty much impossible, to my knowledge. We'd have to guarantee a unique address when it is independently instantiated with the same generics by different crates, it is unclear whether linkers can do that for us. Using Freeze is the right solution for your problem. We should get that stabilized. But it needs an RFC first.

[nikomatsakis]

I don't know why we would have to guarantee that. (We could for example require Freeze and say uniqueness is not guaranteed?) but this seems off topic for this thread, so I guess I'll open the discussion elsewhere.

…

On Fri, May 10, 2024, at 12:47 PM, Ralf Jung wrote: Generic statics are pretty much impossible, to my knowledge. We'd have to guarantee a unique address when it is independently instantiated with the same generics by different crates, it is unclear whether linkers can do that for us. Using Freeze is the right solution for your problem. We should get that stabilized. But it needs an RFC first. — Reply to this email directly, view it on GitHub <#121250 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABF4ZXCA35CB7BEZDL6AN3ZBSQVRAVCNFSM6AAAAABDNYA6XOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCMBUGM4TAMZVGE>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[p-avital]

I don't know why we would have to guarantee that. (We could for example require Freeze and say uniqueness is not guaranteed?) but this seems off topic for this thread, so I guess I'll open the discussion elsewhere.

I'd argue the same guarantees should be made as for static as it is today. I would have guessed that ensuring proper linkage for generic statics and generic functions would have similar challenges (which have been solved for functions, evidently); hence my general confusion at generic statics not being a thing, but I'll gladly admit to not knowing enough about this to foresee some static-specific pitfalls.

[p-avital]

Using Freeze is the right solution for your problem. We should get that stabilized. But it needs an RFC first.

I'll write an RFC then :)

[RalfJung]

I don't know why we would have to guarantee that. (We could for example require Freeze and say uniqueness is not guaranteed?)

The fact that statics are a place and have a unique address is pretty much their defining feature, and what separates them from consts (which are values and where addresses are pretty unreliable). If a unique address is not needed and Freeze is enough then a const should be used, not a static.

[RalfJung]

I'd argue the same guarantees should be made as for static as it is today.

That was also my argument.

I would have guessed that ensuring proper linkage for generic statics and generic functions would have similar challenges (which have been solved for functions, evidently); hence my general confusion at generic statics not being a thing, but I'll gladly admit to not knowing enough about this to foresee some static-specific pitfalls.

For functions we do not guarantee unique addresses (and same for vtables, FWIW). You can find several issues in the issue tracker of people being confused by the behavior of == on function pointers (or raw pointers that carry vtables). For statics I don't think we can afford this: their mutability means we really need uniqueness. (And if we don't support interior mutability for generic statics and don't guarantee a unique address one may as well use a const, and avoid the confusion.)

[p-avital]

As promised, here's the stabilization RFC for core::marker::Freeze: rust-lang/rfcs#3633

[Kyykek]

@RalfJung I'm struggling to understand why the change to static promotion was even necessary. From what I've seen and tested myself, the only scenario where I've gotten an error in 1.78 for code that compiled in 1.77 is when dereferencing consts in struct initializers and then static promoting the result, but this should always be ok to do if I'm not mistaken.
Let's consider the original code:

impl<Lhs: DynDebug, Rhs: DynDebug> DynDebug for AndExpr<Lhs, Rhs> {
    type VTable = (&'static Lhs::VTable, &'static Rhs::VTable);
    const VTABLE: &'static Self::VTable = &(*Lhs::VTABLE, *Rhs::VTABLE);
}

Here, Lhs::VTABLE is a const of type &T, which requires and therefore can be said to imply T: Freeze. Hence, we can prove that (*Lhs::VTABLE, *Rhs::VTABLE): Freeze, since (T, U): Freeze if T: Freeze and U: Freeze. Therefore, static promotion on (*Lhs::VTABLE, *Rhs::VTABLE) is sound.
Is there anything I am overlooking here? I.e. are there any examples of code where static promotion was performed on values that actually contained (or could not be proven not to contain) a cell?

Edit: Alternatively, my question would be this: Is "if &X is a const, then X: Freeze" correct, and can this be formalized as a behavior in the compiler without needing bounds somehow?

[RalfJung]

Here, Lhs::VTABLE is a const of type &T, which requires and therefore can be said to imply T: Freeze

No, it's not that simple. We don't know how the const was defined, and it could have used all sorts of terrible unsafe code.

For instance:

const C: &Cell<i32> = unsafe { std::mem::transmute(&0) };

This currently gets rejected, but we could conceivably allow this one day. So we shouldn't rely on this always being a hard error.

I don't know if there is a counterexample to this implicit bound, but I don't think it matters. Implicit bounds are very fragile, Rust has an entire line of soundness bugs caused by such assumptions elsewhere in the compiler. So I'm not willing to rely on implicit bounds for this. The bound needs to be made explicit.

(And that is leaving aside the fact that I have no idea how one could possibly implement the analysis based on the implicit bound you suggested. Usually we can't just feed assumptions into the trait system like that, I think.)

[Kyykek]

Here, Lhs::VTABLE is a const of type &T, which requires and therefore can be said to imply T: Freeze

No, it's not that simple. We don't know how the const was defined, and it could have used all sorts of terrible unsafe code.

For instance:

const C: &Cell = unsafe { std::mem::transmute(&0) };

This currently gets rejected, but we could conceivably allow this one day. So we shouldn't rely on this always being a hard error.

I don't know if there is a counterexample to this implicit bound, but I don't think it matters. Implicit bounds are very fragile, Rust has an entire line of soundness bugs caused by such assumptions elsewhere in the compiler. So I'm not willing to rely on implicit bounds for this. The bound needs to be made explicit.

Hm, yeah the part about avoiding implicit bounds is understandable. I also forgot about the work towards allowing statics to be referenced in consts, so a reference to a static containing interior mutability might exist in a const in the future (i think).
I'm curious as to why this seems to never cause a problem in 1.77 though, when playing around with it, the implcit bound I described was just what I felt like was happening, since the compiler always just seemed to know that anything I dereferenced from a const would not have interior mutability.