Skip to content

Turn order dependent trait objects future incompat warning into a hard error #136968

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

Merged
merged 1 commit into from
Mar 9, 2025
Merged
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
37 changes: 0 additions & 37 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ declare_lint_pass! {
NEVER_TYPE_FALLBACK_FLOWING_INTO_UNSAFE,
NON_CONTIGUOUS_RANGE_ENDPOINTS,
NON_EXHAUSTIVE_OMITTED_PATTERNS,
ORDER_DEPENDENT_TRAIT_OBJECTS,
OUT_OF_SCOPE_MACRO_CALLS,
OVERLAPPING_RANGE_ENDPOINTS,
PATTERNS_IN_FNS_WITHOUT_BODY,
Expand Down Expand Up @@ -1502,42 +1501,6 @@ declare_lint! {
};
}

declare_lint! {
/// The `order_dependent_trait_objects` lint detects a trait coherency
/// violation that would allow creating two trait impls for the same
/// dynamic trait object involving marker traits.
///
/// ### Example
///
/// ```rust,compile_fail
/// pub trait Trait {}
///
/// impl Trait for dyn Send + Sync { }
/// impl Trait for dyn Sync + Send { }
/// ```
///
/// {{produces}}
///
/// ### Explanation
///
/// A previous bug caused the compiler to interpret traits with different
/// orders (such as `Send + Sync` and `Sync + Send`) as distinct types
/// when they were intended to be treated the same. This allowed code to
/// define separate trait implementations when there should be a coherence
/// error. This is a [future-incompatible] lint to transition this to a
/// hard error in the future. See [issue #56484] for more details.
///
/// [issue #56484]: https://github.com/rust-lang/rust/issues/56484
/// [future-incompatible]: ../index.md#future-incompatible-lints
pub ORDER_DEPENDENT_TRAIT_OBJECTS,
Deny,
"trait-object types were treated as different depending on marker-trait order",
@future_incompatible = FutureIncompatibleInfo {
reason: FutureIncompatibilityReason::FutureReleaseErrorReportInDeps,
reference: "issue #56484 <https://github.com/rust-lang/rust/issues/56484>",
};
}

declare_lint! {
/// The `coherence_leak_check` lint detects conflicting implementations of
/// a trait that are only distinguished by the old leak-check code.
Expand Down
6 changes: 0 additions & 6 deletions compiler/rustc_middle/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -985,12 +985,6 @@ rustc_queries! {
separate_provide_extern
}

query self_ty_of_trait_impl_enabling_order_dep_trait_object_hack(
key: DefId
) -> Option<ty::EarlyBinder<'tcx, ty::Ty<'tcx>>> {
desc { |tcx| "computing self type wrt issue #33140 `{}`", tcx.def_path_str(key) }
}

/// Maps a `DefId` of a type to a list of its inherent impls.
/// Contains implementations of methods that are inherent to a type.
/// Methods in these implementations don't need to be exported.
Expand Down
47 changes: 1 addition & 46 deletions compiler/rustc_middle/src/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1424,39 +1424,6 @@ pub enum ImplOverlapKind {
/// Whether or not the impl is permitted due to the trait being a `#[marker]` trait
marker: bool,
},
/// These impls are allowed to overlap, but that raises an
/// issue #33140 future-compatibility warning (tracked in #56484).
///
/// Some background: in Rust 1.0, the trait-object types `Send + Sync` (today's
/// `dyn Send + Sync`) and `Sync + Send` (now `dyn Sync + Send`) were different.
///
/// The widely-used version 0.1.0 of the crate `traitobject` had accidentally relied on
/// that difference, doing what reduces to the following set of impls:
///
/// ```compile_fail,(E0119)
/// trait Trait {}
/// impl Trait for dyn Send + Sync {}
/// impl Trait for dyn Sync + Send {}
/// ```
///
/// Obviously, once we made these types be identical, that code causes a coherence
/// error and a fairly big headache for us. However, luckily for us, the trait
/// `Trait` used in this case is basically a marker trait, and therefore having
/// overlapping impls for it is sound.
///
/// To handle this, we basically regard the trait as a marker trait, with an additional
/// future-compatibility warning. To avoid accidentally "stabilizing" this feature,
/// it has the following restrictions:
///
/// 1. The trait must indeed be a marker-like trait (i.e., no items), and must be
/// positive impls.
/// 2. The trait-ref of both impls must be equal.
/// 3. The trait-ref of both impls must be a trait object type consisting only of
/// marker traits.
/// 4. Neither of the impls can have any where-clauses.
///
/// Once `traitobject` 0.1.0 is no longer an active concern, this hack can be removed.
FutureCompatOrderDepTraitObjects,
}

