Auto merge of rust-lang#122791 - compiler-errors:make-coinductive-alw…

…ays, r=lcnr Make inductive cycles always ambiguous This makes inductive cycles always result in ambiguity rather than be treated like a stack-dependent error. This has some interactions with specialization, and so breaks a few UI tests that I don't agree should've ever worked in the first place, and also breaks a handful of crates in a way that I don't believe is a problem. On the bright side, it puts us in a better spot when it comes to eventually enabling coinduction everywhere. ## Results This was cratered in rust-lang#116494 (comment), which boils down to two regressions: * `lu_packets` - This code should have never compiled in the first place. More below. * **ALL** other regressions are due to `[email protected]` (edit: and `[email protected]`) - This actually seems to be fixed in version `0.11.0-beta.5`, which is the *most* up to date version, but it's still prerelease on crates.io so I don't think cargo ends up picking `beta.5` when building dependent crates. ### `lu_packets` Firstly, this crate uses specialization, so I think it's automatically worth breaking. However, I've minimized [the regression](https://crater-reports.s3.amazonaws.com/pr-116494-3/try%23d614ed876e31a5f3ad1d0fbf848fcdab3a29d1d8/gh/lcdr.lu_packets/log.txt) to: ```rust // Upstream crate pub trait Serialize {} impl Serialize for &() {} impl<S> Serialize for &[S] where for<'a> &'a S: Serialize {} // ----------------------------------------------------------------------- // // Downstream crate #![feature(specialization)] #![allow(incomplete_features, unused)] use upstream::Serialize; trait Replica { fn serialize(); } impl<T> Replica for T { default fn serialize() {} } impl<T> Replica for Option<T> where for<'a> &'a T: Serialize, { fn serialize() {} } ``` Specifically this fails when computing the specialization graph for the `downstream` crate. The code ends up cycling on `&[?0]: Serialize` when we equate `&?0 = &[?1]` during impl matching, which ends up needing to prove `&[?1]: Serialize`, which since cycles are treated like ambiguity, ends up in a **fatal overflow**. For some reason this requires two crates, squashing them into one crate doesn't work. Side-note: This code is subtly order dependent. When minimizing, I ended up having the code start failing on `nightly` very easily after removing and reordering impls. This seems to me all the more reason to remove this behavior altogether. ## Side-note: Item Bounds (edit: this was fixed independently in rust-lang#121123) Due to the changes in rust-lang#120584 where we now consider an alias's item bounds *and* all the item bounds of the alias's nested self type aliases, I've had to add e6b64c6 which is a hack to make sure we're not eagerly normalizing bounds that have nothing to do with the predicate we're trying to solve, and which result in. This is fixed in a more principled way in rust-lang#121123. --- r? lcnr for an initial review
rust-lang-ci · Apr 2, 2024 · 32f1b4a · 32f1b4a
2 parents 88c2f4f + 56dbeeb
commit 32f1b4a
Show file tree

Hide file tree

Showing 12 changed files with 104 additions and 117 deletions.
diff --git a/compiler/rustc_middle/src/traits/select.rs b/compiler/rustc_middle/src/traits/select.rs
@@ -193,7 +193,6 @@ pub enum SelectionCandidate<'tcx> {
 /// The evaluation results are ordered:
 ///     - `EvaluatedToOk` implies `EvaluatedToOkModuloRegions`
 ///       implies `EvaluatedToAmbig` implies `EvaluatedToAmbigStackDependent`
-///     - `EvaluatedToErr` implies `EvaluatedToErrStackDependent`
 ///     - the "union" of evaluation results is equal to their maximum -
 ///     all the "potential success" candidates can potentially succeed,
 ///     so they are noops when unioned with a definite error, and within
