Auto merge of #114023 - compiler-errors:coinductive-cycle-lint, r=lcnr

Warn on inductive cycle in coherence leading to impls being considered not overlapping

This PR implements a `coinductive_overlap_in_coherence` lint (#114040), which warns users against cases where two impls are considered **not** to overlap during coherence due to an inductive cycle disproving one of the predicates after unifying the two impls.

Cases where this lint fires will become an overlap error if we ever move to coinduction, so I'd like to make this a warning to avoid having more crates take advantage of this behavior in the mean time. Also, since the new trait solver treats inductive cycles as ambiguity, not an error, this is a blocker for landing the new trait solver in coherence.

Author: bors

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,44 @@ 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
+    /// #![deny(coinductive_overlap_in_coherence)]
+    ///
+    /// trait CyclicTrait {}
+    /// impl<T: CyclicTrait> CyclicTrait for T {}
+    ///
+    /// trait Trait {}
+    /// impl<T: CyclicTrait> Trait for T {}
+    /// // conflicting impl with the above
+    /// impl Trait for u8 {}
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// We have two choices for impl which satisfy `u8: Trait`: the blanket impl
+    /// for generic `T`, and the direct impl for `u8`. These two impls nominally
+    /// overlap, since we can infer `T = u8` in the former impl, but since the where
+    /// clause `u8: CyclicTrait` would end up resulting in a cycle (since it depends
+    /// on itself), the blanket impl is not considered to hold for `u8`. This will
+    /// change in a future release.
+    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 <https://github.com/rust-lang/rust/issues/114040>",
+    };
+}
+
 declare_lint! {
     /// The `unknown_diagnostic_attributes` lint detects unrecognized diagnostic attributes.
     ///

compiler/rustc_middle/src/ty/mod.rs

@@ -238,7 +238,7 @@ pub struct ImplHeader<'tcx> {
     pub impl_def_id: DefId,
     pub self_ty: Ty<'tcx>,
     pub trait_ref: Option<TraitRef<'tcx>>,
-    pub predicates: Vec<Predicate<'tcx>>,
+    pub predicates: Vec<(Predicate<'tcx>, Span)>,
 }
 
 #[derive(Copy, Clone, PartialEq, Eq, Debug, TypeFoldable, TypeVisitable)]

compiler/rustc_trait_selection/src/traits/coherence.rs

@@ -7,7 +7,7 @@
 use crate::infer::outlives::env::OutlivesEnvironment;
 use crate::infer::InferOk;
 use crate::traits::outlives_bounds::InferCtxtExt as _;
-use crate::traits::select::IntercrateAmbiguityCause;
+use crate::traits::select::{IntercrateAmbiguityCause, TreatInductiveCycleAs};
 use crate::traits::util::impl_subject_and_oblig;
 use crate::traits::SkipLeakCheck;
 use crate::traits::{
@@ -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;
@@ -151,14 +152,16 @@ fn with_fresh_ty_vars<'cx, 'tcx>(
             .predicates_of(impl_def_id)
             .instantiate(tcx, impl_args)
             .iter()
-            .map(|(c, _)| c.as_predicate())
+            .map(|(c, s)| (c.as_predicate(), s))
             .collect(),
     };
 
-    let InferOk { value: mut header, obligations } =
-        selcx.infcx.at(&ObligationCause::dummy(), param_env).normalize(header);
+    let InferOk { value: mut header, obligations } = selcx
+        .infcx
+        .at(&ObligationCause::dummy_with_span(tcx.def_span(impl_def_id)), param_env)
+        .normalize(header);
 
-    header.predicates.extend(obligations.into_iter().map(|o| o.predicate));
+    header.predicates.extend(obligations.into_iter().map(|o| (o.predicate, o.cause.span)));
     header
 }
 
@@ -207,16 +210,76 @@ fn overlap<'tcx>(
     let equate_obligations = equate_impl_headers(selcx.infcx, &impl1_header, &impl2_header)?;
     debug!("overlap: unification check succeeded");
 
-    if overlap_mode.use_implicit_negative()
-        && impl_intersection_has_impossible_obligation(
-            selcx,
-            param_env,
-            &impl1_header,
-            impl2_header,
-            equate_obligations,
-        )
-    {
-        return None;
+    if overlap_mode.use_implicit_negative() {
+        for mode in [TreatInductiveCycleAs::Ambig, TreatInductiveCycleAs::Recur] {
+            if let Some(failing_obligation) = selcx.with_treat_inductive_cycle_as(mode, |selcx| {
+                impl_intersection_has_impossible_obligation(
+                    selcx,
+                    param_env,
+                    &impl1_header,
+                    &impl2_header,
+                    &equate_obligations,
+                )
+            }) {
+                if matches!(mode, TreatInductiveCycleAs::Recur) {
+                    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),
+                        format!(
+                            "implementations {} will conflict in the future",
+                            match impl1_header.trait_ref {
+                                Some(trait_ref) => {
+                                    let trait_ref = infcx.resolve_vars_if_possible(trait_ref);
+                                    format!(
+                                        "of `{}` for `{}`",
+                                        trait_ref.print_only_trait_path(),
+                                        trait_ref.self_ty()
+                                    )
+                                }
+                                None => format!(
+                                    "for `{}`",
+                                    infcx.resolve_vars_if_possible(impl1_header.self_ty)
+                                ),
+                            },
+                        ),
+                        |lint| {
+                            lint.note(
+                                "impls that are not considered to overlap may be considered to \
+                                overlap in the future",
+                            )
+                            .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,
+                                    format!(
+                                        "`{}` may be considered to hold in future releases, \
+                                        causing the impls to overlap",
+                                        infcx
+                                            .resolve_vars_if_possible(failing_obligation.predicate)
+                                    ),
+                                );
+                            }
+                            lint
+                        },
+                    );
+                }
+
+                return None;
+            }
+        }
     }
 
     // We toggle the `leak_check` by using `skip_leak_check` when constructing the
