Skip to content

make incoherent_auto_trait_objects a hard error #102474

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

make incoherent_auto_trait_objects a hard error #102474

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
36 changes: 0 additions & 36 deletions compiler/rustc_lint_defs/src/builtin.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1426,41 +1426,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 {
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 Expand Up @@ -3302,7 +3267,6 @@ declare_lint_pass! {
PATTERNS_IN_FNS_WITHOUT_BODY,
MISSING_FRAGMENT_SPECIFIER,
LATE_BOUND_LIFETIME_ARGUMENTS,
ORDER_DEPENDENT_TRAIT_OBJECTS,
COHERENCE_LEAK_CHECK,
DEPRECATED,
UNUSED_UNSAFE,
Expand Down
4 changes: 0 additions & 4 deletions compiler/rustc_middle/src/query/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -722,10 +722,6 @@ rustc_queries! {
separate_provide_extern
}

query issue33140_self_ty(key: DefId) -> Option<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
67 changes: 7 additions & 60 deletions compiler/rustc_middle/src/ty/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2109,45 +2109,9 @@ impl<'tcx> FieldDef {

pub type Attributes<'tcx> = impl Iterator<Item = &'tcx ast::Attribute>;
#[derive(Debug, PartialEq, Eq)]
pub enum ImplOverlapKind {
/// These impls are always allowed to overlap.
Permitted {
/// 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.
///
/// 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
/// that difference, making 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.
Issue33140,
pub struct PermittedImplOverlap {
/// Whether or not the impl is permitted due to the trait being a `#[marker]` trait
pub marker: bool,
}

impl<'tcx> TyCtxt<'tcx> {
Expand Down Expand Up @@ -2229,13 +2193,13 @@ impl<'tcx> TyCtxt<'tcx> {
self,
def_id1: DefId,
def_id2: DefId,
) -> Option<ImplOverlapKind> {
) -> Option<PermittedImplOverlap> {
// If either trait impl references an error, they're allowed to overlap,
// as one of them essentially doesn't exist.
if self.impl_trait_ref(def_id1).map_or(false, |tr| tr.references_error())
|| self.impl_trait_ref(def_id2).map_or(false, |tr| tr.references_error())
{
return Some(ImplOverlapKind::Permitted { marker: false });
return Some(PermittedImplOverlap { marker: false });
}

match (self.impl_polarity(def_id1), self.impl_polarity(def_id2)) {
Expand All @@ -2245,7 +2209,7 @@ impl<'tcx> TyCtxt<'tcx> {
"impls_are_allowed_to_overlap({:?}, {:?}) = Some(Permitted) (reservations)",
def_id1, def_id2
);
return Some(ImplOverlapKind::Permitted { marker: false });
return Some(PermittedImplOverlap { marker: false });
}
(ImplPolarity::Positive, ImplPolarity::Negative)
| (ImplPolarity::Negative, ImplPolarity::Positive) => {
Expand Down Expand Up @@ -2273,25 +2237,8 @@ impl<'tcx> TyCtxt<'tcx> {
"impls_are_allowed_to_overlap({:?}, {:?}) = Some(Permitted) (marker overlap)",
def_id1, def_id2
);
Some(ImplOverlapKind::Permitted { marker: true })
Some(PermittedImplOverlap { marker: true })
} else {
if let Some(self_ty1) = self.issue33140_self_ty(def_id1) {
if let Some(self_ty2) = self.issue33140_self_ty(def_id2) {
if self_ty1 == self_ty2 {
debug!(
"impls_are_allowed_to_overlap({:?}, {:?}) - issue #33140 HACK",
def_id1, def_id2
);
return Some(ImplOverlapKind::Issue33140);
} else {
debug!(
"impls_are_allowed_to_overlap({:?}, {:?}) - found {:?} != {:?}",
def_id1, def_id2, self_ty1, self_ty2
);
}
}
}

debug!("impls_are_allowed_to_overlap({:?}, {:?}) = None", def_id1, def_id2);
None
}
Expand Down
1 change: 0 additions & 1 deletion compiler/rustc_trait_selection/src/traits/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ pub use self::project::{normalize, normalize_projection_type, normalize_to};
pub use self::select::{EvaluationCache, SelectionCache, SelectionContext};
pub use self::select::{EvaluationResult, IntercrateAmbiguityCause, OverflowError};
pub use self::specialize::specialization_graph::FutureCompatOverlapError;
pub use self::specialize::specialization_graph::FutureCompatOverlapErrorKind;
pub use self::specialize::{specialization_graph, translate_substs, OverlapError};
pub use self::structural_match::{
search_for_adt_const_param_violation, search_for_structural_match_violation,
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_trait_selection/src/traits/select/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1752,7 +1752,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {

if other.evaluation.must_apply_considering_regions() {
match tcx.impls_are_allowed_to_overlap(other_def, victim_def) {
Some(ty::ImplOverlapKind::Permitted { marker: true }) => {
Some(ty::PermittedImplOverlap { marker: true }) => {
// Subtle: If the predicate we are evaluating has inference
// variables, do *not* allow discarding candidates due to
// marker trait impls.
Expand Down
72 changes: 26 additions & 46 deletions compiler/rustc_trait_selection/src/traits/specialize/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,13 @@ use specialization_graph::GraphExt;
use crate::errors::NegativePositiveConflict;
use crate::infer::{InferCtxt, InferOk, TyCtxtInferExt};
use crate::traits::select::IntercrateAmbiguityCause;
use crate::traits::{self, coherence, FutureCompatOverlapErrorKind, ObligationCause};
use crate::traits::{self, coherence, ObligationCause};
use rustc_data_structures::fx::{FxHashSet, FxIndexSet};
use rustc_errors::{struct_span_err, EmissionGuarantee, LintDiagnosticBuilder};
use rustc_hir::def_id::{DefId, LocalDefId};
use rustc_middle::ty::{self, ImplSubject, TyCtxt};
use rustc_middle::ty::{InternalSubsts, SubstsRef};
use rustc_session::lint::builtin::COHERENCE_LEAK_CHECK;
use rustc_session::lint::builtin::ORDER_DEPENDENT_TRAIT_OBJECTS;
use rustc_span::{Span, DUMMY_SP};

use super::util;
Expand Down Expand Up @@ -260,9 +259,9 @@ pub(super) fn specialization_graph_provider(
let insert_result = sg.insert(tcx, impl_def_id.to_def_id(), overlap_mode);
// Report error if there was one.
let (overlap, used_to_be_allowed) = match insert_result {
Err(overlap) => (Some(overlap), None),
Ok(Some(overlap)) => (Some(overlap.error), Some(overlap.kind)),
Ok(None) => (None, None),
Err(overlap) => (Some(overlap), false),
Ok(Some(overlap)) => (Some(overlap.error), overlap.used_to_be_allowed),
Ok(None) => (None, false),
};

if let Some(overlap) = overlap {
Expand All @@ -286,7 +285,7 @@ fn report_overlap_conflict(
tcx: TyCtxt<'_>,
overlap: OverlapError,
impl_def_id: LocalDefId,
used_to_be_allowed: Option<FutureCompatOverlapErrorKind>,
used_to_be_allowed: bool,
sg: &mut specialization_graph::Graph,
) {
let impl_polarity = tcx.impl_polarity(impl_def_id.to_def_id());
Expand Down Expand Up @@ -342,7 +341,7 @@ fn report_conflicting_impls(
tcx: TyCtxt<'_>,
overlap: OverlapError,
impl_def_id: LocalDefId,
used_to_be_allowed: Option<FutureCompatOverlapErrorKind>,
used_to_be_allowed: bool,
sg: &mut specialization_graph::Graph,
) {
let impl_span = tcx.def_span(impl_def_id);
Expand All @@ -353,21 +352,16 @@ fn report_conflicting_impls(
fn decorate<G: EmissionGuarantee>(
tcx: TyCtxt<'_>,
overlap: OverlapError,
used_to_be_allowed: Option<FutureCompatOverlapErrorKind>,
impl_span: Span,
err: LintDiagnosticBuilder<'_, G>,
) -> G {
let msg = format!(
"conflicting implementations of trait `{}`{}{}",
"conflicting implementations of trait `{}`{}",
overlap.trait_desc,
overlap
.self_desc
.clone()
.map_or_else(String::new, |ty| { format!(" for type `{}`", ty) }),
match used_to_be_allowed {
Some(FutureCompatOverlapErrorKind::Issue33140) => ": (E0119)",
_ => "",
}
);
let mut err = err.build(&msg);
match tcx.span_of_impl(overlap.with_impl) {
Expand Down Expand Up @@ -401,39 +395,25 @@ fn report_conflicting_impls(
err.emit()
}

match used_to_be_allowed {
None => {
let reported = if overlap.with_impl.is_local()
|| tcx.orphan_check_impl(impl_def_id).is_ok()
{
let err = struct_span_err!(tcx.sess, impl_span, E0119, "");
Some(decorate(
tcx,
overlap,
used_to_be_allowed,
impl_span,
LintDiagnosticBuilder::new(err),
))
} else {
Some(tcx.sess.delay_span_bug(impl_span, "impl should have failed the orphan check"))
};
sg.has_errored = reported;
}
Some(kind) => {
let lint = match kind {
FutureCompatOverlapErrorKind::Issue33140 => ORDER_DEPENDENT_TRAIT_OBJECTS,
FutureCompatOverlapErrorKind::LeakCheck => COHERENCE_LEAK_CHECK,
};
tcx.struct_span_lint_hir(
lint,
tcx.hir().local_def_id_to_hir_id(impl_def_id),
impl_span,
|ldb| {
decorate(tcx, overlap, used_to_be_allowed, impl_span, ldb);
},
);
}
};
if used_to_be_allowed {
tcx.struct_span_lint_hir(
COHERENCE_LEAK_CHECK,
tcx.hir().local_def_id_to_hir_id(impl_def_id),
impl_span,
|ldb| {
decorate(tcx, overlap, impl_span, ldb);
},
);
} else {
let reported = if overlap.with_impl.is_local() || tcx.orphan_check_impl(impl_def_id).is_ok()
{
let err = struct_span_err!(tcx.sess, impl_span, E0119, "");
Some(decorate(tcx, overlap, impl_span, LintDiagnosticBuilder::new(err)))
} else {
Some(tcx.sess.delay_span_bug(impl_span, "impl should have failed the orphan check"))
};
sg.has_errored = reported;
}
}

/// Recovers the "impl X for Y" signature from `impl_def_id` and returns it as a
Expand Down
21 changes: 3 additions & 18 deletions compiler/rustc_trait_selection/src/traits/specialize/specialization_graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,10 @@ use rustc_middle::ty::{self, TyCtxt, TypeVisitable};

pub use rustc_middle::traits::specialization_graph::*;

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

#[derive(Debug)]
pub struct FutureCompatOverlapError {
pub error: OverlapError,
pub kind: FutureCompatOverlapErrorKind,
pub used_to_be_allowed: bool,
}

/// The result of attempting to insert an impl into a group of children.
Expand Down Expand Up @@ -146,10 +140,7 @@ impl ChildrenExt<'_> for Children {
if should_err {
Err(error)
} else {
*last_lint = Some(FutureCompatOverlapError {
error,
kind: FutureCompatOverlapErrorKind::LeakCheck,
});
*last_lint = Some(FutureCompatOverlapError { error, used_to_be_allowed: true });

Ok((false, false))
}
Expand All @@ -167,13 +158,7 @@ impl ChildrenExt<'_> for Children {
tcx.impls_are_allowed_to_overlap(impl_def_id, possible_sibling)
{
match overlap_kind {
ty::ImplOverlapKind::Permitted { marker: _ } => {}
ty::ImplOverlapKind::Issue33140 => {
*last_lint_mut = Some(FutureCompatOverlapError {
error: create_overlap_error(overlap),
kind: FutureCompatOverlapErrorKind::Issue33140,
});
}
ty::PermittedImplOverlap { marker: _ } => {}
}

return Ok((false, false));
Expand Down
Loading