@@ -219,52 +218,9 @@ pub enum EvaluationResult {
     /// variables. We are somewhat imprecise there, so we don't actually
     /// know the real result.
     ///
-    /// This can't be trivially cached for the same reason as `EvaluatedToErrStackDependent`.
+    /// This can't be trivially cached because the result depends on the
+    /// stack results.
     EvaluatedToAmbigStackDependent,
-    /// Evaluation failed because we encountered an obligation we are already
-    /// trying to prove on this branch.
-    ///
-    /// We know this branch can't be a part of a minimal proof-tree for
-    /// the "root" of our cycle, because then we could cut out the recursion
-    /// and maintain a valid proof tree. However, this does not mean
-    /// that all the obligations on this branch do not hold -- it's possible
-    /// that we entered this branch "speculatively", and that there
-    /// might be some other way to prove this obligation that does not
-    /// go through this cycle -- so we can't cache this as a failure.
-    ///
-    /// For example, suppose we have this:
-    ///
-    /// ```rust,ignore (pseudo-Rust)
-    /// pub trait Trait { fn xyz(); }
-    /// // This impl is "useless", but we can still have
-    /// // an `impl Trait for SomeUnsizedType` somewhere.
-    /// impl<T: Trait + Sized> Trait for T { fn xyz() {} }
-    ///
-    /// pub fn foo<T: Trait + ?Sized>() {
-    ///     <T as Trait>::xyz();
-    /// }
-    /// ```
-    ///
-    /// When checking `foo`, we have to prove `T: Trait`. This basically
-    /// translates into this:
-    ///
-    /// ```plain,ignore
-    /// (T: Trait + Sized →_\impl T: Trait), T: Trait ⊢ T: Trait
-    /// ```
-    ///
-    /// When we try to prove it, we first go the first option, which
-    /// recurses. This shows us that the impl is "useless" -- it won't
-    /// tell us that `T: Trait` unless it already implemented `Trait`
-    /// by some other means. However, that does not prevent `T: Trait`
-    /// does not hold, because of the bound (which can indeed be satisfied
-    /// by `SomeUnsizedType` from another crate).
-    //
-    // FIXME: when an `EvaluatedToErrStackDependent` goes past its parent root, we
-    // ought to convert it to an `EvaluatedToErr`, because we know
-    // there definitely isn't a proof tree for that obligation. Not
-    // doing so is still sound -- there isn't any proof tree, so the
-    // branch still can't be a part of a minimal one -- but does not re-enable caching.
-    EvaluatedToErrStackDependent,
     /// Evaluation failed.
     EvaluatedToErr,
 }
@@ -290,13 +246,13 @@ impl EvaluationResult {
             | EvaluatedToAmbig
             | EvaluatedToAmbigStackDependent => true,
 
-            EvaluatedToErr | EvaluatedToErrStackDependent => false,
+            EvaluatedToErr => false,
         }
     }
 
     pub fn is_stack_dependent(self) -> bool {
         match self {
-            EvaluatedToAmbigStackDependent | EvaluatedToErrStackDependent => true,
+            EvaluatedToAmbigStackDependent => true,
 
             EvaluatedToOkModuloOpaqueTypes
             | EvaluatedToOk

diff --git a/compiler/rustc_trait_selection/src/infer.rs b/compiler/rustc_trait_selection/src/infer.rs
@@ -49,8 +49,7 @@ impl<'tcx> InferCtxt<'tcx> {
     /// - the parameter environment
     ///
     /// Invokes `evaluate_obligation`, so in the event that evaluating
-    /// `Ty: Trait` causes overflow, EvaluatedToErrStackDependent
-    /// (or EvaluatedToAmbigStackDependent) will be returned.
+    /// `Ty: Trait` causes overflow, EvaluatedToAmbigStackDependent will be returned.
     #[instrument(level = "debug", skip(self, params), ret)]
     fn type_implements_trait(
         &self,

diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs
@@ -210,7 +210,7 @@ fn overlap<'tcx>(
         .intercrate(true)
         .with_next_trait_solver(tcx.next_trait_solver_in_coherence())
         .build();
-    let selcx = &mut SelectionContext::with_treat_inductive_cycle_as_ambig(&infcx);
+    let selcx = &mut SelectionContext::new(&infcx);
     if track_ambiguity_causes.is_yes() {
         selcx.enable_tracking_intercrate_ambiguity_causes();
     }

diff --git a/compiler/rustc_trait_selection/src/traits/select/mod.rs b/compiler/rustc_trait_selection/src/traits/select/mod.rs
@@ -126,8 +126,6 @@ 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.
@@ -208,47 +206,13 @@ 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 => EvaluatedToAmbigStackDependent,
-            TreatInductiveCycleAs::Recur => EvaluatedToErrStackDependent,
-        }
-    }
-}
-
 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_ambig(
-        infcx: &'cx InferCtxt<'tcx>,
-    ) -> SelectionContext<'cx, 'tcx> {
-        // Should be executed in a context where caching is disabled,
-        // otherwise the cache is poisoned with the temporary result.
-        assert!(infcx.intercrate);
-        SelectionContext {
-            treat_inductive_cycle: TreatInductiveCycleAs::Ambig,
-            ..SelectionContext::new(infcx)
         }
     }
 
@@ -756,7 +720,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
                                 stack.update_reached_depth(stack_arg.1);
                                 return Ok(EvaluatedToOk);
                             } else {
-                                return Ok(self.treat_inductive_cycle.into());
+                                return Ok(EvaluatedToAmbigStackDependent);
                             }
                         }
                         return Ok(EvaluatedToOk);
