Add trait_upcasting related languages changes

[WaffleLapkin]

I applied review comments from #1259 and added updates from rust-lang/rust#120248.

Tracking issue: rust-lang/rust#65991
Stabilization proposal: rust-lang/rust#134367

cc @crlf0710 @compiler-errors @RalfJung

[WaffleLapkin]

rust-lang/rust#101336 is still open, did we actually end up committing to something?

[compiler-errors]

@WaffleLapkin: Yes, it's just pending documentation rust-lang/rust#101336 (comment) -- presumably this is (A.) on Niko's list.

[compiler-errors]

You may also want to edit https://doc.rust-lang.org/reference/behavior-considered-undefined.html as Niko mentioned in the issue you linked; you may want to re-read the FCP, but I believe it's UB to conjure up a vtable now since it must be valid for upcasting.

[RalfJung]

You may also want to edit https://doc.rust-lang.org/reference/behavior-considered-undefined.html as Niko mentioned in the issue you linked; you may want to re-read the FCP, but I believe it's UB to conjure up a vtable now since it must be valid for upcasting.

What exactly are you referring to here? I assume the "issue" is rust-lang/rust#101336 (a link would have been good, it took clicking on 4 links to track this down :D ). Niko says a lot of things there, though. :)

[WaffleLapkin]

@compiler-errors doc.rust-lang.org/reference/behavior-considered-undefined.html already mentions invalid metadata in any kind of pointer makes an invalid value, so I don't think we need to add anything on top of that?

• Invalid metadata in a wide reference, Box<T>, or raw pointer. The requirement for the metadata is determined by the type of the unsized tail:
  • dyn Trait metadata is invalid if it is not a pointer to a vtable for Trait.
  • Slice ([T]) metadata is invalid if the length is not a valid usize (i.e., it must not be read from uninitialized memory). Furthermore, for wide references and Box<T>, slice metadata is invalid if it makes the total size of the pointed-to value bigger than isize::MAX.

[compiler-errors]

I believe this is ready and consistent with what is implemented on master. I think all that needs to be done w.r.t. trait upcasting is a fresh stabilization PR?

[PoignardAzur]

Any movement on this?

[WaffleLapkin]

@PoignardAzur I'm working on a stabilization PR (after which this can be merged)

[WaffleLapkin]

@compiler-errors could you take another look, do the changes still make sense?

[WaffleLapkin]

@rustbot ready

[traviscross]

In the original PR for this...

• Add trait_upcasting related languages changes #1259

...there's a discussion about adding a line to the behavior considered undefined of:

• Performing non-nop coercion on a dangling or unaligned raw pointer.

There was some back and forth and it's not clear where that landed. Thoughts on that?

@compiler-errors @RalfJung @WaffleLapkin

[compiler-errors]

The comment here is the answer https://github.com/rust-lang/reference/pull/1259/files#r1404804424. I don't think this needs further changing. It doesn't really matter if the pointer is bad; upcasting only cares about the vtable metadata.

[nikomatsakis]

@compiler-errors doc.rust-lang.org/reference/behavior-considered-undefined.html already mentions invalid metadata in any kind of pointer makes an invalid value, so I don't think we need to add anything on top of that?

• Invalid metadata in a wide reference, Box<T>, or raw pointer. The requirement for the metadata is determined by the type of the unsized tail:

  • dyn Trait metadata is invalid if it is not a pointer to a vtable for Trait.
  • Slice ([T]) metadata is invalid if the length is not a valid usize (i.e., it must not be read from uninitialized memory). Furthermore, for wide references and Box<T>, slice metadata is invalid if it makes the total size of the pointed-to value bigger than isize::MAX.

@WaffleLapkin I think this isn't quite right. How I read the Behaviors Considered Undefined section, it says...

Producing an invalid value. “Producing” a value happens any time a value is assigned to or read from a place, passed to a function/primitive operation or returned from a function/primitive operation.

