about summary refs log tree commit diff
diff options
context:
space:
mode:
authorMatthias Krüger <matthias.krueger@famsik.de>2024-09-03 19:13:26 +0200
committerGitHub <noreply@github.com>2024-09-03 19:13:26 +0200
commit485fd3815c9f51ca1034f18d6c5aaca93ae40fe5 (patch)
treea2c49fae34eee2e626fa7e7239ae634d1333055c
parent2f6e85567af202e5fd9f4e20c194396806fe949f (diff)
parent6188aae369961d6328f7275c78bdf256133b7d5d (diff)
Rollup merge of #129896 - lcnr:bail-on-unknowable, r=jackh726
do not attempt to prove unknowable goals

In case a goal is unknowable, we previously still checked all other possible ways to prove this goal, even though its final result is already guaranteed to be ambiguous. By ignoring all other candidates in that case we can avoid a lot of unnecessary work, fixing the performance regression in typenum found in #121848.

This is already the behavior in the old solver. This could in theory cause future-compatability issues as considering fewer goals unknowable may end up causing performance regressions/hangs. I am quite confident that this will not be an issue.

r? ``@compiler-errors``
-rw-r--r--compiler/rustc_next_trait_solver/src/solve/assembly/mod.rs66
-rw-r--r--compiler/rustc_trait_selection/src/traits/coherence.rs138
-rw-r--r--compiler/rustc_type_ir/src/solve/mod.rs2
-rw-r--r--tests/ui/coherence/normalize-for-errors.next.stderr2
-rw-r--r--tests/ui/coherence/normalize-for-errors.rs2
5 files changed, 101 insertions, 109 deletions
diff --git a/compiler/rustc_next_trait_solver/src/solve/assembly/mod.rs b/compiler/rustc_next_trait_solver/src/solve/assembly/mod.rs
index bb05eb4c256..6c9a6011144 100644
--- a/compiler/rustc_next_trait_solver/src/solve/assembly/mod.rs
+++ b/compiler/rustc_next_trait_solver/src/solve/assembly/mod.rs
@@ -304,6 +304,11 @@ where
 
         let mut candidates = vec![];
 
+        if self.solver_mode() == SolverMode::Coherence {
+            if let Ok(candidate) = self.consider_coherence_unknowable_candidate(goal) {
+                return vec![candidate];
+            }
+        }
         self.assemble_impl_candidates(goal, &mut candidates);
 
         self.assemble_builtin_impl_candidates(goal, &mut candidates);
@@ -314,11 +319,8 @@ where
 
         self.assemble_param_env_candidates(goal, &mut candidates);
 
