Rollup merge of rust-lang#120836 - lcnr:param-env-hide-impl, r=BoxyUwU

hide impls if trait bound is proven from env

AVERT YOUR EYES `@compiler-errors`

fixes rust-lang/trait-system-refactor-initiative#76 and rust-lang/trait-system-refactor-initiative#12 (comment)

this is kinda ugly and I hate it, but I wasn't able to think of a cleaner approach for now. I am also unsure whether we have to refine this filtering later on, so by making the change pretty minimal it should be easier to improve going forward.

r? `@BoxyUwU`

Author: matthiaskrgr

Diff for: compiler/rustc_trait_selection/src/solve/assembly/mod.rs

@@ -8,7 +8,7 @@ use rustc_infer::traits::query::NoSolution;
 use rustc_infer::traits::Reveal;
 use rustc_middle::traits::solve::inspect::ProbeKind;
 use rustc_middle::traits::solve::{
-    CandidateSource, CanonicalResponse, Certainty, Goal, QueryResult,
+    CandidateSource, CanonicalResponse, Certainty, Goal, MaybeCause, QueryResult,
 };
 use rustc_middle::traits::BuiltinImplSource;
 use rustc_middle::ty::fast_reject::{SimplifiedType, TreatParams};
@@ -276,25 +276,16 @@ impl<'tcx> EvalCtxt<'_, 'tcx> {
         &mut self,
         goal: Goal<'tcx, G>,
     ) -> Vec<Candidate<'tcx>> {
-        let dummy_candidate = |this: &mut EvalCtxt<'_, 'tcx>, certainty| {
-            let source = CandidateSource::BuiltinImpl(BuiltinImplSource::Misc);
-            let result = this.evaluate_added_goals_and_make_canonical_response(certainty).unwrap();
-            let mut dummy_probe = this.inspect.new_probe();
-            dummy_probe.probe_kind(ProbeKind::TraitCandidate { source, result: Ok(result) });
-            this.inspect.finish_probe(dummy_probe);
-            vec![Candidate { source, result }]
-        };
-
         let Some(normalized_self_ty) =
             self.try_normalize_ty(goal.param_env, goal.predicate.self_ty())
         else {
             debug!("overflow while evaluating self type");
-            return dummy_candidate(self, Certainty::OVERFLOW);
+            return self.forced_ambiguity(MaybeCause::Overflow);
         };
 
         if normalized_self_ty.is_ty_var() {
             debug!("self type has been normalized to infer");
-            return dummy_candidate(self, Certainty::AMBIGUOUS);
+            return self.forced_ambiguity(MaybeCause::Ambiguity);
         }
 
         let goal =
@@ -315,11 +306,26 @@ impl<'tcx> EvalCtxt<'_, 'tcx> {
 
         self.assemble_param_env_candidates(goal, &mut candidates);
 
-        self.assemble_coherence_unknowable_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)
+            }
+        }
 
         candidates
     }
 
+    fn forced_ambiguity(&mut self, cause: MaybeCause) -> Vec<Candidate<'tcx>> {
+        let source = CandidateSource::BuiltinImpl(BuiltinImplSource::Misc);
+        let certainty = Certainty::Maybe(cause);
+        let result = self.evaluate_added_goals_and_make_canonical_response(certainty).unwrap();
+        let mut dummy_probe = self.inspect.new_probe();
+        dummy_probe.probe_kind(ProbeKind::TraitCandidate { source, result: Ok(result) });
+        self.inspect.finish_probe(dummy_probe);
+        vec![Candidate { source, result }]
+    }
+
     #[instrument(level = "debug", skip_all)]
     fn assemble_non_blanket_impl_candidates<G: GoalKind<'tcx>>(
         &mut self,
@@ -779,18 +785,19 @@ impl<'tcx> EvalCtxt<'_, 'tcx> {
         }
     }
 
