Don't ICE because recomputing overflow goals during find_best_leaf_obligation causes inference side-effects

compiler/rustc_trait_selection/src/solve/fulfill.rs

@@ -244,16 +244,23 @@ fn fulfillment_error_for_no_solution<'tcx>(
 
 fn fulfillment_error_for_stalled<'tcx>(
     infcx: &InferCtxt<'tcx>,
-    obligation: PredicateObligation<'tcx>,
+    root_obligation: PredicateObligation<'tcx>,
 ) -> FulfillmentError<'tcx> {
-    let code = infcx.probe(|_| {
-        match infcx.evaluate_root_goal(obligation.clone().into(), GenerateProofTree::Never).0 {
+    let (code, refine_obligation) = infcx.probe(|_| {
+        match infcx.evaluate_root_goal(root_obligation.clone().into(), GenerateProofTree::Never).0 {
             Ok((_, Certainty::Maybe(MaybeCause::Ambiguity))) => {
-                FulfillmentErrorCode::Ambiguity { overflow: None }
-            }
-            Ok((_, Certainty::Maybe(MaybeCause::Overflow { suggest_increasing_limit }))) => {
-                FulfillmentErrorCode::Ambiguity { overflow: Some(suggest_increasing_limit) }
+                (FulfillmentErrorCode::Ambiguity { overflow: None }, true)
             }
+            Ok((_, Certainty::Maybe(MaybeCause::Overflow { suggest_increasing_limit }))) => (
+                FulfillmentErrorCode::Ambiguity { overflow: Some(suggest_increasing_limit) },
+                // Don't look into overflows because we treat overflows weirdly anyways.
+                // In `instantiate_response_discarding_overflow` we set `has_changed = false`,
+                // recomputing the goal again during `find_best_leaf_obligation` may apply
+                // inference guidance that makes other goals go from ambig -> pass, for example.
+                //
+                // FIXME: We should probably just look into overflows here.
+                false,
+            ),
             Ok((_, Certainty::Yes)) => {
                 bug!("did not expect successful goal when collecting ambiguity errors")
             }
@@ -264,9 +271,13 @@ fn fulfillment_error_for_stalled<'tcx>(
     });
 
     FulfillmentError {
-        obligation: find_best_leaf_obligation(infcx, &obligation, true),
+        obligation: if refine_obligation {
+            find_best_leaf_obligation(infcx, &root_obligation, true)
+        } else {
+            root_obligation.clone()
+        },
         code,
-        root_obligation: obligation,
+        root_obligation,
     }
 }
 
@@ -302,41 +313,50 @@ impl<'tcx> BestObligation<'tcx> {
         res
     }
 
-    /// Filter out the candidates that aren't either error or ambiguous (depending
-    /// on what we are looking for), and also throw out candidates that have no
-    /// failing WC (or higher-ranked obligations, for which there should only be
-    /// one candidate anyways -- but I digress). This most likely means that the
-    /// goal just didn't unify at all, e.g. a param candidate with an alias in it.
+    /// Filter out the candidates that aren't interesting to visit for the
+    /// purposes of reporting errors. For ambiguities, we only consider
+    /// candidates that may hold. For errors, we only consider candidates that
+    /// *don't* hold and which have impl-where clauses that also don't hold.
     fn non_trivial_candidates<'a>(
         &self,
         goal: &'a InspectGoal<'a, 'tcx>,
     ) -> Vec<InspectCandidate<'a, 'tcx>> {
-        let mut candidates = goal
-            .candidates()
-            .into_iter()
-            .filter(|candidate| match self.consider_ambiguities {
-                true => matches!(candidate.result(), Ok(Certainty::Maybe(_))),
-                false => matches!(candidate.result(), Err(_)),
-            })
-            .collect::<Vec<_>>();
-
-        // If we have >1 candidate, one may still be due to "boring" reasons, like
-        // an alias-relate that failed to hold when deeply evaluated. We really
-        // don't care about reasons like this.
-        if candidates.len() > 1 {
-            candidates.retain(|candidate| {
-                goal.infcx().probe(|_| {
-                    candidate.instantiate_nested_goals(self.span()).iter().any(|nested_goal| {
-                        matches!(
-                            nested_goal.source(),
-                            GoalSource::ImplWhereBound | GoalSource::InstantiateHigherRanked
-                        ) && match self.consider_ambiguities {
-                            true => matches!(nested_goal.result(), Ok(Certainty::Maybe(_))),
-                            false => matches!(nested_goal.result(), Err(_)),
-                        }
-                    })
-                })
-            });
+        let mut candidates = goal.candidates();
+        match self.consider_ambiguities {
+            true => {
+                // If we have an ambiguous obligation, we must consider *all* candidates
+                // that hold, or else we may guide inference causing other goals to go
+                // from ambig -> pass/fail.
+                candidates.retain(|candidate| candidate.result().is_ok());
+            }
+            false => {
+                // If we have >1 candidate, one may still be due to "boring" reasons, like
+                // an alias-relate that failed to hold when deeply evaluated. We really
+                // don't care about reasons like this.
+                if candidates.len() > 1 {
+                    candidates.retain(|candidate| {
+                        goal.infcx().probe(|_| {
+                            candidate.instantiate_nested_goals(self.span()).iter().any(
+                                |nested_goal| {
+                                    matches!(
+                                        nested_goal.source(),
+                                        GoalSource::ImplWhereBound
+                                            | GoalSource::InstantiateHigherRanked
+                                    ) && match self.consider_ambiguities {
+                                        true => {
+                                            matches!(
+                                                nested_goal.result(),
+                                                Ok(Certainty::Maybe(MaybeCause::Ambiguity))
+                                            )
+                                        }
+                                        false => matches!(nested_goal.result(), Err(_)),
+                                    }
+                                },
+                            )
+                        })
+                    });
+                }
+            }
         }
 
         candidates
@@ -401,7 +421,10 @@ impl<'tcx> ProofTreeVisitor<'tcx> for BestObligation<'tcx> {
 
             // Skip nested goals that aren't the *reason* for our goal's failure.
             match self.consider_ambiguities {
-                true if matches!(nested_goal.result(), Ok(Certainty::Maybe(_))) => {}
+                true if matches!(
+                    nested_goal.result(),
+                    Ok(Certainty::Maybe(MaybeCause::Ambiguity))
+                ) => {}
                 false if matches!(nested_goal.result(), Err(_)) => {}
                 _ => continue,
             }

tests/ui/traits/next-solver/coherence/coherence-fulfill-overflow.stderr

@@ -1,12 +1,12 @@
-error[E0119]: conflicting implementations of trait `Trait` for type `W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<_>>>>>>>>>>>>>>>>>>>>>>`
+error[E0119]: conflicting implementations of trait `Trait` for type `W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<_>>>>>>>>>>>>>>>>>>>>>`
   --> $DIR/coherence-fulfill-overflow.rs:12:1
    |
 LL | impl<T: ?Sized + TwoW> Trait for W<T> {}
    | ------------------------------------- first implementation here
 LL | impl<T: ?Sized + TwoW> Trait for T {}
-   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<_>>>>>>>>>>>>>>>>>>>>>>`
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<_>>>>>>>>>>>>>>>>>>>>>`
    |
-   = note: overflow evaluating the requirement `W<W<W<W<_>>>>: TwoW`
+   = note: overflow evaluating the requirement `W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<W<_>>>>>>>>>>>>>>>>>>>>>: TwoW`
    = help: consider increasing the recursion limit by adding a `#![recursion_limit = "20"]` attribute to your crate (`coherence_fulfill_overflow`)
 
 error: aborting due to 1 previous error

tests/ui/traits/next-solver/cycles/coinduction/fixpoint-exponential-growth.stderr

@@ -1,16 +1,9 @@
-error[E0275]: overflow evaluating the requirement `_: Sized`
+error[E0275]: overflow evaluating the requirement `W<_>: Trait`
   --> $DIR/fixpoint-exponential-growth.rs:33:13
    |
 LL |     impls::<W<_>>();
    |             ^^^^
    |
-note: required for `W<(W<_>, W<_>)>` to implement `Trait`
-  --> $DIR/fixpoint-exponential-growth.rs:23:12
-   |
-LL | impl<T, U> Trait for W<(W<T>, W<U>)>
-   |      -     ^^^^^     ^^^^^^^^^^^^^^^
-   |      |
-   |      unsatisfied trait bound introduced here
 note: required by a bound in `impls`
   --> $DIR/fixpoint-exponential-growth.rs:30:13
    |

tests/ui/traits/next-solver/cycles/double-cycle-inductive-coinductive.stderr

@@ -1,53 +1,21 @@
-error[E0275]: overflow evaluating the requirement `(): Inductive`
+error[E0275]: overflow evaluating the requirement `(): Trait`
   --> $DIR/double-cycle-inductive-coinductive.rs:32:19
    |
 LL |     impls_trait::<()>();
    |                   ^^
    |
-note: required for `()` to implement `Trait`
-  --> $DIR/double-cycle-inductive-coinductive.rs:9:34
-   |
-LL | impl<T: Inductive + Coinductive> Trait for T {}
-   |         ---------                ^^^^^     ^
-   |         |
-   |         unsatisfied trait bound introduced here
-note: required for `()` to implement `Inductive`
-  --> $DIR/double-cycle-inductive-coinductive.rs:12:16
-   |
-LL | impl<T: Trait> Inductive for T {}
-   |         -----  ^^^^^^^^^     ^
-   |         |
-   |         unsatisfied trait bound introduced here
-   = note: 7 redundant requirements hidden
-   = note: required for `()` to implement `Trait`
 note: required by a bound in `impls_trait`
   --> $DIR/double-cycle-inductive-coinductive.rs:17:19
    |
 LL | fn impls_trait<T: Trait>() {}
    |                   ^^^^^ required by this bound in `impls_trait`
 
-error[E0275]: overflow evaluating the requirement `(): CoinductiveRev`
+error[E0275]: overflow evaluating the requirement `(): TraitRev`
   --> $DIR/double-cycle-inductive-coinductive.rs:35:23
    |
 LL |     impls_trait_rev::<()>();
    |                       ^^
    |
-note: required for `()` to implement `TraitRev`
-  --> $DIR/double-cycle-inductive-coinductive.rs:21:40
-   |
-LL | impl<T: CoinductiveRev + InductiveRev> TraitRev for T {}
-   |         --------------                 ^^^^^^^^     ^
-   |         |
-   |         unsatisfied trait bound introduced here
-note: required for `()` to implement `CoinductiveRev`
-  --> $DIR/double-cycle-inductive-coinductive.rs:27:19
-   |
-LL | impl<T: TraitRev> CoinductiveRev for T {}
-   |         --------  ^^^^^^^^^^^^^^     ^
-   |         |
-   |         unsatisfied trait bound introduced here
-   = note: 7 redundant requirements hidden
-   = note: required for `()` to implement `TraitRev`
 note: required by a bound in `impls_trait_rev`
   --> $DIR/double-cycle-inductive-coinductive.rs:29:23
    |

tests/ui/traits/next-solver/cycles/inductive-fixpoint-hang.stderr

@@ -1,19 +1,9 @@
-error[E0275]: overflow evaluating the requirement `W<W<_>>: Trait`
+error[E0275]: overflow evaluating the requirement `W<_>: Trait`
   --> $DIR/inductive-fixpoint-hang.rs:31:19
    |
 LL |     impls_trait::<W<_>>();
    |                   ^^^^
    |
-note: required for `W<W<W<_>>>` to implement `Trait`
-  --> $DIR/inductive-fixpoint-hang.rs:22:17
-   |
-LL | impl<T: ?Sized> Trait for W<W<T>>
-   |                 ^^^^^     ^^^^^^^
-LL | where
-LL |     W<T>: Trait,
-   |           ----- unsatisfied trait bound introduced here
-   = note: 8 redundant requirements hidden
-   = note: required for `W<W<W<W<W<W<W<W<W<W<W<_>>>>>>>>>>>` to implement `Trait`
 note: required by a bound in `impls_trait`
   --> $DIR/inductive-fixpoint-hang.rs:28:19
    |

tests/ui/traits/next-solver/cycles/inductive-not-on-stack.rs

@@ -39,7 +39,7 @@ fn impls_ar<T: AR>() {}
 
 fn main() {
     impls_a::<()>();
-    //~^ ERROR overflow evaluating the requirement `(): B`
+    //~^ ERROR overflow evaluating the requirement `(): A`
 
     impls_ar::<()>();
     //~^ ERROR overflow evaluating the requirement `(): AR`

tests/ui/traits/next-solver/cycles/inductive-not-on-stack.stderr

@@ -1,25 +1,9 @@
-error[E0275]: overflow evaluating the requirement `(): B`
+error[E0275]: overflow evaluating the requirement `(): A`
   --> $DIR/inductive-not-on-stack.rs:41:15
    |
 LL |     impls_a::<()>();
    |               ^^
    |
-note: required for `()` to implement `A`
-  --> $DIR/inductive-not-on-stack.rs:21:16
-   |
-LL | impl<T: B + C> A for T {}
-   |         -      ^     ^
-   |         |
-   |         unsatisfied trait bound introduced here
-note: required for `()` to implement `B`
-  --> $DIR/inductive-not-on-stack.rs:22:12
-   |
-LL | impl<T: A> B for T {}
-   |         -  ^     ^
-   |         |
-   |         unsatisfied trait bound introduced here
-   = note: 7 redundant requirements hidden
-   = note: required for `()` to implement `A`
 note: required by a bound in `impls_a`
   --> $DIR/inductive-not-on-stack.rs:25:15
    |
@@ -32,29 +16,6 @@ error[E0275]: overflow evaluating the requirement `(): AR`
 LL |     impls_ar::<()>();
    |                ^^
    |
-note: required for `()` to implement `BR`
-  --> $DIR/inductive-not-on-stack.rs:35:13
-   |
-LL | impl<T: AR> BR for T {}
-   |         --  ^^     ^
-   |         |
-   |         unsatisfied trait bound introduced here
-note: required for `()` to implement `CR`
-  --> $DIR/inductive-not-on-stack.rs:36:13
-   |
-LL | impl<T: BR> CR for T {}
-   |         --  ^^     ^
-   |         |
-   |         unsatisfied trait bound introduced here
-note: required for `()` to implement `AR`
-  --> $DIR/inductive-not-on-stack.rs:34:18
-   |
-LL | impl<T: CR + BR> AR for T {}
-   |         --       ^^     ^
-   |         |
-   |         unsatisfied trait bound introduced here
-   = note: 6 redundant requirements hidden
-   = note: required for `()` to implement `AR`
 note: required by a bound in `impls_ar`
   --> $DIR/inductive-not-on-stack.rs:38:16
    |

tests/ui/traits/next-solver/cycles/mixed-cycles-1.rs

@@ -35,5 +35,5 @@ fn impls_a<T: A>() {}
 
 fn main() {
     impls_a::<()>();
-    //~^ ERROR overflow evaluating the requirement `(): CInd`
+    //~^ ERROR overflow evaluating the requirement `(): A`
 }

tests/ui/traits/next-solver/cycles/mixed-cycles-1.stderr

@@ -1,46 +1,9 @@
-error[E0275]: overflow evaluating the requirement `(): CInd`
+error[E0275]: overflow evaluating the requirement `(): A`
   --> $DIR/mixed-cycles-1.rs:37:15
    |
 LL |     impls_a::<()>();
    |               ^^
    |
-note: required for `()` to implement `B`
-  --> $DIR/mixed-cycles-1.rs:31:28
-   |
-LL | impl<T: ?Sized + CInd + C> B for T {}
-   |                  ----      ^     ^
-   |                  |
-   |                  unsatisfied trait bound introduced here
-note: required for `()` to implement `C`
-  --> $DIR/mixed-cycles-1.rs:32:25
-   |
-LL | impl<T: ?Sized + B + A> C for T {}
-   |                  -      ^     ^
-   |                  |
-   |                  unsatisfied trait bound introduced here
-note: required for `()` to implement `CInd`
-  --> $DIR/mixed-cycles-1.rs:28:21
-   |
-LL | impl<T: ?Sized + C> CInd for T {}
-   |                  -  ^^^^     ^
-   |                  |
-   |                  unsatisfied trait bound introduced here
-   = note: 4 redundant requirements hidden
-   = note: required for `()` to implement `B`
-note: required for `()` to implement `BInd`
-  --> $DIR/mixed-cycles-1.rs:23:21
-   |
-LL | impl<T: ?Sized + B> BInd for T {}
-   |                  -  ^^^^     ^
-   |                  |
-   |                  unsatisfied trait bound introduced here
-note: required for `()` to implement `A`
-  --> $DIR/mixed-cycles-1.rs:30:28
-   |
-LL | impl<T: ?Sized + BInd + C> A for T {}
-   |                  ----      ^     ^
-   |                  |
-   |                  unsatisfied trait bound introduced here
 note: required by a bound in `impls_a`
   --> $DIR/mixed-cycles-1.rs:34:15
    |

tests/ui/traits/next-solver/cycles/mixed-cycles-2.rs

@@ -28,5 +28,5 @@ fn impls_a<T: A>() {}
 
 fn main() {
     impls_a::<()>();
-    //~^ ERROR overflow evaluating the requirement `(): BInd`
+    //~^ ERROR overflow evaluating the requirement `(): A`
 }