@@ -284,40 +347,30 @@ fn impl_intersection_has_impossible_obligation<'cx, 'tcx>(
     selcx: &mut SelectionContext<'cx, 'tcx>,
     param_env: ty::ParamEnv<'tcx>,
     impl1_header: &ty::ImplHeader<'tcx>,
-    impl2_header: ty::ImplHeader<'tcx>,
-    obligations: PredicateObligations<'tcx>,
-) -> bool {
+    impl2_header: &ty::ImplHeader<'tcx>,
+    obligations: &PredicateObligations<'tcx>,
+) -> Option<PredicateObligation<'tcx>> {
     let infcx = selcx.infcx;
 
-    let obligation_guaranteed_to_fail = move |obligation: &PredicateObligation<'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.
-            selcx.evaluate_root_obligation(obligation).map_or(
-                false, // Overflow has occurred, and treat the obligation as possibly holding.
-                |result| !result.may_apply(),
-            )
-        }
-    };
-
-    let opt_failing_obligation = [&impl1_header.predicates, &impl2_header.predicates]
+    [&impl1_header.predicates, &impl2_header.predicates]
         .into_iter()
         .flatten()
-        .map(|&predicate| {
-            Obligation::new(infcx.tcx, ObligationCause::dummy(), param_env, predicate)
+        .map(|&(predicate, span)| {
+            Obligation::new(infcx.tcx, ObligationCause::dummy_with_span(span), param_env, predicate)
+        })
+        .chain(obligations.into_iter().cloned())
+        .find(|obligation: &PredicateObligation<'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 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(),
+                )
+            }
         })
-        .chain(obligations)
-        .find(obligation_guaranteed_to_fail);
-
-    if let Some(failing_obligation) = opt_failing_obligation {
-        debug!("overlap: obligation unsatisfiable {:?}", failing_obligation);
-        true
-    } else {
-        false
-    }
 }
 
 /// Check if both impls can be satisfied by a common type by considering whether

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,16 +200,54 @@ enum BuiltinImplConditions<'tcx> {
     Ambiguous,
 }
 
