Trait objects can call nonexistent concrete methods

[matthewjasper]

This code segfaults at runtime

use std::{
    fmt::{self, Debug},
};

trait X {
    fn foo(&self) where Self: Debug;
}

impl X for fn(&()) {
    fn foo(&self) where fn(&()): Debug {}
}

impl Debug for dyn X {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        write!(f, "hi!")
    }
}

pub fn main() {
    let x: fn(&()) = |_| ();
    let y = &x as &dyn X;
    y.foo(); // Segfault at opt-level 0, SIGILL otherwise.
}

cc #48557 - Makes this easier to come across.

[leoyvens]

Edit 3: This really isn't the way to fix this issue, you may skip this comment.
Edit 2: I pursued this tentative fix in #50815 but it didn't actually work. Maybe this is not the way to fix it or deeper changes would be needed.

I did some investigation. The ideal fix is to catch this at declaration site, in the well-formedness (WF) check. Failing that we should be catching it at the call site, but that code dodged all checks.

Edit: Actually with #48214 in some cases that WF check is downgraded to a lint, so we must also get the call site check right to fix this for all cases.

The presence of a for<'a> fn(&'a ()), the desugaring of fn(&()), brings suspicion to uses of skip_binder() which would skip the binder for<'a> and turn 'a into an escaping region.

The WF check for the for<'a> fn(&'a ()): Debug trait predicate is reached by a call to check_where_clauses, then to ty::wf::predicate_obligations where we confidently skip the binder

rust/src/librustc/ty/wf.rs

