Add trait_upcasting related languages changes

[WaffleLapkin]

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

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.

[traviscross]

Suggested change

	> Note: "unsizing" is a bit of a misnomer,
	> since this covers unsized->unsized coercions too.
	> Note: These are sometimes called "unsizing" coercions, but that can be a bit of a misnomer, since these cover unsized->unsized coercions too.

Not sure if something like this is what you meant to say or not. The Reference doesn't refer to "unsizing" anywhere -- even though, you're right, we often do in conversation -- so we'd need to introduce that somehow in this note.

[WaffleLapkin]

Hm. There may not be the literal word "unsizing", but we do refer to this coercion as "unsize coercion".

The following coercions are called unsized coercions, since they

I do feel like it's clear what this means, given that this note is surrounded by mentions of unsize coercions. (but maybe not see you seem to be confused?)

[WaffleLapkin]

Although I'd also say that replacing "unsized coercion" to "unsizing coercion" everywhere in the reference would be a good idea. It sounds better and more closely matches how people actually talk about this feature.

[ehuss]

In conversation with @traviscross, I was one of the confused ones. TC pointed out that "unsized coercion" can be interpreted as the codomain so to speak. That is, it doesn't seem like a misnomer to me because it is only referring to coercions to something not sized, and thus seemed accurate.

It is true the word "unsizing" implies "to remove the size of", that is the domain is sized, and thus is not always an accurate term. However, we don't use "unsizing" here.

Probably a bit of a muddled interpretation on my part, but does that make sense?

[RalfJung]

"unsized coercion" sounds like the coercion itself is unsized. I don't even know what that should mean.

[nikomatsakis]

Suggested change

	relate to converting types to unsized types, and are permitted in a few
	relate to converting sized types to unsized types or converting from one unsized type to another, and are permitted in a few

(more clarity, same meaning)

[WaffleLapkin]

For me your proposed change kinda flushes the cache, I had to re-read it 4 times before I figured out what it means 💀 (then again, maybe I shouldn't be reading it at 3am)

Since it's the same meaning I'd think that more concise is better? Do you disagree?

[nikomatsakis]

Does as not permit upcasting?

[WaffleLapkin]

as permits upcasting.

[scottmcm]

Is there worth saying anything here about whether this can change the value of the data pointer, or whether it can only change the vtable pointer?

(Maybe it's just a non-normative note, but contrasting with *const [i32] → *const [i8] that doesn't change the metadata -- even though people sometimes expect it to -- sounds potentially useful.)

[WaffleLapkin]

I think this whole section might need a retractor :p

Specifically I think it should more clearly state the difference between Unsize and CoerceUnsize, mention that the former is not actually a coercion, and the latter is only for pointers.

Then it would also make sense to add a note that unsizing coercion changes the metadata of a pointer, while all other coercions/casts don't.

But, if you don't mind I'll leave this as a follow up ^^'

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