Stabilize feature(trait_upcasting)

[WaffleLapkin]

This feature was "done" for a while now, I think it's finally time to stabilize it! (stabilization report pending)
cc reference PR: rust-lang/reference#1622, tracking issue: #65991

r? compiler-errors

[rustbot]

Some changes occurred in tests/ui/sanitizer

cc @rust-lang/project-exploit-mitigations, @rcvalle

[compiler-errors]

Could you please provide some links that specify what has been done since the last stabilization? Ideally both the issues and the resulting fix PRs.

Ty for doing this tho :)

Gna nominate for T-lang but this should be an easy decision.

[compiler-errors]

And maybe link to the last stabilization too

[rustbot]

The Miri subtree was changed

cc @rust-lang/miri

[WaffleLapkin]

Stabilization report

The core of the feature hasn't changed since the last attempt to stabilize it.

But as a reminder this feature allows "upcasting" dyn Sub to dyn Super, such as in this example:

trait Sub: Super {}
trait Super {}

fn upcast(x: &dyn Sub) -> &dyn Super {
    x // implicit coercion
}

This is a long wanted feature that people used workarounds for for a long while now.

One possible downside is that this forces us into including more data in the vtables. However, our measurements show that the overhead is mostly negligible. Also note that we ate already including this overhead on stable for countless versions and no one ever complained.

Another possible downside is that this feature this allows upcasting of raw trait pointers in safe code. That puts constraints on their library invariant (safety invariant) -- specifically, even *const dyn Trait must always come with a vtable that is valid for Trait. This was also discussed during previous stabilization attempts, but still, something to keep in mind.

I believe that the feature is well tested and is ready for stabilization.

Previous stabilization attempt problems

After the last attempt to stabilize this feature @steffahn found two unsound interactions between trait upcasting and pointer casting (one of which also required feature(arbitrary_self_types)): #120222 and #120217. This caused a revert of the stabilization PR.

Both issues were since fixed in #120248 by adding additional checks for casting pointers, to uphold the library invariant of pointers to trait objects which is needed for this feature.

No new issues were found since.

[WaffleLapkin]

(@RalfJung I'm pretty sure I-lang-easy-decision Issue: The decision needed by the team is conjectured to be easy; this does not imply nomination implies nomination and you don't need I-lang-nominated Nominated for discussion during a lang team meeting. )

[RalfJung]

@traviscross told me the opposite -- it still needs to be nominated with the regular label.

The label description should probably clarify or else people will inevitably keep getting this wrong.

[RalfJung]

The only possible downside

That's a very strong statement.^^

I'd say there is at least one other downside, or point worth mentioning: this allows upcasting of raw trait pointers in safe code. That puts constraints on their library invariant (safety invariant) -- specifically, even *const dyn Trait must always come with a vtable that is valid for Trait.

I'm fine with that, and AFAIK @rust-lang/types agrees, but it is a choice we are making here that should be called out explicitly. @rust-lang/opsem is still discussing what to do with the language invariant for raw trait pointers; having that be different from the library invariant is likely surprising but OTOH we still might want a weaker invariant here.

[WaffleLapkin]

@RalfJung I did call this out, but maybe not explicitly enough ("the only" slipped from an earlier draft). I've updated the wording to more clearly highlight this.

[nikomatsakis]

I'd say there is at least one other downside, or point worth mentioning: this allows upcasting of raw trait pointers in safe code. That puts constraints on their library invariant (safety invariant) -- specifically, even *const dyn Trait must always come with a vtable that is valid for Trait.

I'm fine with that, and AFAIK @rust-lang/types agrees, but it is a choice we are making here that should be called out explicitly. @rust-lang/opsem is still discussing what to do with the language invariant for raw trait pointers; having that be different from the library invariant is likely surprising but OTOH we still might want a weaker invariant here.

worth noting that we did formally decide this here: #101336

[nikomatsakis]

@rfcbot fcp merge

I'm so excited for this. It's going to make Salsa 3.0 work a lot better (along with a bunch of other code...).

Stabilization report

[rfcbot]

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[traviscross]

@rfcbot reviewed

[scottmcm]

I'm in favour here.

One thing I wanted to double-check: how are these encoded in MIR? Are they https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/adjustment/enum.PointerCoercion.html#variant.Unsize or https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/enum.CastKind.html#variant.PtrToPtr? I'm hoping it's the former, as implied by the "convert between wide pointers" doc.

(Asking because if it's the latter I'm worried the mir-opts might be unsound in conjunction with this.)

[compiler-errors]

@scottmcm:

fn foo(_1: &dyn Sub) -> &dyn Sup {
    debug x => _1;
    let mut _0: &dyn Sup;

    bb0: {
        _0 = copy _1 as &dyn Sup (PointerCoercion(Unsize, Implicit));
        return;
    }
}

They are a form of unsizing in both HIR and MIR.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[traviscross]

@rustbot labels -I-lang-nominated

We discussed this on the lang call today, and it's now in FCP.

[WaffleLapkin]

I'm not actually sure what to do with this lint. "Future incompatible" doesn't make sense anymore, since this PR is actually adding the breaking change... should I make it into a normal lint? Should I make it deny-by-default?

[jackh726]

It might be time to do a crater run for switching it to deny-by-default.

The conservative case is to keep it warn and switch it later - I think that's okay.

[jackh726]

Particularly, I could very much imagine people having these impls around today precisely because trait upcasting isn't stable. And so switching it later might be much easier.

[RalfJung]

"Future incompatible" doesn't make sense anymore, since this PR is actually adding the breaking change