Lines 75 to 78 in 2a3f536

	// (*) ok to skip binders, because wf code is prepared for it
	match *predicate {
	ty::Predicate::Trait(ref t) => {
	wf.compute_trait_ref(&t.skip_binder().trait_ref, Elaborate::None); // (*)

only to completely ignore any trait predicates containing an escaping region in compute_trait_ref, so the predicate is not checked at all for WF.

rust/src/librustc/ty/wf.rs

Lines 191 to 196 in 2a3f536

	self.out.extend(
	trait_ref.substs.types()
	.filter(|ty| !ty.has_escaping_regions())
	.map(|ty| traits::Obligation::new(cause.clone(),
	param_env,
	ty::Predicate::WellFormed(ty))));

I'm not sure how to fix but this certainly looks like guilty code.

[nikomatsakis]

OK, so, this first of all is not dependent on #48557. For example, this example will reproduce the problem on stable:

https://play.rust-lang.org/?gist=8c8b887b74da66782339e3a79c21c378&version=stable&mode=debug

The problem is rather a flaw in the object safety rules. Specifically, the object safety rules are supposed to prevent Self from appearing in the where clauses:

rust/src/librustc/traits/object_safety.rs

Lines 132 to 134 in dbd10f8

	if self.predicates_reference_self(trait_def_id, false) {
	violations.push(ObjectSafetyViolation::SupertraitSelf);
	}

But for some reason we seem to permit Self in the special case of Self: Trait:

rust/src/librustc/traits/object_safety.rs

Lines 167 to 170 in dbd10f8

	ty::Predicate::Trait(ref data) => {
	// In the case of a trait predicate, we can skip the "self" type.
	data.skip_binder().input_types().skip(1).any(|t| t.has_self_ty())
	}

I'm actually..not sure why we do that. It seems ok on the trait level, as that is validated at the impl site, but not at the method level.

[leoyvens]

Yes my previous comment was really misled, has nothing to do with HRTB or closures, see this minimization:

trait Trait {}

trait X {
    fn foo(&self) where Self: Trait;
}

impl X for () {
    fn foo(&self) {}
}

impl Trait for dyn X {}

pub fn main() {
    <X as X>::foo(&()); // Segfault at opt-level 0, SIGILL otherwise.
}

[nikomatsakis]

Marking as P-high since the fix may break code.

[nikomatsakis]

So I was discussing this problem with myself (I don't think @leodasvacas is around =) on Discord. You can search for this GUID to find it: 3b75dc47-42f1-44da-a39e-562f5875e932

However, the TL;DR is that we don't have to forbid all where clauses like Self: B where B is some bound. The key question is whether proving that dyn Trait: B implies that the hidden Self also satisfies B. This is true for lifetime bounds but also auto traits. It is not true for general traits (which can be implemented for a dyn Trait type but not necessarily other types).

So I think we should loosen the conditions in the PR, which should help with regressions.

[nikomatsakis]

OK, so I was maybe not completely right. Let me summarize what @leodasvacas and I said on Discord. We currently have two paths forward that permit us to close the soundness hole but not break as many crates.

---

Background: my summary from previous comment is still basically right. For a trait to be object safe, we need to ensure that — if there is a where-clause on a method like Self: B — then proving dyn Trait: B implies that Self: B where Self is the hidden type. This is true for lifetimes. It is not true for general traits. It turns out to be "almost true" for auto traits.

---

Proposal 1. Auto traits are special.

Currently, we do not permit external auto traits to be implemented for anything but structs/enums (example). We do, apparently, permit it for local auto traits. We also permit impls for types like &u32 (example).

I believe the reason for this disparity was that this would give the "auto trait definer" the chance to decide whether these "primitive impls" for built-in types were generated according to the default rules (recursively visit the data) or some other criteria, just as one can do for structs. However, auto traits are unstable, so these rules are subject to change.

The reasoning behind this prohibition is that — when we ask whether a dyn Trait type implements an auto trait — we always check the list of bounds to determine if that is true (so e.g. dyn (Trait + Send): Send but dyn Trait: Send does not). In other words, the compiler is "supplying" the full set of impls for dyn Trait types automatically.

That said, this isn't really a necessary restriction. I think the current implementation is also coherent (no pun intended) and makes a measure of sense. (Well, to be consistent, we probably could allow user-defined impls for dyn Trait where Trait is local.)

---

Interlude.

If you think about it, the heart of the previous proposal was basically banning the ability to implement traits for a dyn Trait. We already sort of have this rule. You cannot, for example, do:

impl Trait for dyn Trait { .. }

This makes sense, since the compiler is going to supply the dyn Trait: Trait impl via virtual dispach. If we were ever to extend to multiple bounds, I think it would also stand to reason that one could not impl Trait for any of the traits in the list:

impl Trait for dyn (Trait + Trait2) { .. }

(Weirdly, impl Bar for dyn (Foo + Bar) seems to .. kind of work today? It gives an error, but a strange one. We should probably change that.)

---

Proposal 2. Action at a distance

In a way, it'd be nice to prevent implementation traits for dyn types altogether — there is always a kind of "weird competition" between the compiler-provided magic and user-given impls. But that's not practical. Among other things, impl Debug for dyn Any is in the standard library. And it's only really needed for the cases where the dyn bounds imply the trait.

@leodasvacas pointed out that we could do a more narrow restriction though. You could say that, if you have trait Foo and it has where Self: Bar, then impl Bar for dyn Foo is illegal (but dyn Foo is object safe). It's kind of a strange "connection" between the traits though.

Among other things, this implies that things like AnyMap are fine.

[nikomatsakis]

Nominating for lang team discussion.

[leoyvens]

Fwiw the typemap crate, which was the major regression caused by the proposed patch, can be hacked into working with the rule that Self: Send makes the method not object-safe.

I think that proposal 2 would be best from a language point of view, it sounds like bad design in principle, but the problematic situations never happen in practice so it would be a an edge case fix for an edge case problem. However it may be impractical to implement, I don't think the compiler can readily tell us if an impl that looks like impl Bar for dyn Foo + .. exists or not.

[daboross]

Forgive me if I'm not fully understanding this issue, but would it be a good solution to have

fn foo(&self) where Self: Trait;

simply imply this?

fn foo(&self) where Self: Trait + Sized;

So methods marked with where Self: Debug or any other trait just wouldn't be callable on type-erased instances of the trait. This seems like it would be a sane solution that, if I understand correctly, wouldn't break existing code.

Being able to impl Trait for OtherTrait is a very useful feature, especially when dealing with Any and similar dynamic tricks. It would seem very strange to remove this for the sake of an edge-case in where in methods.

[lahwran]

I'm going through the I-unsound issues to review their current status and next steps. Looks like the current status of this issue is that the compiler emits a warning for the entire class of unsoundness, leaving no holes arising from this issue in a program that compiles without warning, and further progress is expected to be possible once chalk is ready. If I'm wrong that the warning covers all potential instances of this dyn missing method issue, I'd love to be corrected. In the meantime, without chalk, seems like consensus is that this isn't viable to properly fix in rustc.

[oli-obk]

trait Trait {}

trait X {
    fn foo(&self) where Self: Trait;
}

impl X for () {
    fn foo(&self) {}
}

impl Trait for dyn X {}

pub fn main() {
    <X as X>::foo(&()); // Segfault at opt-level 0, SIGILL otherwise.
}

still segfaults and miri says

error: Undefined Behavior: `dyn` call trying to call something that is not a method
  --> src/main.rs:14:5
   |
14 |     <dyn X as X>::foo(&()); // Segfault at opt-level 0, SIGILL otherwise.
   |     ^^^^^^^^^^^^^^^^^^^^^^ `dyn` call trying to call something that is not a method

[Jules-Bertholet]

There's one potential (partial) fix that I haven't seen discussed, that would continue to support many of the use-cases people have for the trait bounds in question. Notably, it would allow async-trait to keep working without the additional restrictions on auto-trait impls that #107082 imposed.

Let's look at the MCVE for this issue:

trait Trait {}

trait X {
    fn foo(&self) where Self: Trait;
}

impl X for () {
    fn foo(&self) {}
}

impl Trait for dyn X {}

pub fn main() {
    <dyn X as X>::foo(&()); // Segfault at opt-level 0, SIGILL otherwise.
}

The source of the problem is that there is no source for the implementation of <dyn X as X>::foo. Now, let's consider a very similar example:

trait Trait {}

trait X {
    // `foo` now has a default impl
    fn foo(&self) where Self: Trait {}
}

impl X for () {}

impl Trait for dyn X {}

pub fn main() {
    <dyn X as X>::foo(&());
}

In this modified example, there is a potential source for the <dyn X as X>::foo implementation: the default impl! And in fact, given the current state of stable Rust (#20021, no #![feature(trivial_bounds)]), the common case is likely to be that a default method impl exists. And the discussion in #51443 seems to confirm this. So, a potential fix to this issue would be to use the default impl to provide the dyn Trait impl whenever possible, and impose restrictions on object safety only when no default is present.

One wrinkle is that additional rules would be necessary to prevent lifetime-based specialization. Consider:

#![feature(trivial_bounds)]

trait Trait {}

impl Trait for &'static () {}

trait X {
    // `foo` now has a default impl
    fn foo(&self) where Self: Trait {}
}

impl<'a> X for &'a () {
    fn foo(&self) where Self: Trait {
        println!("`Self: 'static` must surely hold at this point");
    }
}

impl<'a> Trait for dyn X + 'a {}

pub fn test<'a>(r: &'a ()) {
    <dyn X + 'a as X>::foo(&r);
}

In this case, the implementation of <dyn X + 'a as X>::foo would have to be taken from the default even in the case of 'a: 'static. Taking it from the impl<'a> X for &'a () block would be unsound for 'a ≠ 'static, and choosing a different impl depending 'a would be unsound lifetime-dependent specialization.

[fmease]

Fixed by #125380. Right? I hope I'm not missing something.