-        match self.solver_mode() {
-            SolverMode::Normal => self.discard_impls_shadowed_by_env(goal, &mut candidates),
-            SolverMode::Coherence => {
-                self.assemble_coherence_unknowable_candidates(goal, &mut candidates)
-            }
+        if self.solver_mode() == SolverMode::Normal {
+            self.discard_impls_shadowed_by_env(goal, &mut candidates);
         }
 
         candidates
@@ -682,38 +684,34 @@ where
     /// also consider impls which may get added in a downstream or sibling crate
     /// or which an upstream impl may add in a minor release.
     ///
-    /// To do so we add an ambiguous candidate in case such an unknown impl could
-    /// apply to the current goal.
+    /// To do so we return a single ambiguous candidate in case such an unknown
+    /// impl could apply to the current goal.
     #[instrument(level = "trace", skip_all)]
-    fn assemble_coherence_unknowable_candidates<G: GoalKind<D>>(
+    fn consider_coherence_unknowable_candidate<G: GoalKind<D>>(
         &mut self,
         goal: Goal<I, G>,
-        candidates: &mut Vec<Candidate<I>>,
-    ) {
-        let cx = self.cx();
-
-        candidates.extend(self.probe_trait_candidate(CandidateSource::CoherenceUnknowable).enter(
-            |ecx| {
-                let trait_ref = goal.predicate.trait_ref(cx);
-                if ecx.trait_ref_is_knowable(goal.param_env, trait_ref)? {
-                    Err(NoSolution)
-                } else {
-                    // While the trait bound itself may be unknowable, we may be able to
-                    // prove that a super trait is not implemented. For this, we recursively
-                    // prove the super trait bounds of the current goal.
-                    //
-                    // We skip the goal itself as that one would cycle.
-                    let predicate: I::Predicate = trait_ref.upcast(cx);
-                    ecx.add_goals(
-                        GoalSource::Misc,
-                        elaborate::elaborate(cx, [predicate])
-                            .skip(1)
-                            .map(|predicate| goal.with(cx, predicate)),
-                    );
-                    ecx.evaluate_added_goals_and_make_canonical_response(Certainty::AMBIGUOUS)
-                }
-            },
-        ))
+    ) -> Result<Candidate<I>, NoSolution> {
+        self.probe_trait_candidate(CandidateSource::CoherenceUnknowable).enter(|ecx| {
+            let cx = ecx.cx();
+            let trait_ref = goal.predicate.trait_ref(cx);
+            if ecx.trait_ref_is_knowable(goal.param_env, trait_ref)? {
+                Err(NoSolution)
+            } else {
+                // While the trait bound itself may be unknowable, we may be able to
+                // prove that a super trait is not implemented. For this, we recursively
+                // prove the super trait bounds of the current goal.
+                //
+                // We skip the goal itself as that one would cycle.
+                let predicate: I::Predicate = trait_ref.upcast(cx);
+                ecx.add_goals(
+                    GoalSource::Misc,
+                    elaborate::elaborate(cx, [predicate])
+                        .skip(1)
+                        .map(|predicate| goal.with(cx, predicate)),
+                );
+                ecx.evaluate_added_goals_and_make_canonical_response(Certainty::AMBIGUOUS)
+            }
+        })
     }
 
     /// If there's a where-bound for the current goal, do not use any impl candidates
diff --git a/compiler/rustc_trait_selection/src/traits/coherence.rs b/compiler/rustc_trait_selection/src/traits/coherence.rs
index 8558520897b..8b55f84bccc 100644
--- a/compiler/rustc_trait_selection/src/traits/coherence.rs
+++ b/compiler/rustc_trait_selection/src/traits/coherence.rs
@@ -29,6 +29,7 @@ use crate::infer::outlives::env::OutlivesEnvironment;
 use crate::infer::InferOk;
 use crate::solve::inspect::{InspectGoal, ProofTreeInferCtxtExt, ProofTreeVisitor};
 use crate::solve::{deeply_normalize_for_diagnostics, inspect};
+use crate::traits::query::evaluate_obligation::InferCtxtExt;
 use crate::traits::select::IntercrateAmbiguityCause;
 use crate::traits::{
     util, FulfillmentErrorCode, NormalizeExt, Obligation, ObligationCause, PredicateObligation,
@@ -624,14 +625,13 @@ impl<'a, 'tcx> ProofTreeVisitor<'tcx> for AmbiguityCausesVisitor<'a, 'tcx> {
         // at ambiguous goals, as for others the coherence unknowable candidate
         // was irrelevant.
         match goal.result() {
-            Ok(Certainty::Maybe(_)) => {}
             Ok(Certainty::Yes) | Err(NoSolution) => return,
+            Ok(Certainty::Maybe(_)) => {}
         }
 
-        let Goal { param_env, predicate } = goal.goal();
-
         // For bound predicates we simply call `infcx.enter_forall`
         // and then prove the resulting predicate as a nested goal.
+        let Goal { param_env, predicate } = goal.goal();
         let trait_ref = match predicate.kind().no_bound_vars() {
             Some(ty::PredicateKind::Clause(ty::ClauseKind::Trait(tr))) => tr.trait_ref,
             Some(ty::PredicateKind::Clause(ty::ClauseKind::Projection(proj)))
@@ -645,7 +645,11 @@ impl<'a, 'tcx> ProofTreeVisitor<'tcx> for AmbiguityCausesVisitor<'a, 'tcx> {
             _ => return,
         };
 
-        // Add ambiguity causes for reservation impls.
+        if trait_ref.references_error() {
+            return;
+        }
+
+        let mut candidates = goal.candidates();
         for cand in goal.candidates() {
             if let inspect::ProbeKind::TraitCandidate {
                 source: CandidateSource::Impl(def_id),
@@ -664,78 +668,68 @@ impl<'a, 'tcx> ProofTreeVisitor<'tcx> for AmbiguityCausesVisitor<'a, 'tcx> {
             }
         }
 
-        // Add ambiguity causes for unknowable goals.
-        let mut ambiguity_cause = None;
-        for cand in goal.candidates() {
-            if let inspect::ProbeKind::TraitCandidate {
-                source: CandidateSource::CoherenceUnknowable,
-                result: Ok(_),
-            } = cand.kind()
-            {
-                let lazily_normalize_ty = |mut ty: Ty<'tcx>| {
-                    if matches!(ty.kind(), ty::Alias(..)) {
-                        let ocx = ObligationCtxt::new(infcx);
-                        ty = ocx
-                            .structurally_normalize(&ObligationCause::dummy(), param_env, ty)
-                            .map_err(|_| ())?;
-                        if !ocx.select_where_possible().is_empty() {
-                            return Err(());
-                        }
-                    }
-                    Ok(ty)
-                };
+        // We also look for unknowable candidates. In case a goal is unknowable, there's
+        // always exactly 1 candidate.
+        let Some(cand) = candidates.pop() else {
+            return;
+        };
 
-                infcx.probe(|_| {
-                    match trait_ref_is_knowable(infcx, trait_ref, lazily_normalize_ty) {
-                        Err(()) => {}
-                        Ok(Ok(())) => warn!("expected an unknowable trait ref: {trait_ref:?}"),
-                        Ok(Err(conflict)) => {
-                            if !trait_ref.references_error() {
-                                // Normalize the trait ref for diagnostics, ignoring any errors if this fails.
-                                let trait_ref =
-                                    deeply_normalize_for_diagnostics(infcx, param_env, trait_ref);
-
-                                let self_ty = trait_ref.self_ty();
-                                let self_ty = self_ty.has_concrete_skeleton().then(|| self_ty);
-                                ambiguity_cause = Some(match conflict {
-                                    Conflict::Upstream => {
-                                        IntercrateAmbiguityCause::UpstreamCrateUpdate {
-                                            trait_ref,
-                                            self_ty,
-                                        }
-                                    }
-                                    Conflict::Downstream => {
-                                        IntercrateAmbiguityCause::DownstreamCrate {
-                                            trait_ref,
-                                            self_ty,
-                                        }
-                                    }
-                                });
-                            }
-                        }
-                    }
-                })
-            } else {
-                match cand.result() {
-                    // We only add an ambiguity cause if the goal would otherwise
-                    // result in an error.
-                    //
-                    // FIXME: While this matches the behavior of the
-                    // old solver, it is not the only way in which the unknowable
-                    // candidates *weaken* coherence, they can also force otherwise
-                    // successful normalization to be ambiguous.
-                    Ok(Certainty::Maybe(_) | Certainty::Yes) => {
-                        ambiguity_cause = None;
-                        break;
-                    }
-                    Err(NoSolution) => continue,
+        let inspect::ProbeKind::TraitCandidate {
+            source: CandidateSource::CoherenceUnknowable,
+            result: Ok(_),
+        } = cand.kind()
+        else {
+            return;
+        };
+
+        let lazily_normalize_ty = |mut ty: Ty<'tcx>| {
+            if matches!(ty.kind(), ty::Alias(..)) {
+                let ocx = ObligationCtxt::new(infcx);
+                ty = ocx
+                    .structurally_normalize(&ObligationCause::dummy(), param_env, ty)
+                    .map_err(|_| ())?;
+                if !ocx.select_where_possible().is_empty() {
+                    return Err(());
                 }
             }
-        }
+            Ok(ty)
+        };
 
-        if let Some(ambiguity_cause) = ambiguity_cause {
-            self.causes.insert(ambiguity_cause);
-        }
+        infcx.probe(|_| {
+            let conflict = match trait_ref_is_knowable(infcx, trait_ref, lazily_normalize_ty) {
+                Err(()) => return,
+                Ok(Ok(())) => {
+                    warn!("expected an unknowable trait ref: {trait_ref:?}");
+                    return;
+                }
+                Ok(Err(conflict)) => conflict,
+            };
+
+            // It is only relevant that a goal is unknowable if it would have otherwise
+            // failed.
+            let non_intercrate_infcx = infcx.fork_with_intercrate(false);
+            if non_intercrate_infcx.predicate_may_hold(&Obligation::new(
+                infcx.tcx,
+                ObligationCause::dummy(),
+                param_env,
+                predicate,
+            )) {
+                return;
+            }
+
+            // Normalize the trait ref for diagnostics, ignoring any errors if this fails.
+            let trait_ref = deeply_normalize_for_diagnostics(infcx, param_env, trait_ref);
+            let self_ty = trait_ref.self_ty();
+            let self_ty = self_ty.has_concrete_skeleton().then(|| self_ty);
+            self.causes.insert(match conflict {
+                Conflict::Upstream => {
+                    IntercrateAmbiguityCause::UpstreamCrateUpdate { trait_ref, self_ty }
+                }
+                Conflict::Downstream => {
+                    IntercrateAmbiguityCause::DownstreamCrate { trait_ref, self_ty }
+                }
+            });
+        });
     }
 }
 
diff --git a/compiler/rustc_type_ir/src/solve/mod.rs b/compiler/rustc_type_ir/src/solve/mod.rs
index 96c939a898b..96998d2ec9f 100644
--- a/compiler/rustc_type_ir/src/solve/mod.rs
+++ b/compiler/rustc_type_ir/src/solve/mod.rs
@@ -58,7 +58,7 @@ pub enum Reveal {
     All,
 }
 
-#[derive(Debug, Clone, Copy)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum SolverMode {
     /// Ordinary trait solving, using everywhere except for coherence.
     Normal,
diff --git a/tests/ui/coherence/normalize-for-errors.next.stderr b/tests/ui/coherence/normalize-for-errors.next.stderr
index 634a10b7a14..44952dc1944 100644
--- a/tests/ui/coherence/normalize-for-errors.next.stderr
+++ b/tests/ui/coherence/normalize-for-errors.next.stderr
@@ -7,7 +7,7 @@ LL |
 LL | impl<S: Iterator> MyTrait<S> for (Box<<(MyType,) as Mirror>::Assoc>, S::Item) {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `(Box<(MyType,)>, <_ as Iterator>::Item)`
    |
-   = note: upstream crates may add a new impl of trait `std::clone::Clone` for type `(MyType,)` in future versions
+   = note: upstream crates may add a new impl of trait `std::clone::Clone` for type `std::boxed::Box<(MyType,)>` in future versions
    = note: upstream crates may add a new impl of trait `std::marker::Copy` for type `std::boxed::Box<(MyType,)>` in future versions
 
 error: aborting due to 1 previous error
diff --git a/tests/ui/coherence/normalize-for-errors.rs b/tests/ui/coherence/normalize-for-errors.rs
index 4188389a3ad..c17bb766b5b 100644
--- a/tests/ui/coherence/normalize-for-errors.rs
+++ b/tests/ui/coherence/normalize-for-errors.rs
@@ -18,6 +18,6 @@ impl<S: Iterator> MyTrait<S> for (Box<<(MyType,) as Mirror>::Assoc>, S::Item) {}
 //~^ ERROR conflicting implementations of trait `MyTrait<_>` for type `(Box<(MyType,)>,
 //~| NOTE conflicting implementation for `(Box<(MyType,)>,
 //~| NOTE upstream crates may add a new impl of trait `std::marker::Copy` for type `std::boxed::Box<(MyType,)>` in future versions
-//[next]~| NOTE upstream crates may add a new impl of trait `std::clone::Clone` for type `(MyType,)` in future versions
+//[next]~| NOTE upstream crates may add a new impl of trait `std::clone::Clone` for type `std::boxed::Box<(MyType,)>` in future versions
 
 fn main() {}