where "producing an invalid value is hence immediate UB". But the consensus from rust-lang/rust#101336 was that having an invalid vtable is not a failure of the validity invariant. Rather, the UB occurs when a dyn pointer (raw, reference, whatever) with a malformed vtable is upcast or has a method invoked on it. Because safe code can perform those operations (including on raw pointers, in the case of upcast and potentially method calls in the future), this implies that the safety invariant (the conditions required to release such a value to safe code) requires a valid vtable. But if you can keep the *const dyn Foo confined to unsafe code and be absolutely sure nobody upcasts it, it doesn't require a valid vtable. Quoting rust-lang/rust#101336:

The proposal is as follows:

• Vtable-adjusting upcasts are safe operations. The upcast is UB if performed on a value without a valid vtable
• As such, the "safety invariant" requires a fully valid vtable.
• The "validity invariant" requires *dyn metadata to be word-aligned and non-null.

Vtable-adjusting upcasts are defined as:

• Trait upcasts that alter the set of methods that can be invoked on the resulting value at runtime (e.g., dyn Bar to dyn Foo from the introduction). In particular, upcasts that simply add or remove auto-traits are not vtable-adjusting (e.g., dyn Debug + Send to dyn Debug).

[RalfJung]

But the consensus from rust-lang/rust#101336 was that having an invalid vtable is not a failure of the validity invariant.

To be clear, consensus was that we are not ruling on the validity invariant at this point. The validity invariant for vtable metadata is an open question, discussed at rust-lang/unsafe-code-guidelines#516. Meanwhile, the reference says that the vtable pointer must point to a vtable of the right trait and Miri enforces this; this is following our usual strategy of having a bit more UB for now and possibly relaxing this in the future.

[nikomatsakis]

Hmm, OK, that's not what I thought the consensus was. =) Maybe I'm misremembering, though when I read the text I quoted it seems clear-ish. We need to find some clearer way though to describe the "upper/lower bounds" of what is and is not UB, I think it's going to be very hard to keep things in sync over time the way it is now (where it is e.g. difficult to tell if the reference is out of date or over approximating).

…

On Thu, Dec 19, 2024, at 1:51 AM, Ralf Jung wrote: > But the consensus from rust-lang/rust#101336 <rust-lang/rust#101336> was that having an invalid vtable is not a failure of the validity invariant. > To be clear, consensus was that we are not ruling on the validity invariant at this point. The validity invariant for vtable metadata is an open question, discussed at rust-lang/unsafe-code-guidelines#516 <rust-lang/unsafe-code-guidelines#516>. Meanwhile, the reference says that the vtable pointer must point to a vtable of the right trait and Miri enforces this; this is following our usual strategy of having a bit more UB for now and possibly relaxing this in the future. — Reply to this email directly, view it on GitHub <#1622 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABF4ZX7ZXAI3MZZOHKHJNL2GJUI7AVCNFSM6AAAAABORRYCSKVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNJSHEYTOOBZGU>. You are receiving this because you commented.Message ID: ***@***.***>

[RalfJung]

We currently say this above the UB list

The following list is not exhaustive; it may grow or shrink.

So the entire thing should be considered as approximating. Maybe we should start slowly widdling this down to just a few items in the list, though I think we should always reserve the right to remove UB.

Anyway this is getting off-topic for this PR. The validity invariant for vtables doesn't have to be settled for this one.

[WaffleLapkin]

@rustbot label -I-lang-nominated

I've also prepared a refactor of the unsizing section: de3ffc3. I'll open a PR once this is merged.

[WaffleLapkin]

rust-lang/rust#134367 (comment) has been merged, so this can be merged as well :)

Dismissing stale review.

[traviscross]

I rebased to collapse some back and forth, to fix an author name, to make some minor stylistic tweaks (capitalization and line unwrapping), and to remove the note section mentioned above.

We're anticipating further work on this to address some things raised in the threads here in a follow-up PR that Waffle has in progress.

[WaffleLapkin]

Follow-up: #1731