Auto merge of rust-lang#103252 - lcnr:recompute_applicable_impls, r=jackh726

selection failure: recompute applicable impls

The way we currently skip errors for ambiguous trait obligations seems pretty fragile so we get some duplicate errors because of this.

Removing this info from selection errors changes this system to be closer to my image of our new trait solver and is also making it far easier to change overflow errors to be non-fatal ✨

r? types cc `@estebank`

Author: bors

compiler/rustc_middle/src/traits/mod.rs

@@ -576,9 +576,6 @@ pub enum SelectionError<'tcx> {
     /// Signaling that an error has already been emitted, to avoid
     /// multiple errors being shown.
     ErrorReporting,
-    /// Multiple applicable `impl`s where found. The `DefId`s correspond to
-    /// all the `impl`s' Items.
-    Ambiguous(Vec<DefId>),
 }
 
 /// When performing resolution, it is typically the case that there

compiler/rustc_middle/src/ty/mod.rs

@@ -2550,11 +2550,11 @@ impl<'tcx> TyCtxt<'tcx> {
 
     /// Looks up the span of `impl_did` if the impl is local; otherwise returns `Err`
     /// with the name of the crate containing the impl.
-    pub fn span_of_impl(self, impl_did: DefId) -> Result<Span, Symbol> {
-        if let Some(impl_did) = impl_did.as_local() {
-            Ok(self.def_span(impl_did))
+    pub fn span_of_impl(self, impl_def_id: DefId) -> Result<Span, Symbol> {
+        if let Some(impl_def_id) = impl_def_id.as_local() {
+            Ok(self.def_span(impl_def_id))
         } else {
-            Err(self.crate_name(impl_did.krate))
+            Err(self.crate_name(impl_def_id.krate))
         }
     }
 

compiler/rustc_trait_selection/src/traits/chalk_fulfill.rs

@@ -14,15 +14,22 @@ pub struct FulfillmentContext<'tcx> {
     obligations: FxIndexSet<PredicateObligation<'tcx>>,
 
     relationships: FxHashMap<ty::TyVid, ty::FoundRelationships>,
+
+    usable_in_snapshot: bool,
 }
 
 impl FulfillmentContext<'_> {
     pub(crate) fn new() -> Self {
         FulfillmentContext {
             obligations: FxIndexSet::default(),
             relationships: FxHashMap::default(),
+            usable_in_snapshot: false,
         }
     }
+
+    pub(crate) fn new_in_snapshot() -> Self {
+        FulfillmentContext { usable_in_snapshot: true, ..Self::new() }
+    }
 }
 
 impl<'tcx> TraitEngine<'tcx> for FulfillmentContext<'tcx> {
@@ -41,7 +48,9 @@ impl<'tcx> TraitEngine<'tcx> for FulfillmentContext<'tcx> {
         infcx: &InferCtxt<'tcx>,
         obligation: PredicateObligation<'tcx>,
     ) {
-        assert!(!infcx.is_in_snapshot());
+        if !self.usable_in_snapshot {
+            assert!(!infcx.is_in_snapshot());
+        }
         let obligation = infcx.resolve_vars_if_possible(obligation);
 
         super::relationships::update(self, infcx, &obligation);
@@ -72,7 +81,9 @@ impl<'tcx> TraitEngine<'tcx> for FulfillmentContext<'tcx> {
     }
 
     fn select_where_possible(&mut self, infcx: &InferCtxt<'tcx>) -> Vec<FulfillmentError<'tcx>> {
-        assert!(!infcx.is_in_snapshot());
+        if !self.usable_in_snapshot {
+            assert!(!infcx.is_in_snapshot());
+        }
 
         let mut errors = Vec::new();
         let mut next_round = FxIndexSet::default();

compiler/rustc_trait_selection/src/traits/engine.rs

@@ -38,7 +38,7 @@ impl<'tcx> TraitEngineExt<'tcx> for dyn TraitEngine<'tcx> {
 
     fn new_in_snapshot(tcx: TyCtxt<'tcx>) -> Box<Self> {
         if tcx.sess.opts.unstable_opts.chalk {
-            Box::new(ChalkFulfillmentContext::new())
+            Box::new(ChalkFulfillmentContext::new_in_snapshot())
         } else {
             Box::new(FulfillmentContext::new_in_snapshot())
         }
@@ -119,13 +119,10 @@ impl<'a, 'tcx> ObligationCtxt<'a, 'tcx> {
         expected: T,
         actual: T,
     ) -> Result<(), TypeError<'tcx>> {
-        match self.infcx.at(cause, param_env).eq(expected, actual) {
-            Ok(InferOk { obligations, value: () }) => {
-                self.register_obligations(obligations);
-                Ok(())
-            }
-            Err(e) => Err(e),
-        }
+        self.infcx
+            .at(cause, param_env)
+            .eq(expected, actual)
+            .map(|infer_ok| self.register_infer_ok_obligations(infer_ok))
     }
 
     pub fn sup<T: ToTrace<'tcx>>(
@@ -144,6 +141,10 @@ impl<'a, 'tcx> ObligationCtxt<'a, 'tcx> {
         }
     }
 
+    pub fn select_where_possible(&self) -> Vec<FulfillmentError<'tcx>> {
+        self.engine.borrow_mut().select_where_possible(self.infcx)
+    }
+
     pub fn select_all_or_error(&self) -> Vec<FulfillmentError<'tcx>> {
         self.engine.borrow_mut().select_all_or_error(self.infcx)
     }