+    /// In coherence we have to not only care about all impls we know about, but
+    /// 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.
     #[instrument(level = "debug", skip_all)]
     fn assemble_coherence_unknowable_candidates<G: GoalKind<'tcx>>(
         &mut self,
         goal: Goal<'tcx, G>,
         candidates: &mut Vec<Candidate<'tcx>>,
     ) {
         let tcx = self.tcx();
-        match self.solver_mode() {
-            SolverMode::Normal => return,
-            SolverMode::Coherence => {}
-        };
-
         let result = self.probe_misc_candidate("coherence unknowable").enter(|ecx| {
             let trait_ref = goal.predicate.trait_ref(tcx);
             #[derive(Debug)]
@@ -820,6 +827,51 @@ impl<'tcx> EvalCtxt<'_, 'tcx> {
         }
     }
 
+    /// If there's a where-bound for the current goal, do not use any impl candidates
+    /// to prove the current goal. Most importantly, if there is a where-bound which does
+    /// not specify any associated types, we do not allow normalizing the associated type
+    /// by using an impl, even if it would apply.
+    ///
+    ///  <https://github.com/rust-lang/trait-system-refactor-initiative/issues/76>
+    // FIXME(@lcnr): The current structure here makes me unhappy and feels ugly. idk how
+    // to improve this however. However, this should make it fairly straightforward to refine
+    // the filtering going forward, so it seems alright-ish for now.
+    fn discard_impls_shadowed_by_env<G: GoalKind<'tcx>>(
+        &mut self,
+        goal: Goal<'tcx, G>,
+        candidates: &mut Vec<Candidate<'tcx>>,
+    ) {
+        let tcx = self.tcx();
+        let trait_goal: Goal<'tcx, ty::TraitPredicate<'tcx>> =
+            goal.with(tcx, goal.predicate.trait_ref(tcx));
+        let mut trait_candidates_from_env = Vec::new();
+        self.assemble_param_env_candidates(trait_goal, &mut trait_candidates_from_env);
+        self.assemble_alias_bound_candidates(trait_goal, &mut trait_candidates_from_env);
+        if !trait_candidates_from_env.is_empty() {
+            let trait_env_result = self.merge_candidates(trait_candidates_from_env);
+            match trait_env_result.unwrap().value.certainty {
+                // If proving the trait goal succeeds by using the env,
+                // we freely drop all impl candidates.
+                //
+                // FIXME(@lcnr): It feels like this could easily hide
+                // a forced ambiguity candidate added earlier.
+                // This feels dangerous.
+                Certainty::Yes => {
+                    candidates.retain(|c| match c.source {
+                        CandidateSource::Impl(_) | CandidateSource::BuiltinImpl(_) => false,
+                        CandidateSource::ParamEnv(_) | CandidateSource::AliasBound => true,
+                    });
+                }
+                // If it is still ambiguous we instead just force the whole goal
+                // to be ambig and wait for inference constraints. See
+                // tests/ui/traits/next-solver/env-shadows-impls/ambig-env-no-shadow.rs
+                Certainty::Maybe(cause) => {
+                    *candidates = self.forced_ambiguity(cause);
+                }
+            }
+        }
+    }
+
     /// If there are multiple ways to prove a trait or projection goal, we have
     /// to somehow try to merge the candidates into one. If that fails, we return
     /// ambiguity.
@@ -832,34 +884,8 @@ impl<'tcx> EvalCtxt<'_, 'tcx> {
         let responses = candidates.iter().map(|c| c.result).collect::<Vec<_>>();
         if let Some(result) = self.try_merge_responses(&responses) {
             return Ok(result);
+        } else {
+            self.flounder(&responses)
         }
-
-        // We then check whether we should prioritize `ParamEnv` candidates.
-        //
-        // Doing so is incomplete and would therefore be unsound during coherence.
-        match self.solver_mode() {
-            SolverMode::Coherence => (),
-            // Prioritize `ParamEnv` candidates only if they do not guide inference.
-            //
-            // This is still incomplete as we may add incorrect region bounds.
-            SolverMode::Normal => {
-                let param_env_responses = candidates
-                    .iter()
-                    .filter(|c| {
-                        matches!(
-                            c.source,
-                            CandidateSource::ParamEnv(_) | CandidateSource::AliasBound
-                        )
-                    })
-                    .map(|c| c.result)
-                    .collect::<Vec<_>>();
-                if let Some(result) = self.try_merge_responses(&param_env_responses) {
-                    // We strongly prefer alias and param-env bounds here, even if they affect inference.
-                    // See https://github.com/rust-lang/trait-system-refactor-initiative/issues/11.
-                    return Ok(result);
-                }
-            }
-        }
-        self.flounder(&responses)
     }
 }

