Disqualify built-in trait impl if it seems likely to overlap in an unsound way with a blanket impl

[compiler-errors]

Hi T-types.

TL;DR: This PR fixes an unsoundness having to do with dyn Trait trait object built-in impls overlapping with user-written blanket impls by disqualifying the built-in impl when the trait has associated types and it fails a simple heuristic (based off of where clauses¹) to determine whether or not the built-in impl overlaps with the user-written impl.

What is the unsoundness

We currently do not detect when an object's built-in impl overlaps with a blanket impl for an unsized self type, like impl<T: ?Sized> Foo for T. The trait solver prefers object candidates over impl candidates because of traits like Any, which require dispatching to their virtual function to operate corerctly.

This preference can be side-stepped when we're in a generic context, and when the blanket impl differs from the built-in impl in an associated type, this can be used to unsoundly implement a transmutation function:

trait Object<U> {
    type Output;
}
impl<T: ?Sized, U> Object<U> for T {
    type Output = U;
}
fn foo<T: ?Sized, U>(x: <T as Object<U>>::Output) -> U {
    x
}

fn transmute<T, U>(x: T) -> U {
    foo::<dyn Object<U, Output = T>, U>(x)
}

In this case, the user-written Object<U, Output = U> impl overlaps with the built-in Object<U, Output = T> from the dyn.

How does this PR fix this?

We implement a heuristic query called trait_has_impl_which_may_shadow_dyn that takes a trait def id and the principal def id of a dyn type. It only considers traits with associated types, since those are the only traits that are inherently unsound with overlapping impls.

For the given trait def id, we check all of its impls:

• We use deep-reject to unify the self type. If it fails, then we disqualify the impl.
• If it has a Sized predicate that shares the same self type as the impl, then it is disqualified.
• For each other trait predicate that shares the self type, we recurse, checking if that trait may apply to the dyn.
  • If we find no impl, then the impl is disqualified.
• If the impl remains, then we consider the impl to be shadowed and return true.

If this heuristic returns true, then it will disqualify the built-in dyn impl so we always prefer the user-written impl. This check must be complete, i.e. it may never return false if an impl may apply, since we will then fail to disqualify an overlapping impl which gives us a way to side-step this check. However, it doesn't need to be sound i.e. it may return false-positives and may consider more impls to overlap than necessary. This will result in trait errors, but this does not happen in practice according to the crater run results.

Regressions:

transmutter-0.1.0 - This is just an example of the unsoundness in question.
hooks-yew-1.0.0-alpha.2 - This is a legitimate regression. lcnr minimized it down to:

trait Trait<T> {
    fn method(&self);
}
trait Erased<T> {
    type Assoc;
    fn erased(&self);
}
impl<T> Trait<T> for dyn Erased<T, Assoc = ()> {
    fn method(&self) {
        self.erased();
    }
}
impl<T, H: Trait<T> + ?Sized> Erased<T> for H {
    type Assoc = ();
    fn erased(&self) {
        self.method();
    }
}

fn break_me(hr: Box<dyn for<'a> Erased<&'a (), Assoc = ()>>) {
    hr.erased();
}

Which fails because our dyn overlap heuristic is not smart enough, since it doesn't consider higher-ranked placeholder errors when unifying the impl signature...

Alternatives

Always prefer the blanket impl

This would technically be the most sound option, since we could just always shadow the built-in object impl by a blanket if it applies.

However, this is likely unworkable, because although we can use some sort of intrinsic or built-in compiler hack to fix the behavior of Any, I don't believe that we could apply this hack to user-built analogues of Any or other traits that generally prefer to dispatch to the built-in object impl over the blanket, and so this would a breaking change in general.

This PR compromises by only shadowing the built-in impl when it has associated types, as it would be unsound in these cases if the associated type differed between the impls.

Always consider the impl as being "specialized" by the built-in object impl

This means that any blanket impl that overlaps with a built-in object impl is considered to be "further specializable", and it would mean that we cannot project an associated type in an example like:

trait Mirror {
    type Assoc;
}

impl<T: ?Sized> Mirror for T {
    type Assoc;
}

fn foo<T: ?Sized>(x: &T) -> &<T as Mirror>::Assoc { x }

since that <T as Mirror>::Assoc type may return a different associated type for the "specializing" built-in dyn impl.

Footnotes

1. More about this below. ↩

[compiler-errors]

@bors try

[bors]

