Skip to content

Commit

Permalink
Rollup merge of rust-lang#107082 - dtolnay:autotraits, r=lcnr
Browse files Browse the repository at this point in the history
Autotrait bounds on dyn-safe trait methods

This PR is a successor to rust-lang#106604 implementing the approach encouraged by rust-lang#106604 (comment).

**I propose making it legal to use autotraits as trait bounds on the `Self` type of trait methods in a trait object.** rust-lang#51443 (comment) justifies why this use case is particularly important in the context of the async-trait crate.

```rust
#![feature(auto_traits)]
#![deny(where_clauses_object_safety)]

auto trait AutoTrait {}

trait MyTrait {
    fn f(&self) where Self: AutoTrait;
}

fn main() {
    let _: &dyn MyTrait;
}
```

Previously this would fail with:

```console
error: the trait `MyTrait` cannot be made into an object
 --> src/main.rs:7:8
  |
7 |     fn f(&self) where Self: AutoTrait;
  |        ^
  |
  = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
  = note: for more information, see issue rust-lang#51443 <rust-lang#51443>
note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
 --> src/main.rs:7:8
  |
6 | trait MyTrait {
  |       ------- this trait cannot be made into an object...
7 |     fn f(&self) where Self: AutoTrait;
  |        ^ ...because method `f` references the `Self` type in its `where` clause
  = help: consider moving `f` to another trait
```

In order for this to be sound without hitting rust-lang#50781, **I further propose that we disallow handwritten autotrait impls that apply to trait objects.** Both of the following were previously allowed (_on nightly_) and no longer allowed in my proposal:

```rust
auto trait AutoTrait {}

trait MyTrait {}
impl AutoTrait for dyn MyTrait {}  // NOT ALLOWED

impl<T: ?Sized> AutoTrait for T {}  // NOT ALLOWED
```

(`impl<T> AutoTrait for T {}` remains allowed.)

After this change, traits with a default impl are implemented for a trait object **if and only if** the autotrait is one of the trait object's trait bounds (or a supertrait of a bound). In other words `dyn Trait + AutoTrait` always implements AutoTrait while `dyn Trait` never implements AutoTrait.

Fixes dtolnay/async-trait#228.

r? `@lcnr`
  • Loading branch information