@@ -875,7 +839,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
                             }
                         }
                         ProjectAndUnifyResult::FailedNormalization => Ok(EvaluatedToAmbig),
-                        ProjectAndUnifyResult::Recursive => Ok(self.treat_inductive_cycle.into()),
+                        ProjectAndUnifyResult::Recursive => Ok(EvaluatedToAmbigStackDependent),
                         ProjectAndUnifyResult::MismatchedProjectionTypes(_) => Ok(EvaluatedToErr),
                     }
                 }
@@ -1180,7 +1144,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
                 Some(EvaluatedToOk)
             } else {
                 debug!("evaluate_stack --> recursive, inductive");
-                Some(self.treat_inductive_cycle.into())
+                Some(EvaluatedToAmbigStackDependent)
             }
         } else {
             None

diff --git a/tests/ui/marker_trait_attr/unsound-overlap.rs b/tests/ui/marker_trait_attr/unsound-overlap.rs
@@ -8,6 +8,7 @@ trait B {}
 impl<T: A> B for T {}
 impl<T: B> A for T {}
 impl A for &str {}
+//~^ ERROR type annotations needed: cannot satisfy `&str: A`
 impl<T: A + B> A for (T,) {}
 trait TraitWithAssoc {
     type Assoc;

diff --git a/tests/ui/marker_trait_attr/unsound-overlap.stderr b/tests/ui/marker_trait_attr/unsound-overlap.stderr
@@ -1,12 +1,27 @@
+error[E0283]: type annotations needed: cannot satisfy `&str: A`
+  --> $DIR/unsound-overlap.rs:10:12
+   |
+LL | impl A for &str {}
+   |            ^^^^
+   |
+note: multiple `impl`s satisfying `&str: A` found
+  --> $DIR/unsound-overlap.rs:9:1
+   |
+LL | impl<T: B> A for T {}
+   | ^^^^^^^^^^^^^^^^^^
+LL | impl A for &str {}
+   | ^^^^^^^^^^^^^^^
+
 error[E0119]: conflicting implementations of trait `TraitWithAssoc` for type `((&str,),)`
-  --> $DIR/unsound-overlap.rs:20:1
+  --> $DIR/unsound-overlap.rs:21:1
    |
 LL | impl<T: A> TraitWithAssoc for T {
    | ------------------------------- first implementation here
 ...
 LL | impl TraitWithAssoc for ((&str,),) {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `((&str,),)`
 
-error: aborting due to 1 previous error
+error: aborting due to 2 previous errors
 
-For more information about this error, try `rustc --explain E0119`.
+Some errors have detailed explanations: E0119, E0283.
+For more information about an error, try `rustc --explain E0119`.
diff --git a/tests/ui/specialization/issue-39448.rs b/tests/ui/specialization/issue-39448.rs
@@ -22,6 +22,7 @@ trait FromA<T> {
 }
 
 impl<T: A, U: A + FromA<T>> FromA<T> for U {
+    //~^ ERROR cycle detected when computing whether impls specialize one another
     default fn from(x: T) -> Self {
         ToA::to(x)
     }
@@ -42,7 +43,7 @@ where
 
 #[allow(dead_code)]
 fn foo<T: A, U: A>(x: T, y: U) -> U {
-    x.foo(y.to()).to() //~ ERROR overflow evaluating the requirement
+    x.foo(y.to()).to()
 }
 
 fn main() {

diff --git a/tests/ui/specialization/issue-39448.stderr b/tests/ui/specialization/issue-39448.stderr
@@ -8,28 +8,21 @@ LL | #![feature(specialization)]
    = help: consider using `min_specialization` instead, which is more stable and complete
    = note: `#[warn(incomplete_features)]` on by default
 
-error[E0275]: overflow evaluating the requirement `T: FromA<U>`
-  --> $DIR/issue-39448.rs:45:13
-   |
-LL |     x.foo(y.to()).to()
-   |             ^^
-   |
-note: required for `T` to implement `FromA<U>`
-  --> $DIR/issue-39448.rs:24:29
+error[E0391]: cycle detected when computing whether impls specialize one another
+  --> $DIR/issue-39448.rs:24:1
    |
 LL | impl<T: A, U: A + FromA<T>> FromA<T> for U {
-   |                   --------  ^^^^^^^^     ^
-   |                   |
-   |                   unsatisfied trait bound introduced here
-note: required for `U` to implement `ToA<T>`
-  --> $DIR/issue-39448.rs:34:12
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: ...which requires evaluating trait selection obligation `u16: FromA<u8>`...
+   = note: ...which again requires computing whether impls specialize one another, completing the cycle
+note: cycle used when building specialization graph of trait `FromA`
+  --> $DIR/issue-39448.rs:20:1
    |
-LL | impl<T, U> ToA<U> for T
-   |            ^^^^^^     ^
-LL | where
-LL |     U: FromA<T>,
-   |        -------- unsatisfied trait bound introduced here
+LL | trait FromA<T> {
+   | ^^^^^^^^^^^^^^
+   = note: see https://rustc-dev-guide.rust-lang.org/overview.html#queries and https://rustc-dev-guide.rust-lang.org/query.html for more information
 
 error: aborting due to 1 previous error; 1 warning emitted
 
-For more information about this error, try `rustc --explain E0275`.
+For more information about this error, try `rustc --explain E0391`.
diff --git a/tests/ui/specialization/issue-39618.rs b/tests/ui/specialization/issue-39618.rs
@@ -2,8 +2,6 @@
 // FIXME(JohnTitor): Centril pointed out this looks suspicions, we should revisit here.
 // More context: https://github.com/rust-lang/rust/pull/69192#discussion_r379846796
 
-//@ check-pass
-
 #![feature(specialization)] //~ WARN the feature `specialization` is incomplete
 
 trait Foo {
@@ -19,6 +17,7 @@ impl<T> Bar for T where T: Foo {
 }
 
 impl<T> Foo for T where T: Bar {
+    //~^ ERROR cycle detected when computing whether impls specialize one another
     fn foo(&self) {}
 }
 

diff --git a/tests/ui/specialization/issue-39618.stderr b/tests/ui/specialization/issue-39618.stderr
@@ -1,5 +1,5 @@
 warning: the feature `specialization` is incomplete and may not be safe to use and/or cause compiler crashes
-  --> $DIR/issue-39618.rs:7:12
+  --> $DIR/issue-39618.rs:5:12
    |
 LL | #![feature(specialization)]
    |            ^^^^^^^^^^^^^^
@@ -8,5 +8,21 @@ LL | #![feature(specialization)]
    = help: consider using `min_specialization` instead, which is more stable and complete
    = note: `#[warn(incomplete_features)]` on by default
 
-warning: 1 warning emitted
+error[E0391]: cycle detected when computing whether impls specialize one another
+  --> $DIR/issue-39618.rs:19:1
+   |
+LL | impl<T> Foo for T where T: Bar {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+   = note: ...which requires evaluating trait selection obligation `u64: Bar`...
+   = note: ...which again requires computing whether impls specialize one another, completing the cycle
+note: cycle used when building specialization graph of trait `Foo`
+  --> $DIR/issue-39618.rs:7:1
+   |
+LL | trait Foo {
+   | ^^^^^^^^^
+   = note: see https://rustc-dev-guide.rust-lang.org/overview.html#queries and https://rustc-dev-guide.rust-lang.org/query.html for more information
+
+error: aborting due to 1 previous error; 1 warning emitted
 
+For more information about this error, try `rustc --explain E0391`.
diff --git a/tests/ui/traits/stack-error-order-dependence-2.rs b/tests/ui/traits/stack-error-order-dependence-2.rs
@@ -0,0 +1,24 @@
+//@ check-pass
+// Regression test for <https://github.com/rust-lang/rust/issues/123303>.
+// This time EXCEPT without `dyn` builtin bounds :^)
+
+pub trait Trait: Supertrait {}
+
+trait Impossible {}
+impl<F: ?Sized + Impossible> Trait for F {}
+
+pub trait Supertrait {}
+
+impl<T: ?Sized + Trait + Impossible> Supertrait for T {}
+
+fn needs_supertrait<T: ?Sized + Supertrait>() {}
+fn needs_trait<T: ?Sized + Trait>() {}
+
+struct A;
+impl Trait for A where A: Supertrait {}
+impl Supertrait for A {}
+
+fn main() {
+    needs_supertrait::<A>();
+    needs_trait::<A>();
+}
diff --git a/tests/ui/traits/stack-error-order-dependence.rs b/tests/ui/traits/stack-error-order-dependence.rs
@@ -0,0 +1,19 @@
+//@ check-pass
+// Regression test for <https://github.com/rust-lang/rust/issues/123303>.
+
+pub trait Trait: Supertrait {}
+
+trait Impossible {}
+impl<F: ?Sized + Impossible> Trait for F {}
+
+pub trait Supertrait {}
+
+impl<T: ?Sized + Trait + Impossible> Supertrait for T {}
+
+fn needs_supertrait<T: ?Sized + Supertrait>() {}
+fn needs_trait<T: ?Sized + Trait>() {}
+
+fn main() {
+    needs_supertrait::<dyn Trait>();
+    needs_trait::<dyn Trait>();
+}