Skip to content

Commit 888ca93

Browse files
dtolnaydtolnay
committed
PR 107082 review feedback
1 parent ba9b196 commit 888ca93

File tree

2 files changed

+174
-96
lines changed

2 files changed

+174
-96
lines changed

‎compiler/rustc_hir_analysis/src/coherence/orphan.rs‎

Lines changed: 160 additions & 90 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use rustc_hir as hir;
88
use rustc_middle::ty::subst::InternalSubsts;
99
use rustc_middle::ty::util::IgnoreRegions;
1010
use rustc_middle::ty::{
11-
self, ImplPolarity, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitor,
11+
self, AliasKind, ImplPolarity, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitor,
1212
};
1313
use rustc_session::lint;
1414
use rustc_span::def_id::{DefId, LocalDefId};
@@ -101,97 +101,169 @@ fn do_orphan_check_impl<'tcx>(
101101

102102
if tcx.trait_is_auto(trait_def_id) {
103103
let self_ty = trait_ref.self_ty();
104-
let self_ty_kind = self_ty.kind();
104+
105+
// If the impl is in the same crate as the auto-trait, almost anything
106+
// goes.
107+
//
108+
// impl MyAuto for Rc<Something> {} // okay
109+
// impl<T> !MyAuto for *const T {} // okay
110+
// impl<T> MyAuto for T {} // okay
111+
//
112+
// But there is one important exception: implementing for a trait object
113+
// is not allowed.
114+
//
115+
// impl MyAuto for dyn Trait {} // NOT OKAY
116+
// impl<T: ?Sized> MyAuto for T {} // NOT OKAY
117+
//
118+
// With this restriction, it's guaranteed that an auto-trait is
119+
// implemented for a trait object if and only if the auto-trait is one
120+
// of the trait object's trait bounds (or a supertrait of a bound). In
121+
// other words `dyn Trait + AutoTrait` always implements AutoTrait,
122+
// while `dyn Trait` never implements AutoTrait.
123+
//
124+
// This is necessary in order for autotrait bounds on methods of trait
125+
// objects to be sound.
126+
//
127+
// auto trait AutoTrait {}
128+
//
129+
// trait ObjectSafeTrait {
130+
// fn f(&self) where Self: AutoTrait;
131+
// }
132+
//
133+
// We can allow f to be called on `dyn ObjectSafeTrait + AutoTrait`.
134+
//
135+
// If we didn't deny `impl AutoTrait for dyn Trait`, it would be unsound
136+
// for the ObjectSafeTrait shown above to be object safe because someone
137+
// could take some type implementing ObjectSafeTrait but not AutoTrait,
138+
// unsize it to `dyn ObjectSafeTrait`, and call .f() which has no
139+
// concrete implementation (issue #50781).
140+
enum LocalImpl {
141+
Allow,
142+
Disallow { problematic_kind: &'static str },
143+
}
144+
145+
// If the auto-trait is from a dependency, it must only be getting
146+
// implemented for a nominal type, and specifically one local to the
147+
// current crate.
148+
//
149+
// impl<T> Sync for MyStruct<T> {} // okay
150+
//
151+
// impl Sync for Rc<MyStruct> {} // NOT OKAY
152+
enum NonlocalImpl {
153+
Allow,
154+
DisallowBecauseNonlocal,
155+
DisallowOther,
156+
}
157+
158+
// Exhaustive match considering that this logic is essential for
159+
// soundness.
160+
let (local_impl, nonlocal_impl) = match self_ty.kind() {
161+
// struct Struct<T>;
162+
// impl AutoTrait for Struct<Foo> {}
163+
ty::Adt(self_def, _) => (
164+
LocalImpl::Allow,
165+
if self_def.did().is_local() {
166+
NonlocalImpl::Allow
167+
} else {
168+
NonlocalImpl::DisallowBecauseNonlocal
169+
},
170+
),
171+
172+
// extern { type OpaqueType; }
173+
// impl AutoTrait for OpaqueType {}
174+
ty::Foreign(did) => (
175+
LocalImpl::Allow,
176+
if did.is_local() {
177+
NonlocalImpl::Allow
178+
} else {
179+
NonlocalImpl::DisallowBecauseNonlocal
180+
},
181+
),
182+
183+
// impl AutoTrait for dyn Trait {}
184+
ty::Dynamic(..) => (
185+
LocalImpl::Disallow { problematic_kind: "trait object" },
186+
NonlocalImpl::DisallowOther,
187+
),
188+
189+
// impl<T> AutoTrait for T {}
190+
// impl<T: ?Sized> AutoTrait for T {}
191+
ty::Param(..) => (
192+
if self_ty.is_sized(tcx, tcx.param_env(def_id)) {
193+
LocalImpl::Allow
194+
} else {
195+
LocalImpl::Disallow { problematic_kind: "generic type" }
196+
},
197+
NonlocalImpl::DisallowOther,
198+
),
199+
200+
// trait Id { type This: ?Sized; }
201+
// impl<T: ?Sized> Id for T {
202+
// type This = T;
203+
// }
204+
// impl<T: ?Sized> AutoTrait for <T as Id>::This {}
205+
ty::Alias(AliasKind::Projection, _) => (
206+
LocalImpl::Disallow { problematic_kind: "associated type" },
207+
NonlocalImpl::DisallowOther,
208+
),
209+
210+
// Some of these should perhaps be a delay_span_bug.
211+
ty::Bool
212+
| ty::Char
213+
| ty::Int(..)
214+
| ty::Uint(..)
215+
| ty::Float(..)
216+
| ty::Str
217+
| ty::Array(..)
218+
| ty::Slice(..)
219+
| ty::RawPtr(..)
220+
| ty::Ref(..)
221+
| ty::FnDef(..)
222+
| ty::FnPtr(..)
223+
| ty::Closure(..)
224+
| ty::Generator(..)
225+
| ty::GeneratorWitness(..)
226+
| ty::Never
227+
| ty::Tuple(..)
228+
| ty::Alias(AliasKind::Opaque, ..)
229+
| ty::Bound(..)
230+
| ty::Placeholder(..)
231+
| ty::Infer(..)
232+
| ty::Error(..) => (LocalImpl::Allow, NonlocalImpl::DisallowOther),
233+
};
105234

106235
if trait_def_id.is_local() {
107-
// If the auto-trait is local, almost anything goes.
108-
//
109-
// impl MyAuto for Rc<Something> {} // okay
110-
// impl<T> !MyAuto for *const T {} // okay
111-
// impl<T> MyAuto for T {} // okay
112-
//
113-
// But there is one important exception: implementing for a trait
114-
// object is not allowed.
115-
//
116-
// impl MyAuto for dyn Trait {} // NOT OKAY
117-
// impl<T: ?Sized> MyAuto for T {} // NOT OKAY
118-
//
119-
// With this restriction, it's guaranteed that an auto-trait is
120-
// implemented for a trait object if and only if the auto-trait is
121-
// one of the trait object's trait bounds (or a supertrait of a
122-
// bound). In other words `dyn Trait + AutoTrait` always implements
123-
// AutoTrait, while `dyn Trait` never implements AutoTrait.
124-
//
125-
// This is necessary in order for autotrait bounds on methods of
126-
// trait objects to be sound.
127-
//
128-
// auto trait AutoTrait {}
129-
//
130-
// trait ObjectSafeTrait {
131-
// fn f(&self) where Self: AutoTrait;
132-
// }
133-
//
134-
// We can allow f to be called on `dyn ObjectSafeTrait + AutoTrait`.
135-
//
136-
// If we didn't deny `impl AutoTrait for dyn Trait`, it would be
137-
// unsound for the ObjectSafeTrait shown above to be object safe
138-
// because someone could take some type implementing ObjectSafeTrait
139-
// but not AutoTrait, unsize it to `dyn ObjectSafeTrait`, and call
140-
// .f() which has no concrete implementation (issue #50781).
141-
let problematic_kind = match self_ty_kind {
142-
ty::Dynamic(..) => Some("trait object"),
143-
ty::Param(..) if !self_ty.is_sized(tcx, tcx.param_env(def_id)) => {
144-
Some("generic type")
236+
match local_impl {
237+
LocalImpl::Allow => {}
238+
LocalImpl::Disallow { problematic_kind } => {
239+
let msg = format!(
240+
"traits with a default impl, like `{trait}`, \
241+
cannot be implemented for {problematic_kind} `{self_ty}`",
242+
trait = tcx.def_path_str(trait_def_id),
243+
);
244+
let label = format!(
245+
"a trait object implements `{trait}` if and only if `{trait}` \
246+
is one of the trait object's trait bounds",
247+
trait = tcx.def_path_str(trait_def_id),
248+
);
249+
let reported =
250+
struct_span_err!(tcx.sess, sp, E0321, "{}", msg).note(label).emit();
251+
return Err(reported);
145252
}
146-
_ => None,
147-
};
148-
149-
if let Some(problematic_kind) = problematic_kind {
150-
let msg = format!(
151-
"traits with a default impl, like `{trait}`, \
152-
cannot be implemented for {problematic_kind} `{self_ty}`",
153-
trait = tcx.def_path_str(trait_def_id),
154-
);
155-
let label = format!(
156-
"a trait object implements `{trait}` if and only if `{trait}` \
157-
is one of the trait object's trait bounds",
158-
trait = tcx.def_path_str(trait_def_id),
159-
);
160-
let reported = struct_span_err!(tcx.sess, sp, E0321, "{}", msg).note(label).emit();
161-
return Err(reported);
162253
}
163254
} else {
164-
// If the auto-trait is not local, it must only be getting
165-
// implemented for a nominal type, and specifically one local to the
166-
// current crate.
167-
//
168-
// impl<T> Sync for MyStruct<T> {} // okay
169-
//
170-
// impl Sync for Rc<MyStruct> {} // NOT OKAY
171-
//
172-
let opt_self_def_id = match *self_ty_kind {
173-
ty::Adt(self_def, _) => Some(self_def.did()),
174-
ty::Foreign(did) => Some(did),
175-
_ => None,
176-
};
177-
178-
let msg = match opt_self_def_id {
179-
Some(self_def_id) => {
180-
if self_def_id.is_local() {
181-
None
182-
} else {
183-
Some((
184-
format!(
185-
"cross-crate traits with a default impl, like `{}`, \
186-
can only be implemented for a struct/enum type \
187-
defined in the current crate",
188-
tcx.def_path_str(trait_def_id)
189-
),
190-
"can't implement cross-crate trait for type in another crate",
191-
))
192-
}
193-
}
194-
None => Some((
255+
if let Some((msg, label)) = match nonlocal_impl {
256+
NonlocalImpl::Allow => None,
257+
NonlocalImpl::DisallowBecauseNonlocal => Some((
258+
format!(
259+
"cross-crate traits with a default impl, like `{}`, \
260+
can only be implemented for a struct/enum type \
261+
defined in the current crate",
262+
tcx.def_path_str(trait_def_id)
263+
),
264+
"can't implement cross-crate trait for type in another crate",
265+
)),
266+
NonlocalImpl::DisallowOther => Some((
195267
format!(
196268
"cross-crate traits with a default impl, like `{}`, can \
197269
only be implemented for a struct/enum type, not `{}`",
@@ -201,9 +273,7 @@ fn do_orphan_check_impl<'tcx>(
201273
"can't implement cross-crate trait with a default impl for \
202274
non-struct/enum type",
203275
)),
204-
};
205-
206-
if let Some((msg, label)) = msg {
276+
} {
207277
let reported =
208278
struct_span_err!(tcx.sess, sp, E0321, "{}", msg).span_label(sp, label).emit();
209279
return Err(reported);

‎compiler/rustc_trait_selection/src/traits/object_safety.rs‎

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -529,7 +529,7 @@ fn virtual_call_violation_for_method<'tcx>(
529529

530530
// NOTE: This check happens last, because it results in a lint, and not a
531531
// hard error.
532-
if tcx.predicates_of(method.def_id).predicates.iter().any(|(pred, _span)| {
532+
if tcx.predicates_of(method.def_id).predicates.iter().any(|&(pred, span)| {
533533
// dyn Trait is okay:
534534
//
535535
// trait Trait {
@@ -554,7 +554,8 @@ fn virtual_call_violation_for_method<'tcx>(
554554
// because `impl AutoTrait for dyn Trait` is disallowed by coherence.
555555
// Traits with a default impl are implemented for a trait object if and
556556
// only if the autotrait is one of the trait object's trait bounds, like
557-
// in `dyn Trait + AutoTrait`.
557+
// in `dyn Trait + AutoTrait`. This guarantees that trait objects only
558+
// implement auto traits if the underlying type does as well.
558559
if let ty::PredicateKind::Clause(ty::Clause::Trait(ty::TraitPredicate {
559560
trait_ref: pred_trait_ref,
560561
constness: ty::BoundConstness::NotConst,
@@ -563,10 +564,17 @@ fn virtual_call_violation_for_method<'tcx>(
563564
&& pred_trait_ref.self_ty() == tcx.types.self_param
564565
&& tcx.trait_is_auto(pred_trait_ref.def_id)
565566
{
566-
// Only check the rest of the bound's parameters. So `Self: Bound<Self>`
567-
// is still considered illegal.
568-
let rest_of_substs = &pred_trait_ref.substs[1..];
569-
return contains_illegal_self_type_reference(tcx, trait_def_id, rest_of_substs);
567+
// Consider bounds like `Self: Bound<Self>`. Auto traits are not
568+
// allowed to have generic parameters so `auto trait Bound<T> {}`
569+
// would already have reported an error at the definition of the
570+
// auto trait.
571+
if pred_trait_ref.substs.len() != 1 {
572+
tcx.sess.diagnostic().delay_span_bug(
573+
span,
574+
"auto traits cannot have generic parameters",
575+
);
576+
}
577+
return false;
570578
}
571579

572580
contains_illegal_self_type_reference(tcx, trait_def_id, pred.clone())

0 commit comments

Comments
 (0)