+#[derive(Copy, Clone)]
+pub enum TreatInductiveCycleAs {
+    /// This is the previous behavior, where `Recur` represents an inductive
+    /// cycle that is known not to hold. This is not forwards-compatible with
+    /// coinduction, and will be deprecated. This is the default behavior
+    /// of the old trait solver due to back-compat reasons.
+    Recur,
+    /// This is the behavior of the new trait solver, where inductive cycles
+    /// are treated as ambiguous and possibly holding.
+    Ambig,
+}
+
+impl From<TreatInductiveCycleAs> for EvaluationResult {
+    fn from(treat: TreatInductiveCycleAs) -> EvaluationResult {
+        match treat {
+            TreatInductiveCycleAs::Ambig => EvaluatedToUnknown,
+            TreatInductiveCycleAs::Recur => EvaluatedToRecur,
+        }
+    }
+}
+
 impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
     pub fn new(infcx: &'cx InferCtxt<'tcx>) -> SelectionContext<'cx, 'tcx> {
         SelectionContext {
             infcx,
             freshener: infcx.freshener(),
             intercrate_ambiguity_causes: None,
             query_mode: TraitQueryMode::Standard,
+            treat_inductive_cycle: TreatInductiveCycleAs::Recur,
         }
     }
 
+    // Sets the `TreatInductiveCycleAs` mode temporarily in the selection context
+    pub fn with_treat_inductive_cycle_as<T>(
+        &mut self,
+        treat_inductive_cycle: TreatInductiveCycleAs,
+        f: impl FnOnce(&mut Self) -> T,
+    ) -> T {
+        // Should be executed in a context where caching is disabled,
+        // otherwise the cache is poisoned with the temporary result.
+        assert!(self.is_intercrate());
+        let treat_inductive_cycle =
+            std::mem::replace(&mut self.treat_inductive_cycle, treat_inductive_cycle);
+        let value = f(self);
+        self.treat_inductive_cycle = treat_inductive_cycle;
+        value
+    }
+
     pub fn with_query_mode(
         infcx: &'cx InferCtxt<'tcx>,
         query_mode: TraitQueryMode,
@@ -719,7 +759,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
                                 stack.update_reached_depth(stack_arg.1);
                                 return Ok(EvaluatedToOk);
                             } else {
-                                return Ok(EvaluatedToRecur);
+                                return Ok(self.treat_inductive_cycle.into());
                             }
                         }
                         return Ok(EvaluatedToOk);
@@ -837,7 +877,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
                             }
                         }
                         ProjectAndUnifyResult::FailedNormalization => Ok(EvaluatedToAmbig),
-                        ProjectAndUnifyResult::Recursive => Ok(EvaluatedToRecur),
+                        ProjectAndUnifyResult::Recursive => Ok(self.treat_inductive_cycle.into()),
                         ProjectAndUnifyResult::MismatchedProjectionTypes(_) => Ok(EvaluatedToErr),
                     }
                 }
@@ -1151,7 +1191,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
                 Some(EvaluatedToOk)
             } else {
                 debug!("evaluate_stack --> recursive, inductive");
-                Some(EvaluatedToRecur)
+                Some(self.treat_inductive_cycle.into())
             }
         } else {
             None

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<T>(PhantomData<T>);
+
+// This impl overlaps with the `derive` unless we reject the nested
+// `Interval<?1>: PartialOrd<Interval<?1>>` candidate which results
+// in a - currently inductive - cycle.
+impl<T, Q> PartialEq<Q> for Interval<T>
+//~^ ERROR implementations of `PartialEq<Interval<_>>` for `Interval<_>` will conflict in the future
+//~| WARN this was previously accepted by the compiler but is being phased out
+where
+    T: Borrow<Q>,
+    Q: ?Sized + PartialOrd,
+{
+    fn eq(&self, _: &Q) -> bool {
+        true
+    }
+}
+
+impl<T, Q> PartialOrd<Q> for Interval<T>
+where
+    T: Borrow<Q>,
+    Q: ?Sized + PartialOrd,
+{
+    fn partial_cmp(&self, _: &Q) -> Option<Ordering> {
+        None
+    }
+}
+
+fn main() {}

tests/ui/coherence/warn-when-cycle-is-error-in-coherence.stderr

@@ -0,0 +1,23 @@
+error: implementations of `PartialEq<Interval<_>>` for `Interval<_>` will conflict 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<T, Q> PartialEq<Q> for Interval<T>
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the first impl is here
+...
+LL |     Q: ?Sized + PartialOrd,
+   |                 ---------- `Interval<_>: PartialOrd` may be considered to hold in future releases, 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 <https://github.com/rust-lang/rust/issues/114040>
+   = note: impls that are not considered to overlap may be considered to overlap in the future
+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
+