compiler/rustc_trait_selection/src/traits/error_reporting/ambiguity.rs

@@ -0,0 +1,52 @@
+use rustc_hir::def_id::DefId;
+use rustc_infer::infer::InferCtxt;
+use rustc_infer::traits::{Obligation, ObligationCause, TraitObligation};
+use rustc_span::DUMMY_SP;
+
+use crate::traits::ObligationCtxt;
+
+pub fn recompute_applicable_impls<'tcx>(
+    infcx: &InferCtxt<'tcx>,
+    obligation: &TraitObligation<'tcx>,
+) -> Vec<DefId> {
+    let tcx = infcx.tcx;
+    let param_env = obligation.param_env;
+    let dummy_cause = ObligationCause::dummy();
+    let impl_may_apply = |impl_def_id| {
+        let ocx = ObligationCtxt::new_in_snapshot(infcx);
+        let placeholder_obligation =
+            infcx.replace_bound_vars_with_placeholders(obligation.predicate);
+        let obligation_trait_ref =
+            ocx.normalize(dummy_cause.clone(), param_env, placeholder_obligation.trait_ref);
+
+        let impl_substs = infcx.fresh_substs_for_item(DUMMY_SP, impl_def_id);
+        let impl_trait_ref = tcx.bound_impl_trait_ref(impl_def_id).unwrap().subst(tcx, impl_substs);
+        let impl_trait_ref = ocx.normalize(ObligationCause::dummy(), param_env, impl_trait_ref);
+
+        if let Err(_) = ocx.eq(&dummy_cause, param_env, obligation_trait_ref, impl_trait_ref) {
+            return false;
+        }
+
+        let impl_predicates = tcx.predicates_of(impl_def_id).instantiate(tcx, impl_substs);
+        ocx.register_obligations(
+            impl_predicates
+                .predicates
+                .iter()
+                .map(|&predicate| Obligation::new(dummy_cause.clone(), param_env, predicate)),
+        );
+
+        ocx.select_where_possible().is_empty()
+    };
+
+    let mut impls = Vec::new();
+    tcx.for_each_relevant_impl(
+        obligation.predicate.def_id(),
+        obligation.predicate.skip_binder().trait_ref.self_ty(),
+        |impl_def_id| {
+            if infcx.probe(move |_snapshot| impl_may_apply(impl_def_id)) {
+                impls.push(impl_def_id)
+            }
+        },
+    );
+    impls
+}

compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs

@@ -1,3 +1,4 @@
+mod ambiguity;
 pub mod on_unimplemented;
 pub mod suggestions;
 
