Implement lint against coinductive impl overlap

Author: compiler-errors

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>(T);
+    ///
+    /// impl<T, Q> PartialEq<Q> for Interval<T>
+    /// where
+    ///     Q: PartialOrd,
+    /// {
+    ///     fn eq(&self, other: &Q) -> bool {
+    ///         todo!()
+    ///     }
+    /// }
+    ///
+    /// impl<T, Q> PartialOrd<Q> for Interval<T>
+    /// where
+    ///     Q: PartialOrd,
+    /// {
+    ///     fn partial_cmp(&self, other: &Q) -> Option<Ordering> {
+    ///         todo!()
+    ///     }
+    /// }
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// The manual impl of `PartialEq` impl overlaps with the `derive`, since
+    /// if we replace `Q = Interval<T>`, then the second impl leads to a cycle:
+    /// `PartialOrd for Interval<T> where Interval<T>: 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 <https://github.com/rust-lang/rust/issues/114040>",
+    };
+}
+
 declare_lint! {
     /// The `unknown_diagnostic_attributes` lint detects unrecognized diagnostic attributes.
     ///

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 {

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,13 +200,32 @@ 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 {
             infcx,
             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

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 an inductive cycle right now.
+impl<T, Q> PartialEq<Q> for Interval<T>
+//~^ 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>,
+    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,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<T, Q> PartialEq<Q> for Interval<T>
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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 <https://github.com/rust-lang/rust/issues/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
+