Diff for: tests/ui/traits/next-solver/cycles/fixpoint-rerun-all-cycle-heads.rs

@@ -24,6 +24,18 @@ where
 {
 }
 
+// HACK: This impls is necessary so that the impl above is well-formed.
+//
+// When checking that the impl above is well-formed we check `B<T>: Trait<'a, 'b>`
+// with the where clauses `A<T>: Trait<'a, 'b>` and `A<T> NotImplemented`. Trying to
+// use the impl itself to prove that adds region constraints as we uniquified the
+// regions in the `A<T>: Trait<'a, 'b>` where-bound. As both the impl above
+// and the impl below now apply with some constraints, we failed with ambiguity.
+impl<'a, 'b, T: ?Sized> Trait<'a, 'b> for B<T>
+where
+    A<T>: NotImplemented,
+{}
+
 // This impl directly requires 'b to be equal to 'static.
 //
 // Because of the coinductive cycle through `C<T>` it also requires

Diff for: tests/ui/traits/next-solver/cycles/fixpoint-rerun-all-cycle-heads.stderr

@@ -1,5 +1,5 @@
 error: lifetime may not live long enough
-  --> $DIR/fixpoint-rerun-all-cycle-heads.rs:47:5
+  --> $DIR/fixpoint-rerun-all-cycle-heads.rs:59:5
    |
 LL | fn check<'a, T: ?Sized>() {
    |          -- lifetime `'a` defined here

Diff for: tests/ui/traits/next-solver/env-shadows-impls/ambig-env-no-shadow.rs

@@ -0,0 +1,40 @@
+// compile-flags: -Znext-solver
+// check-pass
+
+// If a trait goal is proven using the environment, we discard
+// impl candidates when normalizing. However, in this example
+// the env candidates start as ambiguous and end up not applying,
+// so normalization should succeed later on.
+
+trait Trait<T>: Sized {
+    type Assoc: From<Self>;
+}
+
+impl<T, U> Trait<U> for T {
+    type Assoc = T;
+}
+
+fn mk_assoc<T: Trait<U>, U>(t: T, _: U) -> <T as Trait<U>>::Assoc {
+    t.into()
+}
+
+fn generic<T>(t: T) -> T
+where
+    T: Trait<u32>,
+    T: Trait<i16>,
+{
+    let u = Default::default();
+
+    // at this point we have 2 ambig env candidates
+    let ret: T = mk_assoc(t, u);
+
+    // now both env candidates don't apply, so we're now able to
+    // normalize using this impl candidates. For this to work
+    // the normalizes-to must have remained ambiguous above.
+    let _: u8 = u;
+    ret
+}
+
+fn main() {
+    assert_eq!(generic(1), 1);
+}

Diff for: tests/ui/traits/next-solver/env-shadows-impls/discard-impls-shadowed-by-env-1.rs

@@ -0,0 +1,30 @@
+// compile-flags: -Znext-solver
+// check-pass
+
+// Normalizing `<T as Trait>::TraitAssoc` in the elaborated environment
+// `[T: Trait, T: Super, <T as Super>::SuperAssoc = <T as Trait>::TraitAssoc]`
+// has a single impl candidate, which uses the environment to
+// normalize `<T as Trait>::TraitAssoc` to itself. We avoid this overflow
+// by discarding impl candidates the trait bound is proven by a where-clause.
+
+// https://github.com/rust-lang/trait-system-refactor-initiative/issues/76
+trait Super {
+    type SuperAssoc;
+}
+
+trait Trait: Super<SuperAssoc = Self::TraitAssoc> {
+    type TraitAssoc;
+}
+
+impl<T, U> Trait for T
+where
+    T: Super<SuperAssoc = U>,
+{
+    type TraitAssoc = U;
+}
+
+fn overflow<T: Trait>() {
+    let x: <T as Trait>::TraitAssoc;
+}
+
+fn main() {}

Diff for: tests/ui/traits/next-solver/env-shadows-impls/discard-impls-shadowed-by-env-2.rs

