hide impls if trait bound is proven from env

Author: lcnr

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 =
@@ -317,9 +308,21 @@ impl<'tcx> EvalCtxt<'_, 'tcx> {
 
         self.assemble_coherence_unknowable_candidates(goal, &mut candidates);
 
+        self.discard_impls_shadowed_by_env(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,
@@ -826,6 +829,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.
@@ -838,34 +886,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)
     }
 }

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

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

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

@@ -0,0 +1,29 @@
+// compile-flags: -Znext-solver
+// check-pass
+
+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();
+    let ret: T = mk_assoc(t, u);
+    let _: u8 = u;
+    ret
+}
+
+fn main() {
+    assert_eq!(generic(1), 1);
+}

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() {}

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() {}

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() {}

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() {}

tests/ui/traits/next-solver/normalizes_to_ignores_unnormalizable_candidate.self_infer.stderr 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
 

tests/ui/traits/next-solver/env-shadows-impls/param-candidate-shadows-project.rs

@@ -0,0 +1,31 @@
+// compile-flags: -Znext-solver
+
+trait Foo {
+    type Assoc;
+}
+
+trait Bar {}
+
+impl<T> Foo for T {
+    type Assoc = i32;
+}
+
+impl<T> Bar for T where T: Foo<Assoc = i32> {}
+
+fn require_bar<T: Bar>() {}
+
+fn foo<T: Foo>() {
+    // Unlike the classic solver, the new solver previous projected
+    // `<T as Foo>::Assoc = _` down to `i32` even though there's a param-env
+    // candidate here, since we don't assemble any param-env projection
+    // candidates for `T: Foo` alone.
+    //
+    // However, allowing impl candidates shadowed by env candidates results
+    // in multiple issues, so we explicitly hide them, e.g.
+    //
+    //     https://github.com/rust-lang/trait-system-refactor-initiative/issues/76
+    require_bar::<T>();
+    //~^ ERROR the trait bound `T: Bar` is not satisfied
+}
+
+fn main() {}

tests/ui/traits/next-solver/env-shadows-impls/param-candidate-shadows-project.stderr

@@ -0,0 +1,19 @@
+error[E0277]: the trait bound `T: Bar` is not satisfied
+  --> $DIR/param-candidate-shadows-project.rs:27:19
+   |
+LL |     require_bar::<T>();
+   |                   ^ the trait `Bar` is not implemented for `T`
+   |
+note: required by a bound in `require_bar`
+  --> $DIR/param-candidate-shadows-project.rs:15:19
+   |
+LL | fn require_bar<T: Bar>() {}
+   |                   ^^^ required by this bound in `require_bar`
+help: consider further restricting this bound
+   |
+LL | fn foo<T: Foo + Bar>() {
+   |               +++++
+
+error: aborting due to 1 previous error
+
+For more information about this error, try `rustc --explain E0277`.