⌛ Trying commit 3ee780b with merge 264a7c4...

[bors]

☀️ Try build successful - checks-actions
Build commit: 264a7c4 (264a7c4a1c917c83d13895a31231f9d93c7b67e1)

[compiler-errors]

@craterbot check

[craterbot]

👌 Experiment pr-132289 created and queued.
🤖 Automatically detected try build 264a7c4
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-132289 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[lcnr]

we may also want to emit a warn by default lint for all impls which disable the builtin object impl 🤔 even if that lint will mostly be unactionable 😅 potentially even regardless of whether we actually disable the impl because the trait has an associated type as adding such an impl will mean that adding an associated type will cause your trait to not be object safe anymore which is likely surprising

let's first see the crater run though

[craterbot]

🎉 Experiment pr-132289 is completed!
📊 22 regressed and 1 fixed (531984 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[lcnr]

crater analysis https://hackmd.io/burOEs9MQdelPgk4frE76g

[compiler-errors]

@bors try

[compiler-errors]

for later:

craterbot check p=1 crates=https://crater-reports.s3.amazonaws.com/pr-132289/retry-regressed-list.txt

[bors]

⌛ Trying commit bb80bba with merge 1a961a0...

[compiler-errors]

@bors try

[bors]

⌛ Trying commit 264f242 with merge 4ce0d30...

[lcnr]

@rfcbot fcp merge

see the PR description + #132289 (review)

[rfcbot]

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

• @BoxyUwU
• @compiler-errors
• @jackh726
• @lcnr
• @nikomatsakis
• @oli-obk
• @spastorino

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!

See this document for info about what commands tagged team members can give me.

[jackh726]

we may also want to emit a warn by default lint for all impls which disable the builtin object impl 🤔 even if that lint will mostly be unactionable 😅 potentially even regardless of whether we actually disable the impl because the trait has an associated type as adding such an impl will mean that adding an associated type will cause your trait to not be object safe anymore which is likely surprising

I'm really curious of the fallout of this is (i.e. how widespread is this pattern). There are effectively four levels of strictness here:

1. Error on all impls that may overlap with the builtin impl, regardless of it they have an associated type
2. Error on all impls that may overlap with the builtin impl and have an associated type, whether or not that associated type can overlap the builtin associated type
3. Error on all impls that may overlap with the builtin impl and have an associated type that can overlap with the builtin associated type
4. Error on all impls that may overlap with the builtin impl and have an associated type that does overlap with the builtin associated type.

I'm not sure if there's a meaningful difference between 3 and 4, but there may be in what we can prove.

I imagine 1 and 2 are probably non-starters in terms of breakage, but 3 or 4 may be worth doing at the coherence level. This is instead of the PR as-is, which allows overlapping impls but disables the builtin impl.

Of course, 3 and 4 get harder to implement when you consider GATs (which aren't yet dyn compatible but will be), but maybe not unapproachably harder.

I think this PR is compatible with later switching to one of the 4 sets of rules above, since those are I think more strict than the rules in this PR. So, I think it's probably worth landing this, since is better in terms of soundness.

However, I do think that my comments regarding user type annotations being effectively ignored should be addressed. It isn't critical for correctness, so maybe not worth blocking this PR on, but imo does provide a bad user experience so it's something I would really like to see before the PR lands.

[jackh726]

This is the "simple case" that results in a compile fail - I don't see a similarly simple case that results in a compile pass.

[jackh726]

This is also really confusing to the user:
The Output = T that user writes is basically ignored here. Not only does this result in an unexpected error message ("I said the output is T, I gave it a T"), but it more fundamentally it means that the user can write a bound that is neither checked nor used.

I would want to error on the type annotation dyn Object<U, Output = T> since that is really where the "problem" originates. Naively, I guess you could take dyn Object<U>, search for impls with that self type (finding the built-in impl and the user-written impl), then check if the associated type unifys with all of them.

[compiler-errors]

Not only does this result in an unexpected error message ("I said the output is T, I gave it a T"), but it more fundamentally it means that the user can write a bound that is neither checked nor used.

This is not new. If I write a dyn Trait<Type = Whatever> where Whatever doesn't satisfy the item bounds of Trait::Type, then I already have a bound that isn't being enforced but is also not being used.

I would want to error on the type annotation dyn Object<U, Output = T> since that is really where the "problem" originates.

I don't believe such a check can be implemented, since the well-formedness of an object type does not rely on the object implementing that trait, which is what is this PR adjusts the behavior of.

[jackh726]

I can't comment below, but I was at first extremely surprised that we don't error on the bug::<T>() call - bug<T> requires T: Sized, but in bug_run, we don't know in theory. Looking closer, I guess it's implied by <T as MyTrait>::Target: Sized normalizing to T: Sized, implying T: Sized. Now that we disqualify the builtin impl, this implication should just always be true, right?

Personally, even with the above logic, erroring on line 21 feels right (something about deriving an implication by using a blanket impl to normalize seems off, even though it's sound). But, that seems like a completely separate discussion.

For this error, I think my comment above still stands: the user being able to write Target = u8 here and it basically get ignored is very confusing.

[compiler-errors]

Now that we disqualify the builtin impl, this implication should just always be true, right?

Correct.

erroring on line 21 feels right

In another Rust, maybe, but I don't believe it's feasible to fix this now. Making blanket impls not apply generically causes tons of breakage.

[compiler-errors]

I'm not sure if there's a meaningful difference between 3 and 4, but there may be in what we can prove.

I imagine 1 and 2 are probably non-starters in terms of breakage, but 3 or 4 may be worth doing at the coherence level. This is instead of the PR as-is, which allows overlapping impls but disables the builtin impl.

@jackh726: I'm not certain what you're suggesting to do at the coherence level mechanically. The problem here is that we don't know that the built-in object impl is going to be until we actually have a dyn Trait type in our hands, and unlike coherence, the list of built-in dyn impls is not exhaustive and we don't know them from the AST of the program.

[compiler-errors]

However, I do think that my comments regarding user type annotations being effectively ignored should be addressed. It isn't critical for correctness, so maybe not worth blocking this PR on, but imo does provide a bad user experience so it's something I would really like to see before the PR lands.

Honestly, I have no idea how to begin to improve the diagnostics for the class of errors that can fall out w when a built-in trait object gets disqualified in favor of a blanket impl which overlaps it. Given that the real world fallout is zero crates, I don't expect this to be very high-priority to fix.

[steffahn]

This PR introduces a soundness regression:

type A = &'static [usize; 1];
type B = &'static [usize; 100];

type DynSomething = dyn Something<Assoc = A>;

trait Super {
    type Assoc;
}
impl Super for Foo {
    type Assoc = A;
}

trait IsDynSomething {}
impl IsDynSomething for DynSomething {}

impl<T: ?Sized> Super for T
where
    T: IsDynSomething,
{
    type Assoc = B;
}

trait Something: Super {
    fn method(&self) -> Self::Assoc;
}

struct Foo;
impl Something for Foo {
    fn method(&self) -> Self::Assoc {
        &[1337]
    }
}

fn main() {
    let x = &Foo;
    let y: &DynSomething = x;

    let short_arr: A = x.method();
    let long_arr: B = y.method();
    println!("{:?}", short_arr);
    println!("{:?}", long_arr);
}

fails on stable/nightly, UB on this branch

[compiler-errors]

of fucking course it does lmao

[compiler-errors]

Pretty sure this is just @lcnr's:

disable both Trait and Super for dyn Trait: Super and there is impl<T: Trait> Super for T? we have to disable both Trait and Super here, don't we?

[steffahn]

I don't know, I stopped trying to understand the description and discussion here, and instead just downloaded the branch and played with it until it broke 😇

[compiler-errors]

@steffahn: Ya im just talking to myself (and lcnr maybe)

[steffahn]

Here's another problematic case I could find. It's a 2-crates setup:

First crate (e.g. your lib.rs):

pub type A = &'static [usize; 1];
pub type B = &'static [usize; 100];

pub trait Trait<P> {
    type Assoc;
}

pub type Dyn<P> = dyn Trait<P, Assoc = A>;

pub trait LocallyUnimplemented<P> {}

impl<P, T: ?Sized> Trait<P> for T
where
    T: LocallyUnimplemented<P>,
{
    type Assoc = B;
}

pub fn function<P>() -> <Dyn<P> as Trait<P>>::Assoc {
    &[1337]
}

Second crate (e.g. your main.rs):

use name_of_crate::{function, Dyn, LocallyUnimplemented};

struct Param;

impl LocallyUnimplemented<Param> for Dyn<Param> {}

fn main() {
    let arr: &[usize; 100] = function::<Param>();
    println!("{:?}", arr);
}

example output on this branch:

[1337, 9223371974577750009, 9223361443317940208, 7598807758576447331, 8387236760591036015, 72340172838076673, 72340172838076673, 8028075845441778529, 2338606692308099182, 8295751895439843369, 6926649950808009068, 7935476084319219301, 7236850772546188399, 8385534785046799406, 8031998611105869945, 8245656750015082344, 7308324465818165605, 7235433442201987451, 9041083404643824416, 18445953872741353664, 18445908449167217680, 2334106421097295465, 7308604897068083558, 7954799936718922098, 2339731488442490980, 506381209866536711, 506381209866536711, 1, 2, 1, 0, 2314885530818453536, 2338600897117954080, 651061555542690057, 651061555542690057, 7881131626781303854, 8031998611039609697, 7598543892949132624, 7234304299306872431, 7309993588911404115, 8245935277856024934, 578721382704613384, 578721382704613384, 7021797314962419566, 7305790156120810855, 0, 0, 4716517092481066049, 7308324465885667702, 2332992598738301472, 2322278944636232038, 361700864190383365, 361700864190383365, 3978425819141910832, 7378413942531504440, 49087, 0, 9223372039002259456, 9223372039002259456, 1, 1, 479142256574474, 479520213808129, 479545983611853, 499977143039031, 502300720351447, 502300720351447, 502300720351447, 500269200819878, 500531193825495, 499951373240094, 502300720350900, 502300720351447, 502300720351054, 502300720350990, 504538398312238, 504693017136191, 506316514774122, 504538398313184, 506045931834080, 504538398313220, 504538398313184, 504538398313184, 504538398313184, 504538398313184, 504538398313184, 504538398313184, 504538398313184, 504538398313184, 504538398313184, 504538398313184, 504693017136191, 504693017135876, 504693017135876, 506230615427844, 504693017135840, 504693017135876, 504538398313220, 511964396767968, 510783280763022]

error on stable/nightly

error[E0308]: mismatched types
 --> src/main.rs:8:30
  |
8 |     let arr: &[usize; 100] = function::<Param>();
  |              -------------   ^^^^^^^^^^^^^^^^^^^ expected an array with a fixed size of 100 elements, found one with 1 element
  |              |
  |              expected due to this

[steffahn]

Here's another interesting case, with (up to) 3 crates:

code

(uses the standard library as the first crate:)

pub trait Index<Idx: ?Sized> {
    type Output: ?Sized;
    fn index(&self, index: Idx) -> &Self::Output; // <- the method is actually irrelevant for this issue
}

(any object-safe trait with a type parameter and associated type works)

second crate (e.g. your lib.rs):

use std::ops::Index;

pub trait Trait {
    fn f(&self)
    where
        dyn Index<(), Output = ()>: Index<()>;
    //  rustc (correctly) determines ^^^^^^^^ this bound to be true
}

pub fn call(x: &dyn Trait) {
    x.f(); // so we can call `f`
}

third crate (e.g. your main.rs):

use std::ops::Index;

use name_of_crate::{call, Trait};

trait SubIndex<I>: Index<I> {}

struct Param;

trait Project {
    type Ty: ?Sized;
}
impl Project for () {
    type Ty = dyn SubIndex<Param, Output = ()>;
}

impl Index<Param> for <() as Project>::Ty {
    type Output = ();

    fn index(&self, _: Param) -> &() {
        &()
    }
}

struct Struct;

impl Trait for Struct {
    fn f(&self)
    where
        for<'a> dyn Index<(), Output = ()>: Index<()>,
        // now rustc believes that this bound ^^^^^^^ is false
    {
        println!("hello!");
    }
}

fn main() {
    call(&Struct); // <- hence, the method `f` isn't part of the vtable

    // ^^^ segfaults (calls null function pointer from vtable)
}

remark

I'm saying "up to 3" because above, the first two crates can be the same.

The 3-crate layout does however better clarify the separation of concerns. In particular, one could say: “the Index trait isn't doing anything suspicious”.

On stable rustc, this code runs fine and prints hello.

[compiler-errors]

Yeah, I dont want to work on this anymore. It's a terrible approach and it's not making any progress in FCP anyways. A more feasible strategy at the very least needs to more accurately model the logic of "is this trait implemented via an impl" but likely should use the preexisting machinery we have today.

[jackh726]

@steffahn can you open a PR to add those examples as tests?

[steffahn]

yes, I will do that