Skip to content

Commit

Permalink
make incoherent_auto_trait_objects a hard error
Browse files Browse the repository at this point in the history
  • Loading branch information
lcnr committed Sep 29, 2022
1 parent 8a497b7 commit 96ae278
Show file tree
Hide file tree
Showing 14 changed files with 73 additions and 318 deletions.
36 changes: 0 additions & 36 deletions compiler/rustc_lint_defs/src/builtin.rs
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
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
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
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
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
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
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

0 comments on commit 96ae278

Please sign in to comment.