From 56f5704ff843256912260678433778f27bb6f6c8 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Tue, 25 Jul 2023 01:49:29 +0000 Subject: [PATCH] Implement lint against coinductive impl overlap --- compiler/rustc_lint_defs/src/builtin.rs | 51 +++++++++++++++++++ .../src/traits/coherence.rs | 45 ++++++++++++++-- .../src/traits/select/mod.rs | 36 +++++++++++-- .../warn-when-cycle-is-error-in-coherence.rs | 35 +++++++++++++ ...rn-when-cycle-is-error-in-coherence.stderr | 22 ++++++++ 5 files changed, 183 insertions(+), 6 deletions(-) create mode 100644 tests/ui/coherence/warn-when-cycle-is-error-in-coherence.rs create mode 100644 tests/ui/coherence/warn-when-cycle-is-error-in-coherence.stderr diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index 3747e562cec58..b3e5fee4185ae 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -3366,6 +3366,7 @@ declare_lint_pass! { BYTE_SLICE_IN_PACKED_STRUCT_WITH_DERIVE, CENUM_IMPL_DROP_CAST, COHERENCE_LEAK_CHECK, + COINDUCTIVE_OVERLAP_IN_COHERENCE, CONFLICTING_REPR_HINTS, CONST_EVALUATABLE_UNCHECKED, CONST_ITEM_MUTATION, @@ -4422,6 +4423,56 @@ declare_lint! { @feature_gate = sym::type_privacy_lints; } +declare_lint! { + /// The `coinductive_overlap_in_coherence` lint detects impls which are currently + /// considered not overlapping, but may be considered to overlap if support for + /// coinduction is added to the trait solver. + /// + /// ### Example + /// + /// ```rust,compile_fail + /// use std::borrow::Borrow; + /// use std::cmp::Ordering; + /// use std::marker::PhantomData; + /// + /// #[derive(PartialEq, Default)] + /// pub(crate) struct Interval(T); + /// + /// impl PartialEq for Interval + /// where + /// Q: PartialOrd, + /// { + /// fn eq(&self, other: &Q) -> bool { + /// todo!() + /// } + /// } + /// + /// impl PartialOrd for Interval + /// where + /// Q: PartialOrd, + /// { + /// fn partial_cmp(&self, other: &Q) -> Option { + /// todo!() + /// } + /// } + /// ``` + /// + /// {{produces}} + /// + /// ### Explanation + /// + /// The manual impl of `PartialEq` impl overlaps with the `derive`, since + /// if we replace `Q = Interval`, then the second impl leads to a cycle: + /// `PartialOrd for Interval where Interval: Partial`. + pub COINDUCTIVE_OVERLAP_IN_COHERENCE, + Warn, + "impls that are not considered to overlap may be considered to \ + overlap in the future", + @future_incompatible = FutureIncompatibleInfo { + reference: "issue #114040 ", + }; +} + declare_lint! { /// The `unknown_diagnostic_attributes` lint detects unrecognized diagnostic attributes. /// diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs index dd15f1a5492ee..2cc0b6548769a 100644 --- a/compiler/rustc_trait_selection/src/traits/coherence.rs +++ b/compiler/rustc_trait_selection/src/traits/coherence.rs @@ -24,6 +24,7 @@ use rustc_middle::traits::DefiningAnchor; use rustc_middle::ty::fast_reject::{DeepRejectCtxt, TreatParams}; use rustc_middle::ty::visit::{TypeVisitable, TypeVisitableExt}; use rustc_middle::ty::{self, Ty, TyCtxt, TypeVisitor}; +use rustc_session::lint::builtin::COINDUCTIVE_OVERLAP_IN_COHERENCE; use rustc_span::symbol::sym; use rustc_span::DUMMY_SP; use std::fmt::Debug; @@ -295,9 +296,8 @@ fn impl_intersection_has_impossible_obligation<'cx, 'tcx>( if infcx.next_trait_solver() { infcx.evaluate_obligation(obligation).map_or(false, |result| !result.may_apply()) } else { - // We use `evaluate_root_obligation` to correctly track - // intercrate ambiguity clauses. We do not need this in the - // new solver. + // We use `evaluate_root_obligation` to correctly track intercrate + // ambiguity clauses. We cannot use this in the new solver. selcx.evaluate_root_obligation(obligation).map_or( false, // Overflow has occurred, and treat the obligation as possibly holding. |result| !result.may_apply(), @@ -315,6 +315,45 @@ fn impl_intersection_has_impossible_obligation<'cx, 'tcx>( .find(obligation_guaranteed_to_fail); if let Some(failing_obligation) = opt_failing_obligation { + // Check the failing obligation once again, treating inductive cycles as + // ambiguous instead of error. + if !infcx.next_trait_solver() + && SelectionContext::with_treat_inductive_cycle_as_ambiguous(infcx) + .evaluate_root_obligation(&failing_obligation) + .map_or(true, |result| result.may_apply()) + { + let first_local_impl = impl1_header + .impl_def_id + .as_local() + .or(impl2_header.impl_def_id.as_local()) + .expect("expected one of the impls to be local"); + infcx.tcx.struct_span_lint_hir( + COINDUCTIVE_OVERLAP_IN_COHERENCE, + infcx.tcx.local_def_id_to_hir_id(first_local_impl), + infcx.tcx.def_span(first_local_impl), + "impls that are not considered to overlap may be considered to \ + overlap in the future", + |lint| { + lint.span_label( + infcx.tcx.def_span(impl1_header.impl_def_id), + "the first impl is here", + ) + .span_label( + infcx.tcx.def_span(impl2_header.impl_def_id), + "the second impl is here", + ); + if !failing_obligation.cause.span.is_dummy() { + lint.span_label( + failing_obligation.cause.span, + "this where-clause causes a cycle, but it may be treated \ + as possibly holding in the future, causing the impls to overlap", + ); + } + lint + }, + ); + } + debug!("overlap: obligation unsatisfiable {:?}", failing_obligation); true } else { diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs index 2bd2b08115784..0a8aa1bee26ac 100644 --- a/compiler/rustc_trait_selection/src/traits/select/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/select/mod.rs @@ -118,6 +118,8 @@ pub struct SelectionContext<'cx, 'tcx> { /// policy. In essence, canonicalized queries need their errors propagated /// rather than immediately reported because we do not have accurate spans. query_mode: TraitQueryMode, + + treat_inductive_cycle: TreatInductiveCycleAs, } // A stack that walks back up the stack frame. @@ -198,6 +200,11 @@ enum BuiltinImplConditions<'tcx> { Ambiguous, } +enum TreatInductiveCycleAs { + Recur, + Ambig, +} + impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { pub fn new(infcx: &'cx InferCtxt<'tcx>) -> SelectionContext<'cx, 'tcx> { SelectionContext { @@ -205,6 +212,20 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { freshener: infcx.freshener(), intercrate_ambiguity_causes: None, query_mode: TraitQueryMode::Standard, + treat_inductive_cycle: TreatInductiveCycleAs::Recur, + } + } + + pub fn with_treat_inductive_cycle_as_ambiguous( + infcx: &'cx InferCtxt<'tcx>, + ) -> SelectionContext<'cx, 'tcx> { + assert!(infcx.intercrate, "this doesn't do caching yet, so don't use it out of intercrate"); + SelectionContext { + infcx, + freshener: infcx.freshener(), + intercrate_ambiguity_causes: None, + query_mode: TraitQueryMode::Standard, + treat_inductive_cycle: TreatInductiveCycleAs::Ambig, } } @@ -719,7 +740,10 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { stack.update_reached_depth(stack_arg.1); return Ok(EvaluatedToOk); } else { - return Ok(EvaluatedToRecur); + match self.treat_inductive_cycle { + TreatInductiveCycleAs::Ambig => return Ok(EvaluatedToAmbig), + TreatInductiveCycleAs::Recur => return Ok(EvaluatedToRecur), + } } } return Ok(EvaluatedToOk); @@ -837,7 +861,10 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { } } ProjectAndUnifyResult::FailedNormalization => Ok(EvaluatedToAmbig), - ProjectAndUnifyResult::Recursive => Ok(EvaluatedToRecur), + ProjectAndUnifyResult::Recursive => match self.treat_inductive_cycle { + TreatInductiveCycleAs::Ambig => return Ok(EvaluatedToAmbig), + TreatInductiveCycleAs::Recur => return Ok(EvaluatedToRecur), + }, ProjectAndUnifyResult::MismatchedProjectionTypes(_) => Ok(EvaluatedToErr), } } @@ -1151,7 +1178,10 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> { Some(EvaluatedToOk) } else { debug!("evaluate_stack --> recursive, inductive"); - Some(EvaluatedToRecur) + match self.treat_inductive_cycle { + TreatInductiveCycleAs::Ambig => Some(EvaluatedToAmbig), + TreatInductiveCycleAs::Recur => Some(EvaluatedToRecur), + } } } else { None diff --git a/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.rs b/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.rs new file mode 100644 index 0000000000000..80530acd2aada --- /dev/null +++ b/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.rs @@ -0,0 +1,35 @@ +#![deny(coinductive_overlap_in_coherence)] + +use std::borrow::Borrow; +use std::cmp::Ordering; +use std::marker::PhantomData; + +#[derive(PartialEq, Default)] +pub(crate) struct Interval(PhantomData); + +// This impl overlaps with the `derive` unless we reject the nested +// `Interval: PartialOrd>` candidate which results +// in an inductive cycle right now. +impl PartialEq for Interval +//~^ ERROR impls that are not considered to overlap may be considered to overlap in the future +//~| WARN this was previously accepted by the compiler but is being phased out +where + T: Borrow, + Q: ?Sized + PartialOrd, +{ + fn eq(&self, _: &Q) -> bool { + true + } +} + +impl PartialOrd for Interval +where + T: Borrow, + Q: ?Sized + PartialOrd, +{ + fn partial_cmp(&self, _: &Q) -> Option { + None + } +} + +fn main() {} diff --git a/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.stderr b/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.stderr new file mode 100644 index 0000000000000..fd6193f4b7424 --- /dev/null +++ b/tests/ui/coherence/warn-when-cycle-is-error-in-coherence.stderr @@ -0,0 +1,22 @@ +error: impls that are not considered to overlap may be considered to overlap in the future + --> $DIR/warn-when-cycle-is-error-in-coherence.rs:13:1 + | +LL | #[derive(PartialEq, Default)] + | --------- the second impl is here +... +LL | impl PartialEq for Interval + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the first impl is here +... +LL | Q: ?Sized + PartialOrd, + | ---------- this where-clause causes a cycle, but it may be treated as possibly holding in the future, causing the impls to overlap + | + = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! + = note: for more information, see issue #114040 +note: the lint level is defined here + --> $DIR/warn-when-cycle-is-error-in-coherence.rs:1:9 + | +LL | #![deny(coinductive_overlap_in_coherence)] + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to previous error +