Dylan-DPC authored Feb 3, 2023
2 parents 7c46fb2 + fa84c6f commit 2edb0e9
Show file tree
Hide file tree
Showing 9 changed files with 376 additions and 78 deletions.
4 changes: 2 additions & 2 deletions compiler/rustc_data_structures/src/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,8 @@ cfg_if! {
pub auto trait Send {}
pub auto trait Sync {}

impl<T: ?Sized> Send for T {}
impl<T: ?Sized> Sync for T {}
impl<T> Send for T {}
impl<T> Sync for T {}

#[macro_export]
macro_rules! rustc_erase_owner {
Expand Down
222 changes: 182 additions & 40 deletions compiler/rustc_hir_analysis/src/coherence/orphan.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use rustc_hir as hir;
use rustc_middle::ty::subst::InternalSubsts;
use rustc_middle::ty::util::IgnoreRegions;
use rustc_middle::ty::{
self, ImplPolarity, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitor,
self, AliasKind, ImplPolarity, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitor,
};
use rustc_session::lint;
use rustc_span::def_id::{DefId, LocalDefId};
Expand Down Expand Up @@ -86,7 +86,7 @@ fn do_orphan_check_impl<'tcx>(
// struct B { }
// impl Foo for A { }
// impl Foo for B { }
// impl !Send for (A, B) { }
// impl !Foo for (A, B) { }
// ```
//
// This final impl is legal according to the orphan
Expand All @@ -99,50 +99,192 @@ fn do_orphan_check_impl<'tcx>(
tcx.trait_is_auto(trait_def_id)
);

if tcx.trait_is_auto(trait_def_id) && !trait_def_id.is_local() {
if tcx.trait_is_auto(trait_def_id) {
let self_ty = trait_ref.self_ty();
let opt_self_def_id = match *self_ty.kind() {
ty::Adt(self_def, _) => Some(self_def.did()),
ty::Foreign(did) => Some(did),
_ => None,
};

let msg = match opt_self_def_id {
// We only want to permit nominal types, but not *all* nominal types.
// They must be local to the current crate, so that people
// can't do `unsafe impl Send for Rc<SomethingLocal>` or
// `impl !Send for Box<SomethingLocalAndSend>`.
Some(self_def_id) => {
if self_def_id.is_local() {
None
// If the impl is in the same crate as the auto-trait, almost anything
// goes.
//
// impl MyAuto for Rc<Something> {} // okay
// impl<T> !MyAuto for *const T {} // okay
// impl<T> MyAuto for T {} // okay
//
// But there is one important exception: implementing for a trait object
// is not allowed.
//
// impl MyAuto for dyn Trait {} // NOT OKAY
// impl<T: ?Sized> MyAuto for T {} // NOT OKAY
//
// With this restriction, it's guaranteed that an auto-trait is
// implemented for a trait object if and only if the auto-trait is one
// of the trait object's trait bounds (or a supertrait of a bound). In
// other words `dyn Trait + AutoTrait` always implements AutoTrait,
// while `dyn Trait` never implements AutoTrait.
//
// This is necessary in order for autotrait bounds on methods of trait
// objects to be sound.
//
// auto trait AutoTrait {}
//
// trait ObjectSafeTrait {
// fn f(&self) where Self: AutoTrait;
// }
//
// We can allow f to be called on `dyn ObjectSafeTrait + AutoTrait`.
//
// If we didn't deny `impl AutoTrait for dyn Trait`, it would be unsound
// for the ObjectSafeTrait shown above to be object safe because someone
// could take some type implementing ObjectSafeTrait but not AutoTrait,
// unsize it to `dyn ObjectSafeTrait`, and call .f() which has no
// concrete implementation (issue #50781).
enum LocalImpl {
Allow,
Disallow { problematic_kind: &'static str },
}

// If the auto-trait is from a dependency, it must only be getting
// implemented for a nominal type, and specifically one local to the
// current crate.
//
// impl<T> Sync for MyStruct<T> {} // okay
//
// impl Sync for Rc<MyStruct> {} // NOT OKAY
enum NonlocalImpl {
Allow,
DisallowBecauseNonlocal,
DisallowOther,
}

// Exhaustive match considering that this logic is essential for
// soundness.
let (local_impl, nonlocal_impl) = match self_ty.kind() {
// struct Struct<T>;
// impl AutoTrait for Struct<Foo> {}
ty::Adt(self_def, _) => (
LocalImpl::Allow,
if self_def.did().is_local() {
NonlocalImpl::Allow
} else {
NonlocalImpl::DisallowBecauseNonlocal
},
),

// extern { type OpaqueType; }
// impl AutoTrait for OpaqueType {}
ty::Foreign(did) => (
LocalImpl::Allow,
if did.is_local() {
NonlocalImpl::Allow
} else {
Some((
format!(
"cross-crate traits with a default impl, like `{}`, \
can only be implemented for a struct/enum type \
defined in the current crate",
tcx.def_path_str(trait_def_id)
),
"can't implement cross-crate trait for type in another crate",
))
NonlocalImpl::DisallowBecauseNonlocal
},
),

// impl AutoTrait for dyn Trait {}
ty::Dynamic(..) => (
LocalImpl::Disallow { problematic_kind: "trait object" },
NonlocalImpl::DisallowOther,
),

// impl<T> AutoTrait for T {}
// impl<T: ?Sized> AutoTrait for T {}
ty::Param(..) => (
if self_ty.is_sized(tcx, tcx.param_env(def_id)) {
LocalImpl::Allow
} else {
LocalImpl::Disallow { problematic_kind: "generic type" }
},
NonlocalImpl::DisallowOther,
),

// trait Id { type This: ?Sized; }
// impl<T: ?Sized> Id for T {
// type This = T;
// }
// impl<T: ?Sized> AutoTrait for <T as Id>::This {}
ty::Alias(AliasKind::Projection, _) => (
LocalImpl::Disallow { problematic_kind: "associated type" },
NonlocalImpl::DisallowOther,
),

// type Opaque = impl Trait;
// impl AutoTrait for Opaque {}
ty::Alias(AliasKind::Opaque, _) => (
LocalImpl::Disallow { problematic_kind: "opaque type" },
NonlocalImpl::DisallowOther,
),

ty::Bool
| ty::Char
| ty::Int(..)
| ty::Uint(..)
| ty::Float(..)
| ty::Str
| ty::Array(..)
| ty::Slice(..)
| ty::RawPtr(..)
| ty::Ref(..)
| ty::FnDef(..)
| ty::FnPtr(..)
| ty::Never
| ty::Tuple(..) => (LocalImpl::Allow, NonlocalImpl::DisallowOther),

ty::Closure(..)
| ty::Generator(..)
| ty::GeneratorWitness(..)
| ty::Bound(..)
| ty::Placeholder(..)
| ty::Infer(..) => span_bug!(sp, "weird self type for autotrait impl"),

ty::Error(..) => (LocalImpl::Allow, NonlocalImpl::Allow),
};

if trait_def_id.is_local() {
match local_impl {
LocalImpl::Allow => {}
LocalImpl::Disallow { problematic_kind } => {
let msg = format!(
"traits with a default impl, like `{trait}`, \
cannot be implemented for {problematic_kind} `{self_ty}`",
trait = tcx.def_path_str(trait_def_id),
);
let label = format!(
"a trait object implements `{trait}` if and only if `{trait}` \
is one of the trait object's trait bounds",
trait = tcx.def_path_str(trait_def_id),
);
let reported =
struct_span_err!(tcx.sess, sp, E0321, "{}", msg).note(label).emit();
return Err(reported);
}
}
_ => Some((
format!(
"cross-crate traits with a default impl, like `{}`, can \
} else {
if let Some((msg, label)) = match nonlocal_impl {
NonlocalImpl::Allow => None,
NonlocalImpl::DisallowBecauseNonlocal => Some((
format!(
"cross-crate traits with a default impl, like `{}`, \
can only be implemented for a struct/enum type \
defined in the current crate",
tcx.def_path_str(trait_def_id)
),
"can't implement cross-crate trait for type in another crate",
)),
NonlocalImpl::DisallowOther => Some((
format!(
"cross-crate traits with a default impl, like `{}`, can \
only be implemented for a struct/enum type, not `{}`",
tcx.def_path_str(trait_def_id),
self_ty
),
"can't implement cross-crate trait with a default impl for \
non-struct/enum type",
)),
};

if let Some((msg, label)) = msg {
let reported =
struct_span_err!(tcx.sess, sp, E0321, "{}", msg).span_label(sp, label).emit();
return Err(reported);
tcx.def_path_str(trait_def_id),
self_ty
),
"can't implement cross-crate trait with a default impl for \
non-struct/enum type",
)),
} {
let reported =
struct_span_err!(tcx.sess, sp, E0321, "{}", msg).span_label(sp, label).emit();
return Err(reported);
}
}
}

Expand Down
60 changes: 50 additions & 10 deletions compiler/rustc_trait_selection/src/traits/object_safety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -547,16 +547,56 @@ fn virtual_call_violation_for_method<'tcx>(

// NOTE: This check happens last, because it results in a lint, and not a
// hard error.
if tcx
.predicates_of(method.def_id)
.predicates
.iter()
// A trait object can't claim to live more than the concrete type,
// so outlives predicates will always hold.
.cloned()
.filter(|(p, _)| p.to_opt_type_outlives().is_none())
.any(|pred| contains_illegal_self_type_reference(tcx, trait_def_id, pred))
{
if tcx.predicates_of(method.def_id).predicates.iter().any(|&(pred, span)| {
// dyn Trait is okay:
//
// trait Trait {
// fn f(&self) where Self: 'static;
// }
//
// because a trait object can't claim to live longer than the concrete
// type. If the lifetime bound holds on dyn Trait then it's guaranteed
// to hold as well on the concrete type.
if pred.to_opt_type_outlives().is_some() {
return false;
}

// dyn Trait is okay:
//
// auto trait AutoTrait {}
//
// trait Trait {
// fn f(&self) where Self: AutoTrait;
// }
//
// because `impl AutoTrait for dyn Trait` is disallowed by coherence.
// Traits with a default impl are implemented for a trait object if and
// only if the autotrait is one of the trait object's trait bounds, like
// in `dyn Trait + AutoTrait`. This guarantees that trait objects only
// implement auto traits if the underlying type does as well.
if let ty::PredicateKind::Clause(ty::Clause::Trait(ty::TraitPredicate {
trait_ref: pred_trait_ref,
constness: ty::BoundConstness::NotConst,
polarity: ty::ImplPolarity::Positive,
})) = pred.kind().skip_binder()
&& pred_trait_ref.self_ty() == tcx.types.self_param
&& tcx.trait_is_auto(pred_trait_ref.def_id)
{
// Consider bounds like `Self: Bound<Self>`. Auto traits are not
// allowed to have generic parameters so `auto trait Bound<T> {}`
// would already have reported an error at the definition of the
// auto trait.
if pred_trait_ref.substs.len() != 1 {
tcx.sess.diagnostic().delay_span_bug(
span,
"auto traits cannot have generic parameters",
);
}
return false;
}

contains_illegal_self_type_reference(tcx, trait_def_id, pred.clone())
}) {
return Some(MethodViolationCode::WhereClauseReferencesSelf);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,26 @@ auto trait Marker2 {}
trait Object: Marker1 {}

// A supertrait marker is illegal...
impl !Marker1 for dyn Object + Marker2 { } //~ ERROR E0371
impl !Marker1 for dyn Object + Marker2 {} //~ ERROR E0371
//~^ ERROR 0321
// ...and also a direct component.
impl !Marker2 for dyn Object + Marker2 { } //~ ERROR E0371

// But implementing a marker if it is not present is OK.
impl !Marker2 for dyn Object {} // OK
impl !Marker2 for dyn Object + Marker2 {} //~ ERROR E0371
//~^ ERROR 0321

// A non-principal trait-object type is orphan even in its crate.
impl !Send for dyn Marker2 {} //~ ERROR E0117

// And impl'ing a remote marker for a local trait object is forbidden
// by one of these special orphan-like rules.
// Implementing a marker for a local trait object is forbidden by a special
// orphan-like rule.
impl !Marker2 for dyn Object {} //~ ERROR E0321
impl !Send for dyn Object {} //~ ERROR E0321
impl !Send for dyn Object + Marker2 {} //~ ERROR E0321

fn main() { }
// Blanket impl that applies to dyn Object is equally problematic.
auto trait Marker3 {}
impl<T: ?Sized> !Marker3 for T {} //~ ERROR E0321

auto trait Marker4 {}
impl<T> !Marker4 for T {} // okay

fn main() {}
Loading

0 comments on commit 2edb0e9

Please sign in to comment.