@@ -0,0 +1,29 @@
+// revisions: next current
+//[next] compile-flags: -Znext-solver
+// check-pass
+
+#![allow(warnings)]
+trait Trait<U> {
+    type Assoc;
+}
+
+impl<T> Trait<u64> for T {
+    type Assoc = T;
+}
+
+fn lazy_init<T: Trait<U>, U>() -> (T, <T as Trait<U>>::Assoc) {
+    todo!()
+}
+
+fn foo<T: Trait<u32, Assoc = T>>(x: T) {
+    // When considering impl candidates to be equally valid as env candidates
+    // this ends up being ambiguous as `U` can be both `u32´ and `u64` here.
+    //
+    // This is acceptable breakage but we should still note that it's
+    // theoretically breaking.
+    let (delayed, mut proj) = lazy_init::<_, _>();
+    proj = x;
+    let _: T = delayed;
+}
+
+fn main() {}

Diff for: tests/ui/traits/next-solver/env-shadows-impls/discard-impls-shadowed-by-env-3.rs

@@ -0,0 +1,18 @@
+// compile-flags: -Znext-solver
+// check-pass
+
+// If we normalize using the impl here the constraints from normalization and
+// trait goals can differ. This is especially bad if normalization results
+// in stronger constraints.
+trait Trait<'a> {
+    type Assoc;
+}
+
+impl<T> Trait<'static> for T {
+    type Assoc = ();
+}
+
+// normalizing requires `'a == 'static`, the trait bound does not.
+fn foo<'a, T: Trait<'a>>(_: T::Assoc) {}
+
+fn main() {}

Diff for: tests/ui/traits/next-solver/env-shadows-impls/normalizes_to_ignores_unnormalizable_candidate.rs

@@ -0,0 +1,29 @@
+// compile-flags: -Znext-solver
+
+// Checks whether the new solver is smart enough to infer `?0 = U` when solving:
+// `normalizes-to(<Vec<?0> as Trait>::Assoc, u8)`
+// with `normalizes-to(<Vec<U> as Trait>::Assoc, u8)` in the paramenv even when
+// there is a separate `Vec<T>: Trait` bound  in the paramenv.
+//
+// We currently intentionally do not guide inference this way.
+
+trait Trait {
+    type Assoc;
+}
+
+fn foo<T: Trait<Assoc = u8>>(x: T) {}
+
+fn unconstrained<T>() -> Vec<T> {
+    todo!()
+}
+
+fn bar<T, U>()
+where
+    Vec<T>: Trait,
+    Vec<U>: Trait<Assoc = u8>,
+{
+    foo(unconstrained())
+    //~^ ERROR type annotations needed
+}
+
+fn main() {}

Diff for: tests/ui/traits/next-solver/normalizes_to_ignores_unnormalizable_candidate.self_infer.stderr renamed to tests/ui/traits/next-solver/env-shadows-impls/normalizes_to_ignores_unnormalizable_candidate.stderr

@@ -1,21 +1,21 @@
 error[E0283]: type annotations needed
-  --> $DIR/normalizes_to_ignores_unnormalizable_candidate.rs:36:5
+  --> $DIR/normalizes_to_ignores_unnormalizable_candidate.rs:25:5
    |
 LL |     foo(unconstrained())
    |     ^^^ --------------- type must be known at this point
    |     |
    |     cannot infer type of the type parameter `T` declared on the function `foo`
    |
-   = note: cannot satisfy `_: Trait`
+   = note: cannot satisfy `Vec<_>: Trait`
 note: required by a bound in `foo`
-  --> $DIR/normalizes_to_ignores_unnormalizable_candidate.rs:19:11
+  --> $DIR/normalizes_to_ignores_unnormalizable_candidate.rs:14:11
    |
 LL | fn foo<T: Trait<Assoc = u8>>(x: T) {}
    |           ^^^^^^^^^^^^^^^^^ required by this bound in `foo`
 help: consider specifying the generic argument
    |
-LL |     foo::<T>(unconstrained())
-   |        +++++
+LL |     foo::<Vec<T>>(unconstrained())
+   |        ++++++++++
 
 error: aborting due to 1 previous error