@@ -535,15 +536,6 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
         let mut span = obligation.cause.span;
 
         let mut err = match *error {
-            SelectionError::Ambiguous(ref impls) => {
-                let mut err = self.tcx.sess.struct_span_err(
-                    obligation.cause.span,
-                    &format!("multiple applicable `impl`s for `{}`", obligation.predicate),
-                );
-                self.annotate_source_of_ambiguity(&mut err, impls, obligation.predicate);
-                err.emit();
-                return;
-            }
             SelectionError::Unimplemented => {
                 // If this obligation was generated as a result of well-formedness checking, see if we
                 // can get a better error message by performing HIR-based well-formedness checking.
@@ -2144,12 +2136,25 @@ impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
                     crate::traits::TraitQueryMode::Standard,
                 );
                 match selcx.select_from_obligation(&obligation) {
-                    Err(SelectionError::Ambiguous(impls)) if impls.len() > 1 => {
-                        self.annotate_source_of_ambiguity(&mut err, &impls, predicate);
+                    Ok(None) => {
+                        let impls = ambiguity::recompute_applicable_impls(self.infcx, &obligation);
+                        let has_non_region_infer =
+                            trait_ref.skip_binder().substs.types().any(|t| !t.is_ty_infer());
+                        // It doesn't make sense to talk about applicable impls if there are more
+                        // than a handful of them.
+                        if impls.len() > 1 && impls.len() < 5 && has_non_region_infer {
+                            self.annotate_source_of_ambiguity(&mut err, &impls, predicate);
+                        } else {
+                            if self.is_tainted_by_errors() {
+                                err.delay_as_bug();
+                                return;
+                            }
+                            err.note(&format!("cannot satisfy `{}`", predicate));
+                        }
                     }
                     _ => {
                         if self.is_tainted_by_errors() {
-                            err.cancel();
+                            err.delay_as_bug();
                             return;
                         }
                         err.note(&format!("cannot satisfy `{}`", predicate));
@@ -2441,7 +2446,6 @@ impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
                 }
             }
         }
-        let msg = format!("multiple `impl`s satisfying `{}` found", predicate);
         let mut crate_names: Vec<_> = crates.iter().map(|n| format!("`{}`", n)).collect();
         crate_names.sort();
         crate_names.dedup();
@@ -2462,13 +2466,9 @@ impl<'tcx> InferCtxtPrivExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
             err.downgrade_to_delayed_bug();
             return;
         }
-        let post = if post.len() > 4 {
-            format!(
-                ":\n{}\nand {} more",
-                post.iter().map(|p| format!("- {}", p)).take(4).collect::<Vec<_>>().join("\n"),
-                post.len() - 4,
-            )
-        } else if post.len() > 1 || (post.len() == 1 && post[0].contains('\n')) {
+
+        let msg = format!("multiple `impl`s satisfying `{}` found", predicate);
+        let post = if post.len() > 1 || (post.len() == 1 && post[0].contains('\n')) {
             format!(":\n{}", post.iter().map(|p| format!("- {}", p)).collect::<Vec<_>>().join("\n"),)
         } else if post.len() == 1 {
             format!(": `{}`", post[0])

compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs

@@ -20,7 +20,7 @@ use crate::traits;
 use crate::traits::coherence::Conflict;
 use crate::traits::query::evaluate_obligation::InferCtxtExt;
 use crate::traits::{util, SelectionResult};
-use crate::traits::{Ambiguous, ErrorReporting, Overflow, Unimplemented};
+use crate::traits::{ErrorReporting, Overflow, Unimplemented};
 
 use super::BuiltinImplConditions;
 use super::IntercrateAmbiguityCause;
@@ -200,15 +200,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
                     // and report ambiguity.
                     if i > 1 {
                         debug!("multiple matches, ambig");
-                        return Err(Ambiguous(
-                            candidates
-                                .into_iter()
-                                .filter_map(|c| match c.candidate {
-                                    SelectionCandidate::ImplCandidate(def_id) => Some(def_id),
-                                    _ => None,
-                                })
-                                .collect(),
-                        ));
+                        return Ok(None);
                     }
                 }
             }

compiler/rustc_trait_selection/src/traits/select/mod.rs

@@ -294,9 +294,6 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
                 assert!(self.query_mode == TraitQueryMode::Canonical);
                 return Err(SelectionError::Overflow(OverflowError::Canonical));
             }
-            Err(SelectionError::Ambiguous(_)) => {
-                return Ok(None);
-            }
             Err(e) => {
                 return Err(e);
             }
@@ -931,7 +928,6 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
 
         match self.candidate_from_obligation(stack) {
             Ok(Some(c)) => self.evaluate_candidate(stack, &c),
-            Err(SelectionError::Ambiguous(_)) => Ok(EvaluatedToAmbig),
             Ok(None) => Ok(EvaluatedToAmbig),
             Err(Overflow(OverflowError::Canonical)) => Err(OverflowError::Canonical),
             Err(ErrorReporting) => Err(OverflowError::ErrorReporting),

src/test/ui/error-codes/E0282.rs

@@ -1,3 +1,4 @@
 fn main() {
-    let x = "hello".chars().rev().collect(); //~ ERROR E0282
+    let x = "hello".chars().rev().collect();
+    //~^ ERROR E0282
 }

src/test/ui/error-codes/E0401.rs

@@ -8,7 +8,9 @@ fn foo<T>(x: T) {
            W: Fn()>
            (y: T) { //~ ERROR E0401
     }
-    bfnr(x); //~ ERROR type annotations needed
+    bfnr(x);
+    //~^ ERROR type annotations needed
+    //~| ERROR type annotations needed
 }