So we can't keep this a lint and proceed with compilation when it triggers?

[RalfJung]

Oh wait this is a FutureReleaseSemanticsChange, I've never seen one of those... I see now. Yeah probably the lint should just be removed, but Cc @rust-lang/lang to be sure. This is about #89460.

[WaffleLapkin]

I think disallowing the impl is silly, it can be useful, for example in combination with genetic code. I'm fine with making this a normal lint or removing the lint altogether.

[WaffleLapkin]

Should I nominate for T-lang discussion again, since there doesn't seem to be a consensus of what to do?

[jackh726]

Yes, I think that's good

[lukas-code]

FWIW we currently do not warn for Deref impls that are shadowed by other kinds of coercions, for example:

array->slice unsizing example

use std::ops::Deref;

struct Wrap<T: ?Sized>(T);

impl Deref for Wrap<[u8; 0]> {
    type Target = Wrap<[u8]>;

    fn deref(&self) -> &Self::Target {
        println!("deref!");
        self
    }
}

fn main() {
    let _: &Wrap<[u8]> = &Wrap([]); // doesn't print "deref!"
    let _: &Wrap<[u8]> = Deref::deref(&Wrap([])); // prints "deref!"
}

subtyping example

use std::ops::Deref;

struct Wrap<T: ?Sized>(T);

impl Deref for Wrap<fn(&())> {
    type Target = Wrap<fn(&'static ())>;

    fn deref(&self) -> &Self::Target {
        println!("deref!");
        self
    }
}

fn main() {
    let _: &Wrap<fn(&'static ())> = &Wrap((|_| ()) as fn(&())); // doesn't print "deref!"
    let _: &Wrap<fn(&'static ())> = Deref::deref(&Wrap((|_| ()) as fn(&()))); // prints "deref!"
}

So IMO if we keep this lint, it should apply to all Deref impls that are shadowed by any kind of coercion and not be arbitrarily restricted to the specific Deref impls that will no longer be called after this PR merges.

[WaffleLapkin]

If we go that round I'd prefer to remove the lint & leave implementing the full shadowed_deref lint as a followup.

[tmandry]

@rfcbot reviewed

(Already checked my box, but rfcbot doesn't seem to register this.)

[WaffleLapkin]

I'm re-nominating for lang discussion, specifically to ask what to do with the deref_into_dyn_supertrait lint. As it is right now, it is a FCW lint that warns against impls of Deref for dyn Trait which will be shadowed by the trait upcasting (after this PR). I.e.

#![deny(deref_into_dyn_supertrait)]
#![allow(dead_code)]

use core::ops::Deref;

trait A {}
trait B: A {}
impl<'a> Deref for dyn 'a + B {
    type Target = dyn A;
    fn deref(&self) -> &Self::Target {
        todo!()
    }
}

fn take_a(_: &dyn A) {}

fn take_b(b: &dyn B) {
    take_a(b);
}

error: this `Deref` implementation is covered by an implicit supertrait coercion
 --> src/lib.rs:8:1
  |
8 | impl<'a> Deref for dyn 'a + B {
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ `dyn B` implements `Deref<Target = dyn A>` which conflicts with supertrait `A`
9 |     type Target = dyn A;
  |     -------------------- target type is a supertrait of `dyn B`
  |
  = warning: this will change its meaning in a future release!
  = note: for more information, see issue #89460 <https://github.com/rust-lang/rust/issues/89460>

The impl is not harmful, but is mostly useless, since it will no be called in coercions (after this PR). It might in some cases be useful, if generic code expects T: Deref, although I'm doubting this actually happens. Another use case might be supporting old(er) compilers.

Either way the question is: what to do with the lint? The options are:

1. Completely disallow such impls, since they are functionally useless. This would be a breaking change both today and after stabilization.
2. Always warn (or deny) if we see a shadowed impl, since they are useless but not actively harmful.
   • i.e. turn this lint from an FCW to a normal warning
3. Just ignore impls, without warning. (Or keep an allow by default lint.)

I'm strongly in favor of options 2 and 3, since I do not see a reason to forbid the impl -- it is not harmful (mostly just silly/useless). I'd probably keep the lint as a normal warning (warn by default), to highlight that it's shadowed by built in unsizing most of the time.

Some discussion may be found in this thread: #134367 (comment).

[traviscross]

Some discussion may be found in this thread: #134367 (comment).

Particularly interesting from this discussion is the observation by @lukas-code that we allow and do not lint currently when Deref impls are (partially) shadowed by other similar coercions. E.g.:

#![feature(trait_upcasting)]

use core::ops::Deref;

struct W<T: ?Sized>(T);

impl Deref for W<[u8; 0]> {
    type Target = W<[u8]>;
    fn deref(&self) -> &Self::Target {
        panic!()
    }
}

trait Super {}
trait Sub: Super {}
impl<'a> Deref for W<dyn 'a + Sub> {
    type Target = W<dyn Super>;
    fn deref(&self) -> &Self::Target {
        panic!()
    }
}
impl Super for () {}
impl Sub for () {}

fn main() {
    // Existing:
    let _: &W<[u8]> = &*&W([]); // No panic.
    let _: &W<[u8]> = &**&W([]); // Panic.
    // New:
    let _: &W<dyn Super> = &*(&W(()) as &W<dyn Sub>); // No panic.
    let _: &W<dyn Super> = &**(&W(()) as &W<dyn Sub>); // Panic.
}

[Veykril]

Dropping by to just say that rust-analyzer currently throws a type mismatch on trait upcasting coercions as the feature isn't implemented there yet rust-lang/rust-analyzer#18083