/// Useful source information about where a desugared associated type for an
Expand Down Expand Up @@ -1629,7 +1596,7 @@ impl<'tcx> TyCtxt<'tcx> {
})
}

/// Returns `true` if the impls are the same polarity and the trait either
/// Returns `Some` if the impls are the same polarity and the trait either
/// has no items or is annotated `#[marker]` and prevents item overrides.
#[instrument(level = "debug", skip(self), ret)]
pub fn impls_are_allowed_to_overlap(
Expand Down Expand Up @@ -1670,18 +1637,6 @@ impl<'tcx> TyCtxt<'tcx> {
return Some(ImplOverlapKind::Permitted { marker: true });
}

if let Some(self_ty1) =
self.self_ty_of_trait_impl_enabling_order_dep_trait_object_hack(def_id1)
&& let Some(self_ty2) =
self.self_ty_of_trait_impl_enabling_order_dep_trait_object_hack(def_id2)
{
if self_ty1 == self_ty2 {
return Some(ImplOverlapKind::FutureCompatOrderDepTraitObjects);
} else {
debug!("found {self_ty1:?} != {self_ty2:?}");
}
}

None
}

Expand Down
14 changes: 3 additions & 11 deletions compiler/rustc_trait_selection/src/traits/select/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1920,9 +1920,9 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
let mut impl_candidate = None;
for c in impls {
if let Some(prev) = impl_candidate.replace(c) {
if self.prefer_lhs_over_victim(has_non_region_infer, c, prev) {
if self.prefer_lhs_over_victim(has_non_region_infer, c, prev.0) {
// Ok, prefer `c` over the previous entry
} else if self.prefer_lhs_over_victim(has_non_region_infer, prev, c) {
} else if self.prefer_lhs_over_victim(has_non_region_infer, prev, c.0) {
// Ok, keep `prev` instead of the new entry
impl_candidate = Some(prev);
} else {
Expand Down Expand Up @@ -1981,7 +1981,7 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
&self,
has_non_region_infer: bool,
(lhs, lhs_evaluation): (DefId, EvaluationResult),
(victim, victim_evaluation): (DefId, EvaluationResult),
victim: DefId,
) -> bool {
let tcx = self.tcx();
// See if we can toss out `victim` based on specialization.
Expand All @@ -1997,14 +1997,6 @@ impl<'tcx> SelectionContext<'_, 'tcx> {
}

match tcx.impls_are_allowed_to_overlap(lhs, victim) {
// For #33140 the impl headers must be exactly equal, the trait must not have
// any associated items and there are no where-clauses.
//
// We can just arbitrarily drop one of the impls.
Some(ty::ImplOverlapKind::FutureCompatOrderDepTraitObjects) => {
assert_eq!(lhs_evaluation, victim_evaluation);
true
}
// For candidates which already reference errors it doesn't really
// matter what we do 🤷
Some(ty::ImplOverlapKind::Permitted { marker: false }) => {
Expand Down
9 changes: 2 additions & 7 deletions compiler/rustc_trait_selection/src/traits/specialize/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use rustc_middle::bug;
use rustc_middle::query::LocalCrate;
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_session::lint::builtin::COHERENCE_LEAK_CHECK;
use rustc_span::{DUMMY_SP, ErrorGuaranteed, Span, sym};
use rustc_type_ir::solve::NoSolution;
use specialization_graph::GraphExt;
Expand Down Expand Up @@ -557,13 +557,9 @@ fn report_conflicting_impls<'tcx>(

let msg = || {
format!(
"conflicting implementations of trait `{}`{}{}",
"conflicting implementations of trait `{}`{}",
overlap.trait_ref.print_trait_sugared(),
overlap.self_ty.map_or_else(String::new, |ty| format!(" for type `{ty}`")),
match used_to_be_allowed {
Some(FutureCompatOverlapErrorKind::OrderDepTraitObjects) => ": (E0119)",
_ => "",
}
)
};

Expand All @@ -588,7 +584,6 @@ fn report_conflicting_impls<'tcx>(
}
Some(kind) => {
let lint = match kind {
FutureCompatOverlapErrorKind::OrderDepTraitObjects => ORDER_DEPENDENT_TRAIT_OBJECTS,
FutureCompatOverlapErrorKind::LeakCheck => COHERENCE_LEAK_CHECK,
};
tcx.node_span_lint(lint, tcx.local_def_id_to_hir_id(impl_def_id), impl_span, |err| {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ use crate::traits;

#[derive(Copy, Clone, Debug)]
pub enum FutureCompatOverlapErrorKind {
OrderDepTraitObjects,
LeakCheck,
}

Expand Down Expand Up @@ -151,12 +150,6 @@ impl<'tcx> Children {
{
match overlap_kind {
ty::ImplOverlapKind::Permitted { marker: _ } => {}
ty::ImplOverlapKind::FutureCompatOrderDepTraitObjects => {
*last_lint_mut = Some(FutureCompatOverlapError {
error: create_overlap_error(overlap),
kind: FutureCompatOverlapErrorKind::OrderDepTraitObjects,
});
}
}

return Ok((false, false));
Expand Down
58 changes: 2 additions & 56 deletions compiler/rustc_ty_utils/src/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,11 @@ use rustc_index::bit_set::DenseBitSet;
use rustc_middle::bug;
use rustc_middle::query::Providers;
use rustc_middle::ty::fold::fold_regions;
use rustc_middle::ty::{
self, EarlyBinder, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitor, Upcast,
};
use rustc_middle::ty::{self, Ty, TyCtxt, TypeSuperVisitable, TypeVisitable, TypeVisitor, Upcast};
use rustc_span::DUMMY_SP;
use rustc_span::def_id::{CRATE_DEF_ID, DefId, LocalDefId};
use rustc_trait_selection::traits;
use tracing::{debug, instrument};
use tracing::instrument;

#[instrument(level = "debug", skip(tcx), ret)]
fn sized_constraint_for_ty<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> Option<Ty<'tcx>> {
Expand Down Expand Up @@ -257,57 +255,6 @@ fn param_env_normalized_for_post_analysis(tcx: TyCtxt<'_>, def_id: DefId) -> ty:
typing_env.with_post_analysis_normalized(tcx).param_env
}

/// If the given trait impl enables exploiting the former order dependence of trait objects,
/// returns its self type; otherwise, returns `None`.
///
/// See [`ty::ImplOverlapKind::FutureCompatOrderDepTraitObjects`] for more details.
#[instrument(level = "debug", skip(tcx))]
fn self_ty_of_trait_impl_enabling_order_dep_trait_object_hack(
tcx: TyCtxt<'_>,
def_id: DefId,
) -> Option<EarlyBinder<'_, Ty<'_>>> {
let impl_ =
tcx.impl_trait_header(def_id).unwrap_or_else(|| bug!("called on inherent impl {def_id:?}"));

let trait_ref = impl_.trait_ref.skip_binder();
debug!(?trait_ref);

let is_marker_like = impl_.polarity == ty::ImplPolarity::Positive
&& tcx.associated_item_def_ids(trait_ref.def_id).is_empty();

// Check whether these impls would be ok for a marker trait.
if !is_marker_like {
debug!("not marker-like!");
return None;
}

// impl must be `impl Trait for dyn Marker1 + Marker2 + ...`
if trait_ref.args.len() != 1 {
debug!("impl has args!");
return None;
}

let predicates = tcx.predicates_of(def_id);
if predicates.parent.is_some() || !predicates.predicates.is_empty() {
debug!(?predicates, "impl has predicates!");
return None;
}

let self_ty = trait_ref.self_ty();
let self_ty_matches = match self_ty.kind() {
ty::Dynamic(data, re, _) if re.is_static() => data.principal().is_none(),
_ => false,
};

if self_ty_matches {
debug!("MATCHES!");
Some(EarlyBinder::bind(self_ty))
} else {
debug!("non-matching self type");
None
}
}

/// Check if a function is async.
fn asyncness(tcx: TyCtxt<'_>, def_id: LocalDefId) -> ty::Asyncness {
let node = tcx.hir_node_by_def_id(def_id);
Expand Down Expand Up @@ -367,7 +314,6 @@ pub(crate) fn provide(providers: &mut Providers) {
adt_sized_constraint,
param_env,
param_env_normalized_for_post_analysis,
self_ty_of_trait_impl_enabling_order_dep_trait_object_hack,
defaultness,
unsizing_params_for_adt,
..*providers
Expand Down
3 changes: 0 additions & 3 deletions tests/ui/lint/lint-incoherent-auto-trait-objects.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,13 @@ impl Foo for dyn Send {}

impl Foo for dyn Send + Send {}
//~^ ERROR conflicting implementations
//~| hard error

impl Foo for dyn Send + Sync {}

impl Foo for dyn Sync + Send {}
//~^ ERROR conflicting implementations
//~| hard error

impl Foo for dyn Send + Sync + Send {}
//~^ ERROR conflicting implementations
//~| hard error

fn main() {}
Loading
Loading