Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions compiler/rustc_middle/src/query/keys.rs
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,14 @@ impl Key for (DefId, SimplifiedType) {
}
}

impl Key for (DefId, Option<DefId>) {
type Cache<V> = DefaultCache<Self, V>;

fn default_span(&self, tcx: TyCtxt<'_>) -> Span {
self.0.default_span(tcx)
}
}

impl<'tcx> Key for GenericArgsRef<'tcx> {
type Cache<V> = DefaultCache<Self, V>;

Expand Down
9 changes: 9 additions & 0 deletions compiler/rustc_middle/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1373,6 +1373,15 @@ rustc_queries! {
desc { |tcx| "checking if trait `{}` is dyn-compatible", tcx.def_path_str(trait_id) }
}

query trait_has_impl_which_may_shadow_dyn(key: (DefId, Option<DefId>)) -> bool {
desc {
|tcx| "checking if trait `{}` has an impl which may overlap with \
the built-in impl for `dyn {}`",
tcx.def_path_str(key.0),
key.1.map_or(String::from("..."), |def_id| tcx.def_path_str(def_id)),
}
}

/// Gets the ParameterEnvironment for a given item; this environment
/// will be in "user-facing" mode, meaning that it is suitable for
/// type-checking etc, and it does not normalize specializable
Expand Down
8 changes: 8 additions & 0 deletions compiler/rustc_middle/src/ty/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -589,6 +589,14 @@ impl<'tcx> Interner for TyCtxt<'tcx> {
self.trait_def(trait_def_id).safety == hir::Safety::Unsafe
}

fn trait_has_impl_which_may_shadow_dyn(
self,
trait_def_id: DefId,
principal_def_id: Option<DefId>,
) -> bool {
self.trait_has_impl_which_may_shadow_dyn((trait_def_id, principal_def_id))
}

fn is_impl_trait_in_trait(self, def_id: DefId) -> bool {
self.is_impl_trait_in_trait(def_id)
}
Expand Down
11 changes: 11 additions & 0 deletions compiler/rustc_next_trait_solver/src/solve/assembly/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,17 @@ where
goal: Goal<I, Self>,
assumption: I::Clause,
) -> Result<Candidate<I>, NoSolution> {
let ty::Dynamic(data, _, _) = goal.predicate.self_ty().kind() else {
unreachable!();
};

if ecx.cx().trait_has_impl_which_may_shadow_dyn(
goal.predicate.trait_def_id(ecx.cx()),
data.principal_def_id(),
) {
return Err(NoSolution);
}

Self::probe_and_match_goal_against_assumption(ecx, source, goal, assumption, |ecx| {
let cx = ecx.cx();
let ty::Dynamic(bounds, _, _) = goal.predicate.self_ty().kind() else {
Expand Down
1 change: 1 addition & 0 deletions compiler/rustc_trait_selection/src/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -830,6 +830,7 @@ pub fn provide(providers: &mut Providers) {
specialization_graph_of: specialize::specialization_graph_provider,
specializes: specialize::specializes,
specialization_enabled_in: specialize::specialization_enabled_in,
trait_has_impl_which_may_shadow_dyn: specialize::trait_has_impl_which_may_shadow_dyn,
instantiate_and_check_impossible_predicates,
is_impossible_associated_item,
..*providers
Expand Down
6 changes: 6 additions & 0 deletions compiler/rustc_trait_selection/src/traits/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -817,6 +817,12 @@ fn assemble_candidates_from_object_ty<'cx, 'tcx>(
let env_predicates = data
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let env_predicates = data
let object_ty_predicates = data

.projection_bounds()
.filter(|bound| bound.item_def_id() == obligation.predicate.def_id)
.filter(|bound| {
!tcx.trait_has_impl_which_may_shadow_dyn((
bound.trait_def_id(tcx),
data.principal_def_id(),
))
})
.map(|p| p.with_self_ty(tcx, object_ty).upcast(tcx));

assemble_candidates_from_predicates(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -832,7 +832,8 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
"assemble_candidates_from_object_ty",
);

if !self.tcx().trait_def(obligation.predicate.def_id()).implement_via_object {
let tcx = self.tcx();
if !tcx.trait_def(obligation.predicate.def_id()).implement_via_object {
return;
}

Expand All @@ -853,9 +854,9 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {

if let Some(principal) = data.principal() {
if !self.infcx.tcx.features().dyn_compatible_for_dispatch() {
principal.with_self_ty(self.tcx(), self_ty)
} else if self.tcx().is_dyn_compatible(principal.def_id()) {
principal.with_self_ty(self.tcx(), self_ty)
principal.with_self_ty(tcx, self_ty)
} else if tcx.is_dyn_compatible(principal.def_id()) {
principal.with_self_ty(tcx, self_ty)
} else {
return;
}
Expand All @@ -879,7 +880,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
// correct trait, but also the correct type parameters.
// For example, we may be trying to upcast `Foo` to `Bar<i32>`,
// but `Foo` is declared as `trait Foo: Bar<u32>`.
let candidate_supertraits = util::supertraits(self.tcx(), principal_trait_ref)
let candidate_supertraits = util::supertraits(tcx, principal_trait_ref)
.enumerate()
.filter(|&(_, upcast_trait_ref)| {
self.infcx.probe(|_| {
Expand All @@ -891,6 +892,12 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
.is_ok()
})
})
.filter(|(_, trait_ref)| {
!tcx.trait_has_impl_which_may_shadow_dyn((
trait_ref.def_id(),
Some(principal_trait_ref.def_id()),
))
})
.map(|(idx, _)| ObjectCandidate(idx));

candidates.vec.extend(candidate_supertraits);
Expand Down
130 changes: 129 additions & 1 deletion compiler/rustc_trait_selection/src/traits/specialize/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,10 @@

pub mod specialization_graph;

use rustc_data_structures::fx::FxIndexSet;
use rustc_data_structures::fx::{FxHashSet, FxIndexSet};
use rustc_errors::codes::*;
use rustc_errors::{Diag, EmissionGuarantee};
use rustc_hir::LangItem;
use rustc_hir::def_id::{DefId, LocalDefId};
use rustc_infer::traits::Obligation;
use rustc_middle::bug;
Expand All @@ -22,6 +23,8 @@ use rustc_middle::ty::print::PrintTraitRefExt as _;
use rustc_middle::ty::{self, GenericArgsRef, Ty, TyCtxt, TypeVisitableExt, TypingMode};
use rustc_session::lint::builtin::{COHERENCE_LEAK_CHECK, ORDER_DEPENDENT_TRAIT_OBJECTS};
use rustc_span::{DUMMY_SP, ErrorGuaranteed, Span, sym};
use rustc_type_ir::elaborate;
use rustc_type_ir::fast_reject::{SimplifiedType, TreatParams, simplify_type};
use rustc_type_ir::solve::NoSolution;
use specialization_graph::GraphExt;
use tracing::{debug, instrument};
Expand Down Expand Up @@ -595,3 +598,128 @@ fn report_conflicting_impls<'tcx>(
}
}
}

pub(super) fn trait_has_impl_which_may_shadow_dyn<'tcx>(
tcx: TyCtxt<'tcx>,
(target_trait_def_id, principal_def_id): (DefId, Option<DefId>),
) -> bool {
// We only care about trait objects which have associated types.
if !tcx
.associated_items(target_trait_def_id)
.in_definition_order()
.any(|item| item.kind == ty::AssocKind::Type)
{
return false;
}

let target_self_ty =
principal_def_id.map_or(SimplifiedType::MarkerTraitObject, SimplifiedType::Trait);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
principal_def_id.map_or(SimplifiedType::MarkerTraitObject, SimplifiedType::Trait);
SimplifiedType::Trait(principal_def_id.unwrap());

do we get here with auto-traits? I don't think so, as we'd always return true in this case


let elaborated_supertraits =
principal_def_id.into_iter().flat_map(|def_id| tcx.supertrait_def_ids(def_id)).collect();

trait_has_impl_inner(
tcx,
target_trait_def_id,
target_self_ty,
&elaborated_supertraits,
&mut Default::default(),
false,
)
}

fn trait_has_impl_inner<'tcx>(
tcx: TyCtxt<'tcx>,
target_trait_def_id: DefId,
target_self_ty: SimplifiedType<DefId>,
elaborated_supertraits: &FxHashSet<DefId>,
seen_traits: &mut FxHashSet<DefId>,
for_nested_impl: bool,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pls change to an enum ForNestedImpl

or wait: alternative:

if we duplicate the "is Sized check" in fn trait_has_impl_which_may_shadow_dyn and have it call an extracted function fn check_user_written_impls, which then recurse into trait_has_impl_inner, we don't need ForNestedImpl at all.

) -> bool {
if tcx.is_lang_item(target_trait_def_id, LangItem::Sized) {
return false;
}

// If we've encountered a trait in a cycle, then let's just
// consider it to be implemented defensively.
if !seen_traits.insert(target_trait_def_id) {
return true;
}
// Since we don't pass in the set of auto traits, and just the principal,
// consider all auto traits implemented.
if tcx.trait_is_auto(target_trait_def_id) {
return true;
}
// When checking the outermost call of this recursive function, we don't care
// about elaborated supertraits or built-in impls. This is intuitively because
// `dyn Trait: Trait` doesn't overlap with itself, and all built-in impls have
// `rustc_deny_explicit_impl(implement_via_object = true)`.
if for_nested_impl {
if elaborated_supertraits.contains(&target_trait_def_id) {
return true;
}
// Lazy heuristic that any lang item is potentially a built-in impl.
if tcx.as_lang_item(target_trait_def_id).is_some() {
return true;
}
}

let mut has_offending_impl = false;
tcx.for_each_impl(target_trait_def_id, |impl_def_id| {
if has_offending_impl {
return;
}

let self_ty = tcx
.impl_trait_ref(impl_def_id)
.expect("impl must have trait ref")
.instantiate_identity()
.self_ty();

if simplify_type(tcx, self_ty, TreatParams::InstantiateWithInfer)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if simplify_type(tcx, self_ty, TreatParams::InstantiateWithInfer)
// We only care about impls which are implemented for the trait object
// itself and blanket impls which may apply.
if simplify_type(tcx, self_ty, TreatParams::InstantiateWithInfer)

.is_some_and(|simp| simp != target_self_ty)
{
return;
}

for (pred, _) in
elaborate::elaborate(tcx, tcx.predicates_of(impl_def_id).instantiate_identity(tcx))
{
if let ty::ClauseKind::Trait(trait_pred) = pred.kind().skip_binder()
&& trait_pred.self_ty() == self_ty
&& !trait_has_impl_inner(
tcx,
trait_pred.def_id(),
target_self_ty,
elaborated_supertraits,
seen_traits,
true,
)
{
return;
}
}

if let ty::Alias(ty::Projection, alias_ty) = self_ty.kind() {
for pred in tcx.item_super_predicates(alias_ty.def_id).iter_identity() {
if let ty::ClauseKind::Trait(trait_pred) = pred.kind().skip_binder()
&& trait_pred.self_ty() == self_ty
&& !trait_has_impl_inner(
tcx,
trait_pred.def_id(),
target_self_ty,
elaborated_supertraits,
seen_traits,
false,
)
{
return;
}
}
}

has_offending_impl = true;
});

has_offending_impl
}
6 changes: 6 additions & 0 deletions compiler/rustc_type_ir/src/interner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,12 @@ pub trait Interner:
/// Returns `true` if this is an `unsafe trait`.
fn trait_is_unsafe(self, trait_def_id: Self::DefId) -> bool;

fn trait_has_impl_which_may_shadow_dyn(
self,
trait_def_id: Self::DefId,
principal_def_id: Option<Self::DefId>,
) -> bool;

fn is_impl_trait_in_trait(self, def_id: Self::DefId) -> bool;

fn delay_bug(self, msg: impl ToString) -> Self::ErrorGuaranteed;
Expand Down
4 changes: 4 additions & 0 deletions compiler/rustc_type_ir/src/predicate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -433,6 +433,10 @@ impl<I: Interner> ty::Binder<I, ExistentialProjection<I>> {
pub fn item_def_id(&self) -> I::DefId {
self.skip_binder().def_id
}

pub fn trait_def_id(self, interner: I) -> I::DefId {
interner.parent(self.skip_binder().def_id)
}
}

#[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Debug)]
Expand Down
25 changes: 25 additions & 0 deletions tests/ui/coherence/indirect-impl-blanket-doesnt-overlap-object.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
//@ check-pass

// Make sure that if we don't disqualify a built-in object impl
// due to a blanket with a trait bound that will never apply to
// the object.

pub trait SimpleService {
type Resp;
}

trait Service {
type Resp;
}

impl<S> Service for S where S: SimpleService + ?Sized {
type Resp = <S as SimpleService>::Resp;
}

fn implements_service(x: &(impl Service<Resp = ()> + ?Sized)) {}

fn test(x: &dyn Service<Resp = ()>) {
implements_service(x);
}

fn main() {}
43 changes: 43 additions & 0 deletions tests/ui/coherence/indirect-impl-blanket-downstream-trait-2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
trait Trait {
type Assoc;
fn generate(&self) -> Self::Assoc;
}

trait Other {}

impl<S> Trait for S where S: Other + ?Sized {
type Assoc = &'static str;
fn generate(&self) -> Self::Assoc { "hi" }
}

trait Downstream: Trait<Assoc = usize> {}
impl<T> Other for T where T: ?Sized + Downstream + OnlyDyn {}

trait OnlyDyn {}
impl OnlyDyn for dyn Downstream {}

struct Concrete;
impl Trait for Concrete {
type Assoc = usize;
fn generate(&self) -> Self::Assoc { 42 }
}
impl Downstream for Concrete {}

fn test<T: ?Sized + Other>(x: &T) {
let s: &str = x.generate();
println!("{s}");
}

fn impl_downstream<T: ?Sized + Downstream>(x: &T) {}

fn main() {
let x: &dyn Downstream = &Concrete;

test(x); // This call used to segfault.
//~^ ERROR type mismatch resolving

// This no longer holds since `Downstream: Trait<Assoc = usize>`,
// but the `Trait<Assoc = &'static str>` blanket impl now shadows.
impl_downstream(x);
//~^ ERROR type mismatch resolving
}
Loading
Loading