Auto merge of rust-lang#119934 - compiler-errors:could-impl, r=jackh726

Make `InferCtxtExt::could_impl_trait` more precise, less ICEy

The implementation for `InferCtxtExt::could_impl_trait` was very wrong. Along with being pretty poorly named, way too specific to ADTs, it was also doing impl substitution wrong -- this caused an ICE (rust-lang#119915).

This PR generalizes that code, gives it a clearer name, makes it stop using the new trait solver (lol), and fixes some fallout bad suggestions that are made worse with the code fix.

Fixes rust-lang#119915

Author: bors

compiler/rustc_borrowck/src/diagnostics/mod.rs

@@ -1178,9 +1178,11 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                             } else {
                                 vec![(move_span.shrink_to_hi(), ".clone()".to_string())]
                             };
-                            if let Some(errors) =
-                                self.infcx.could_impl_trait(clone_trait, ty, self.param_env)
-                                && !has_sugg
+                            if let Some(errors) = self.infcx.type_implements_trait_shallow(
+                                clone_trait,
+                                ty,
+                                self.param_env,
+                            ) && !has_sugg
                             {
                                 let msg = match &errors[..] {
                                     [] => "you can `clone` the value and consume it, but this \

compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs

@@ -1217,19 +1217,22 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
                     {
                         match self
                             .infcx
-                            .could_impl_trait(clone_trait, ty.peel_refs(), self.param_env)
+                            .type_implements_trait_shallow(
+                                clone_trait,
+                                ty.peel_refs(),
+                                self.param_env,
+                            )
                             .as_deref()
                         {
                             Some([]) => {
-                                // The type implements Clone.
-                                err.span_help(
-                                    expr.span,
-                                    format!(
-                                        "you can `clone` the `{}` value and consume it, but this \
-                                         might not be your desired behavior",
-                                        ty.peel_refs(),
-                                    ),
-                                );
+                                // FIXME: This error message isn't useful, since we're just
+                                // vaguely suggesting to clone a value that already
+                                // implements `Clone`.
+                                //
+                                // A correct suggestion here would take into account the fact
+                                // that inference may be affected by missing types on bindings,
+                                // etc., to improve "tests/ui/borrowck/issue-91206.stderr", for
+                                // example.
                             }
                             None => {
                                 if let hir::ExprKind::MethodCall(segment, _rcvr, [], span) =

compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs

@@ -1623,7 +1623,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 );
             } else {
                 if let Some(errors) =
-                    self.could_impl_trait(clone_trait_did, expected_ty, self.param_env)
+                    self.type_implements_trait_shallow(clone_trait_did, expected_ty, self.param_env)
                 {
                     match &errors[..] {
                         [] => {}

compiler/rustc_trait_selection/src/infer.rs

@@ -1,13 +1,14 @@
-use crate::solve::FulfillmentCtxt;
 use crate::traits::query::evaluate_obligation::InferCtxtExt as _;
-use crate::traits::{self, DefiningAnchor, ObligationCtxt};
+use crate::traits::{self, DefiningAnchor, ObligationCtxt, SelectionContext};
 
+use crate::traits::TraitEngineExt as _;
 use rustc_hir::def_id::DefId;
 use rustc_hir::lang_items::LangItem;
-use rustc_infer::traits::{TraitEngine, TraitEngineExt};
+use rustc_infer::traits::{Obligation, TraitEngine, TraitEngineExt as _};
 use rustc_middle::arena::ArenaAllocatable;
 use rustc_middle::infer::canonical::{Canonical, CanonicalQueryResponse, QueryResponse};
 use rustc_middle::traits::query::NoSolution;
+use rustc_middle::traits::ObligationCause;
 use rustc_middle::ty::{self, Ty, TyCtxt, TypeFoldable, TypeVisitableExt};
 use rustc_middle::ty::{GenericArg, ToPredicate};
 use rustc_span::DUMMY_SP;
@@ -21,24 +22,36 @@ pub trait InferCtxtExt<'tcx> {
 
     fn type_is_sized_modulo_regions(&self, param_env: ty::ParamEnv<'tcx>, ty: Ty<'tcx>) -> bool;
 
-    /// Check whether a `ty` implements given trait(trait_def_id).
+    /// Check whether a `ty` implements given trait(trait_def_id) without side-effects.
+    ///
     /// The inputs are:
     ///
     /// - the def-id of the trait
     /// - the type parameters of the trait, including the self-type
     /// - 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, EvaluatedToErrStackDependent
+    /// (or EvaluatedToAmbigStackDependent) will be returned.
     fn type_implements_trait(
         &self,
         trait_def_id: DefId,
         params: impl IntoIterator<Item: Into<GenericArg<'tcx>>>,
         param_env: ty::ParamEnv<'tcx>,
     ) -> traits::EvaluationResult;
 
-    fn could_impl_trait(
+    /// Returns `Some` if a type implements a trait shallowly, without side-effects,
+    /// along with any errors that would have been reported upon further obligation
+    /// processing.
+    ///
+    /// - If this returns `Some([])`, then the trait holds modulo regions.
+    /// - If this returns `Some([errors..])`, then the trait has an impl for
+    /// the self type, but some nested obligations do not hold.
+    /// - If this returns `None`, no implementation that applies could be found.
+    ///
+    /// FIXME(-Znext-solver): Due to the recursive nature of the new solver,
+    /// this will probably only ever return `Some([])` or `None`.
+    fn type_implements_trait_shallow(
         &self,
         trait_def_id: DefId,
         ty: Ty<'tcx>,
@@ -86,64 +99,26 @@ impl<'tcx> InferCtxtExt<'tcx> for InferCtxt<'tcx> {
         self.evaluate_obligation(&obligation).unwrap_or(traits::EvaluationResult::EvaluatedToErr)
     }
 
-    fn could_impl_trait(
+    fn type_implements_trait_shallow(
         &self,
         trait_def_id: DefId,
         ty: Ty<'tcx>,
         param_env: ty::ParamEnv<'tcx>,
     ) -> Option<Vec<traits::FulfillmentError<'tcx>>> {
         self.probe(|_snapshot| {
-            if let ty::Adt(def, args) = ty.kind()
-                && let Some((impl_def_id, _)) = self
-                    .tcx
-                    .all_impls(trait_def_id)
-                    .filter_map(|impl_def_id| {
-                        self.tcx.impl_trait_ref(impl_def_id).map(|r| (impl_def_id, r))
-                    })
-                    .map(|(impl_def_id, imp)| (impl_def_id, imp.skip_binder()))
-                    .find(|(_, imp)| match imp.self_ty().peel_refs().kind() {
-                        ty::Adt(i_def, _) if i_def.did() == def.did() => true,
-                        _ => false,
-                    })
-            {
-                let mut fulfill_cx = FulfillmentCtxt::new(self);
-                // We get all obligations from the impl to talk about specific
-                // trait bounds.
-                let obligations = self
-                    .tcx
-                    .predicates_of(impl_def_id)
-                    .instantiate(self.tcx, args)
-                    .into_iter()
-                    .map(|(clause, span)| {
-                        traits::Obligation::new(
-                            self.tcx,
-                            traits::ObligationCause::dummy_with_span(span),
-                            param_env,
-                            clause,
-                        )
-                    })
-                    .collect::<Vec<_>>();
-                fulfill_cx.register_predicate_obligations(self, obligations);
-                let trait_ref = ty::TraitRef::new(self.tcx, trait_def_id, [ty]);
-                let obligation = traits::Obligation::new(
-                    self.tcx,
-                    traits::ObligationCause::dummy(),
-                    param_env,
-                    trait_ref,
-                );
-                fulfill_cx.register_predicate_obligation(self, obligation);
-                let mut errors = fulfill_cx.select_all_or_error(self);
-                // We remove the last predicate failure, which corresponds to
-                // the top-level obligation, because most of the type we only
-                // care about the other ones, *except* when it is the only one.
-                // This seems to only be relevant for arbitrary self-types.
-                // Look at `tests/ui/moves/move-fn-self-receiver.rs`.
-                if errors.len() > 1 {
-                    errors.truncate(errors.len() - 1);
+            let mut selcx = SelectionContext::new(self);
+            match selcx.select(&Obligation::new(
+                self.tcx,
+                ObligationCause::dummy(),
+                param_env,
+                ty::TraitRef::new(self.tcx, trait_def_id, [ty]),
+            )) {
+                Ok(Some(selection)) => {
+                    let mut fulfill_cx = <dyn TraitEngine<'tcx>>::new(self);
+                    fulfill_cx.register_predicate_obligations(self, selection.nested_obligations());
+                    Some(fulfill_cx.select_all_or_error(self))
                 }
-                Some(errors)
-            } else {
-                None
+                Ok(None) | Err(_) => None,
             }
         })
     }

compiler/rustc_trait_selection/src/solve/fulfill.rs

@@ -36,6 +36,11 @@ pub struct FulfillmentCtxt<'tcx> {
 
 impl<'tcx> FulfillmentCtxt<'tcx> {
     pub fn new(infcx: &InferCtxt<'tcx>) -> FulfillmentCtxt<'tcx> {
+        assert!(
+            infcx.next_trait_solver(),
+            "new trait solver fulfillment context created when \
+            infcx is set up for old trait solver"
+        );
         FulfillmentCtxt { obligations: Vec::new(), usable_in_snapshot: infcx.num_open_snapshots() }
     }
 }

compiler/rustc_trait_selection/src/traits/fulfill.rs

@@ -80,6 +80,11 @@ static_assert_size!(PendingPredicateObligation<'_>, 72);
 impl<'tcx> FulfillmentContext<'tcx> {
     /// Creates a new fulfillment context.
     pub(super) fn new(infcx: &InferCtxt<'tcx>) -> FulfillmentContext<'tcx> {
+        assert!(
+            !infcx.next_trait_solver(),
+            "old trait solver fulfillment context created when \
+            infcx is set up for new trait solver"
+        );
         FulfillmentContext {
             predicates: ObligationForest::new(),
             usable_in_snapshot: infcx.num_open_snapshots(),

tests/ui/borrowck/issue-119915-bad-clone-suggestion.rs

@@ -0,0 +1,28 @@
+use std::marker::PhantomData;
+
+struct Example<E, FakeParam>(PhantomData<(fn(E), fn(FakeParam))>);
+
+struct NoLifetime;
+struct Immutable<'a>(PhantomData<&'a ()>);
+
+impl<'a, E: 'a> Copy for Example<E, Immutable<'a>> {}
+impl<'a, E: 'a> Clone for Example<E, Immutable<'a>> {
+    fn clone(&self) -> Self {
+        *self
+    }
+}
+
+impl<E, FakeParam> Example<E, FakeParam> {
+    unsafe fn change<NewFakeParam>(self) -> Example<E, NewFakeParam> {
+        Example(PhantomData)
+    }
+}
+
+impl<E> Example<E, NoLifetime> {
+    fn the_ice(&mut self) -> Example<E, Immutable<'_>> {
+        unsafe { self.change() }
+        //~^ ERROR cannot move out of `*self` which is behind a mutable reference
+    }
+}
+
+fn main() {}

tests/ui/borrowck/issue-119915-bad-clone-suggestion.stderr

@@ -0,0 +1,17 @@
+error[E0507]: cannot move out of `*self` which is behind a mutable reference
+  --> $DIR/issue-119915-bad-clone-suggestion.rs:23:18
+   |
+LL |         unsafe { self.change() }
+   |                  ^^^^ -------- `*self` moved due to this method call
+   |                  |
+   |                  move occurs because `*self` has type `Example<E, NoLifetime>`, which does not implement the `Copy` trait
+   |
+note: `Example::<E, FakeParam>::change` takes ownership of the receiver `self`, which moves `*self`
+  --> $DIR/issue-119915-bad-clone-suggestion.rs:16:36
+   |
+LL |     unsafe fn change<NewFakeParam>(self) -> Example<E, NewFakeParam> {
+   |                                    ^^^^
+
+error: aborting due to 1 previous error
+
+For more information about this error, try `rustc --explain E0507`.

tests/ui/borrowck/issue-85765-closure.rs

@@ -3,7 +3,6 @@ fn main() {
         let mut test = Vec::new();
         let rofl: &Vec<Vec<i32>> = &mut test;
         //~^ HELP consider changing this binding's type
-        //~| HELP you can `clone` the `Vec<Vec<i32>>` value and consume it, but this might not be your desired behavior
         rofl.push(Vec::new());
         //~^ ERROR cannot borrow `*rofl` as mutable, as it is behind a `&` reference
         //~| NOTE `rofl` is a `&` reference, so the data it refers to cannot be borrowed as mutable

tests/ui/borrowck/issue-85765-closure.stderr

@@ -1,21 +1,16 @@
 error[E0596]: cannot borrow `*rofl` as mutable, as it is behind a `&` reference
-  --> $DIR/issue-85765-closure.rs:7:9
+  --> $DIR/issue-85765-closure.rs:6:9
    |
 LL |         rofl.push(Vec::new());
    |         ^^^^ `rofl` is a `&` reference, so the data it refers to cannot be borrowed as mutable
    |
-help: you can `clone` the `Vec<Vec<i32>>` value and consume it, but this might not be your desired behavior
-  --> $DIR/issue-85765-closure.rs:4:36
-   |
-LL |         let rofl: &Vec<Vec<i32>> = &mut test;
-   |                                    ^^^^^^^^^
 help: consider changing this binding's type
    |
 LL |         let rofl: &mut Vec<Vec<i32>> = &mut test;
    |                   ~~~~~~~~~~~~~~~~~~
 
 error[E0594]: cannot assign to `*r`, which is behind a `&` reference
-  --> $DIR/issue-85765-closure.rs:14:9
+  --> $DIR/issue-85765-closure.rs:13:9
    |
 LL |         *r = 0;
    |         ^^^^^^ `r` is a `&` reference, so the data it refers to cannot be written
@@ -26,7 +21,7 @@ LL |         let r = &mut mutvar;
    |                  +++
 
 error[E0594]: cannot assign to `*x`, which is behind a `&` reference
-  --> $DIR/issue-85765-closure.rs:21:9
+  --> $DIR/issue-85765-closure.rs:20:9
    |
 LL |         *x = 1;
    |         ^^^^^^ `x` is a `&` reference, so the data it refers to cannot be written
@@ -37,7 +32,7 @@ LL |         let x: &mut usize = &mut{0};
    |                ~~~~~~~~~~
 
 error[E0594]: cannot assign to `*y`, which is behind a `&` reference
-  --> $DIR/issue-85765-closure.rs:28:9
+  --> $DIR/issue-85765-closure.rs:27:9
    |
 LL |         *y = 1;
    |         ^^^^^^ `y` is a `&` reference, so the data it refers to cannot be written