Rollup merge of #132757 - compiler-errors:yeet-check-wf, r=lcnr

Get rid of `check_opaque_type_well_formed`

Instead, replicate it by improving the span of the opaque in `check_opaque_meets_bounds`.

This has two consequences:
1. We now prefer "concrete type differs" errors, since we'll hit those first before we check the opaque is WF.
2. Spans have gotten slightly worse.

Specifically, (2.) could be improved by adding a new obligation cause that explains that the definition's environment has stronger assumptions than the declaration.

r? lcnr

compiler/rustc_borrowck/src/region_infer/opaque_types.rs

@@ -1,18 +1,14 @@
 use rustc_data_structures::fx::FxIndexMap;
 use rustc_errors::ErrorGuaranteed;
-use rustc_hir::OpaqueTyOrigin;
-use rustc_hir::def::DefKind;
 use rustc_hir::def_id::LocalDefId;
 use rustc_infer::infer::{InferCtxt, NllRegionVariableOrigin, TyCtxtInferExt as _};
-use rustc_infer::traits::{Obligation, ObligationCause};
 use rustc_macros::extension;
 use rustc_middle::ty::visit::TypeVisitableExt;
 use rustc_middle::ty::{
     self, GenericArgKind, GenericArgs, OpaqueHiddenType, OpaqueTypeKey, Ty, TyCtxt, TypeFoldable,
     TypingMode,
 };
 use rustc_span::Span;
-use rustc_trait_selection::error_reporting::InferCtxtErrorExt;
 use rustc_trait_selection::traits::ObligationCtxt;
 use tracing::{debug, instrument};
 
@@ -303,91 +299,7 @@ impl<'tcx> InferCtxt<'tcx> {
             return Ty::new_error(self.tcx, e);
         }
 
-        // `definition_ty` does not live in of the current inference context,
-        // so lets make sure that we don't accidentally misuse our current `infcx`.
-        match check_opaque_type_well_formed(
-            self.tcx,
-            self.next_trait_solver(),
-            opaque_type_key.def_id,
-            instantiated_ty.span,
-            definition_ty,
-        ) {
-            Ok(hidden_ty) => hidden_ty,
-            Err(guar) => Ty::new_error(self.tcx, guar),
-        }
-    }
-}
-
-/// This logic duplicates most of `check_opaque_meets_bounds`.
-/// FIXME(oli-obk): Also do region checks here and then consider removing
-/// `check_opaque_meets_bounds` entirely.
-fn check_opaque_type_well_formed<'tcx>(
-    tcx: TyCtxt<'tcx>,
-    next_trait_solver: bool,
-    def_id: LocalDefId,
-    definition_span: Span,
-    definition_ty: Ty<'tcx>,
-) -> Result<Ty<'tcx>, ErrorGuaranteed> {
-    // Only check this for TAIT. RPIT already supports `tests/ui/impl-trait/nested-return-type2.rs`
-    // on stable and we'd break that.
-    let opaque_ty_hir = tcx.hir().expect_opaque_ty(def_id);
-    let OpaqueTyOrigin::TyAlias { .. } = opaque_ty_hir.origin else {
-        return Ok(definition_ty);
-    };
-    let param_env = tcx.param_env(def_id);
-
-    let mut parent_def_id = def_id;
-    while tcx.def_kind(parent_def_id) == DefKind::OpaqueTy {
-        parent_def_id = tcx.local_parent(parent_def_id);
-    }
-
-    // FIXME(#132279): This should eventually use the already defined hidden types
-    // instead. Alternatively we'll entirely remove this function given we also check
-    // the opaque in `check_opaque_meets_bounds` later.
-    let infcx = tcx
-        .infer_ctxt()
-        .with_next_trait_solver(next_trait_solver)
-        .build(TypingMode::analysis_in_body(tcx, parent_def_id));
-    let ocx = ObligationCtxt::new_with_diagnostics(&infcx);
-    let identity_args = GenericArgs::identity_for_item(tcx, def_id);
-
-    // Require that the hidden type actually fulfills all the bounds of the opaque type, even without
-    // the bounds that the function supplies.
-    let opaque_ty = Ty::new_opaque(tcx, def_id.to_def_id(), identity_args);
-    ocx.eq(&ObligationCause::misc(definition_span, def_id), param_env, opaque_ty, definition_ty)
-        .map_err(|err| {
-            infcx
-                .err_ctxt()
-                .report_mismatched_types(
-                    &ObligationCause::misc(definition_span, def_id),
-                    param_env,
-                    opaque_ty,
-                    definition_ty,
-                    err,
-                )
-                .emit()
-        })?;
-
-    // Require the hidden type to be well-formed with only the generics of the opaque type.
-    // Defining use functions may have more bounds than the opaque type, which is ok, as long as the
-    // hidden type is well formed even without those bounds.
-    let predicate = ty::Binder::dummy(ty::PredicateKind::Clause(ty::ClauseKind::WellFormed(
-        definition_ty.into(),
-    )));
-    ocx.register_obligation(Obligation::misc(tcx, definition_span, def_id, param_env, predicate));
-
-    // Check that all obligations are satisfied by the implementation's
-    // version.
-    let errors = ocx.select_all_or_error();
-
-    // This is fishy, but we check it again in `check_opaque_meets_bounds`.
-    // Remove once we can prepopulate with known hidden types.
-    let _ = infcx.take_opaque_types();
-
-    if errors.is_empty() {
-        Ok(definition_ty)
-    } else {
-        Err(infcx.err_ctxt().report_fulfillment_errors(errors))
+        definition_ty
     }
 }
 

compiler/rustc_hir_analysis/src/check/check.rs

@@ -5,13 +5,14 @@ use rustc_abi::FieldIdx;
 use rustc_data_structures::unord::{UnordMap, UnordSet};
 use rustc_errors::MultiSpan;
 use rustc_errors::codes::*;
-use rustc_hir::Node;
 use rustc_hir::def::{CtorKind, DefKind};
+use rustc_hir::{Node, intravisit};
 use rustc_infer::infer::{RegionVariableOrigin, TyCtxtInferExt};
-use rustc_infer::traits::Obligation;
+use rustc_infer::traits::{Obligation, ObligationCauseCode};
 use rustc_lint_defs::builtin::{
     REPR_TRANSPARENT_EXTERNAL_PRIVATE_FIELDS, UNSUPPORTED_FN_PTR_CALLING_CONVENTIONS,
 };
+use rustc_middle::hir::nested_filter;
 use rustc_middle::middle::resolve_bound_vars::ResolvedArg;
 use rustc_middle::middle::stability::EvalResult;
 use rustc_middle::span_bug;
@@ -190,7 +191,7 @@ fn check_static_inhabited(tcx: TyCtxt<'_>, def_id: LocalDefId) {
 /// Checks that an opaque type does not contain cycles and does not use `Self` or `T::Foo`
 /// projections that would result in "inheriting lifetimes".
 fn check_opaque(tcx: TyCtxt<'_>, def_id: LocalDefId) {
-    let hir::OpaqueTy { origin, .. } = tcx.hir().expect_opaque_ty(def_id);
+    let hir::OpaqueTy { origin, .. } = *tcx.hir().expect_opaque_ty(def_id);
 
     // HACK(jynelson): trying to infer the type of `impl trait` breaks documenting
     // `async-std` (and `pub async fn` in general).
@@ -200,23 +201,20 @@ fn check_opaque(tcx: TyCtxt<'_>, def_id: LocalDefId) {
         return;
     }
 
-    let span = tcx.def_span(def_id);
-
     if tcx.type_of(def_id).instantiate_identity().references_error() {
         return;
     }
-    if check_opaque_for_cycles(tcx, def_id, span).is_err() {
+    if check_opaque_for_cycles(tcx, def_id).is_err() {
         return;
     }
 
-    let _ = check_opaque_meets_bounds(tcx, def_id, span, origin);
+    let _ = check_opaque_meets_bounds(tcx, def_id, origin);
 }
 
 /// Checks that an opaque type does not contain cycles.
 pub(super) fn check_opaque_for_cycles<'tcx>(
     tcx: TyCtxt<'tcx>,
     def_id: LocalDefId,
-    span: Span,
 ) -> Result<(), ErrorGuaranteed> {
     let args = GenericArgs::identity_for_item(tcx, def_id);
 
@@ -233,7 +231,7 @@ pub(super) fn check_opaque_for_cycles<'tcx>(
             .try_expand_impl_trait_type(def_id.to_def_id(), args, InspectCoroutineFields::No)
             .is_err()
         {
-            let reported = opaque_type_cycle_error(tcx, def_id, span);
+            let reported = opaque_type_cycle_error(tcx, def_id);
             return Err(reported);
         }
 
@@ -267,10 +265,16 @@ pub(super) fn check_opaque_for_cycles<'tcx>(
 fn check_opaque_meets_bounds<'tcx>(
     tcx: TyCtxt<'tcx>,
     def_id: LocalDefId,
-    span: Span,
-    origin: &hir::OpaqueTyOrigin<LocalDefId>,
+    origin: hir::OpaqueTyOrigin<LocalDefId>,
 ) -> Result<(), ErrorGuaranteed> {
-    let defining_use_anchor = match *origin {
+    let (span, definition_def_id) =
+        if let Some((span, def_id)) = best_definition_site_of_opaque(tcx, def_id, origin) {
+            (span, Some(def_id))
+        } else {
+            (tcx.def_span(def_id), None)
+        };
+
+    let defining_use_anchor = match origin {
         hir::OpaqueTyOrigin::FnReturn { parent, .. }
         | hir::OpaqueTyOrigin::AsyncFn { parent, .. }
         | hir::OpaqueTyOrigin::TyAlias { parent, .. } => parent,
@@ -281,7 +285,7 @@ fn check_opaque_meets_bounds<'tcx>(
     let infcx = tcx.infer_ctxt().build(TypingMode::analysis_in_body(tcx, defining_use_anchor));
     let ocx = ObligationCtxt::new_with_diagnostics(&infcx);
 
-    let args = match *origin {
+    let args = match origin {
         hir::OpaqueTyOrigin::FnReturn { parent, .. }
         | hir::OpaqueTyOrigin::AsyncFn { parent, .. }
         | hir::OpaqueTyOrigin::TyAlias { parent, .. } => GenericArgs::identity_for_item(
@@ -306,8 +310,33 @@ fn check_opaque_meets_bounds<'tcx>(
         _ => re,
     });
 
-    let misc_cause = traits::ObligationCause::misc(span, def_id);
+    // HACK: We eagerly instantiate some bounds to report better errors for them...
+    // This isn't necessary for correctness, since we register these bounds when
+    // equating the opaque below, but we should clean this up in the new solver.
+    for (predicate, pred_span) in
+        tcx.explicit_item_bounds(def_id).iter_instantiated_copied(tcx, args)
+    {
+        let predicate = predicate.fold_with(&mut BottomUpFolder {
+            tcx,
+            ty_op: |ty| if ty == opaque_ty { hidden_ty } else { ty },
+            lt_op: |lt| lt,
+            ct_op: |ct| ct,
+        });
+
+        ocx.register_obligation(Obligation::new(
+            tcx,
+            ObligationCause::new(
+                span,
+                def_id,
+                ObligationCauseCode::OpaqueTypeBound(pred_span, definition_def_id),
+            ),
+            param_env,
+            predicate,
+        ));
+    }
 
+    let misc_cause = ObligationCause::misc(span, def_id);
+    // FIXME: We should just register the item bounds here, rather than equating.
     match ocx.eq(&misc_cause, param_env, opaque_ty, hidden_ty) {
         Ok(()) => {}
         Err(ty_err) => {
@@ -364,6 +393,97 @@ fn check_opaque_meets_bounds<'tcx>(
     }
 }
 
+fn best_definition_site_of_opaque<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    opaque_def_id: LocalDefId,
+    origin: hir::OpaqueTyOrigin<LocalDefId>,
+) -> Option<(Span, LocalDefId)> {
+    struct TaitConstraintLocator<'tcx> {
+        opaque_def_id: LocalDefId,
+        tcx: TyCtxt<'tcx>,
+    }
+    impl<'tcx> TaitConstraintLocator<'tcx> {
+        fn check(&self, item_def_id: LocalDefId) -> ControlFlow<(Span, LocalDefId)> {
+            if !self.tcx.has_typeck_results(item_def_id) {
+                return ControlFlow::Continue(());
+            }
+
+            if let Some(hidden_ty) =
+                self.tcx.mir_borrowck(item_def_id).concrete_opaque_types.get(&self.opaque_def_id)
+            {
+                ControlFlow::Break((hidden_ty.span, item_def_id))
+            } else {
+                ControlFlow::Continue(())
+            }
+        }
+    }
+    impl<'tcx> intravisit::Visitor<'tcx> for TaitConstraintLocator<'tcx> {
+        type NestedFilter = nested_filter::All;
+        type Result = ControlFlow<(Span, LocalDefId)>;
+        fn nested_visit_map(&mut self) -> Self::Map {
+            self.tcx.hir()
+        }
+        fn visit_expr(&mut self, ex: &'tcx hir::Expr<'tcx>) -> Self::Result {
+            if let hir::ExprKind::Closure(closure) = ex.kind {
+                self.check(closure.def_id)?;
+            }
+            intravisit::walk_expr(self, ex)
+        }
+        fn visit_item(&mut self, it: &'tcx hir::Item<'tcx>) -> Self::Result {
+            self.check(it.owner_id.def_id)?;
+            intravisit::walk_item(self, it)
+        }
+        fn visit_impl_item(&mut self, it: &'tcx hir::ImplItem<'tcx>) -> Self::Result {
+            self.check(it.owner_id.def_id)?;
+            intravisit::walk_impl_item(self, it)
+        }
+        fn visit_trait_item(&mut self, it: &'tcx hir::TraitItem<'tcx>) -> Self::Result {
+            self.check(it.owner_id.def_id)?;
+            intravisit::walk_trait_item(self, it)
+        }
+        fn visit_foreign_item(&mut self, it: &'tcx hir::ForeignItem<'tcx>) -> Self::Result {
+            intravisit::walk_foreign_item(self, it)
+        }
+    }
+
+    let mut locator = TaitConstraintLocator { tcx, opaque_def_id };
+    match origin {
+        hir::OpaqueTyOrigin::FnReturn { parent, .. }
+        | hir::OpaqueTyOrigin::AsyncFn { parent, .. } => locator.check(parent).break_value(),
+        hir::OpaqueTyOrigin::TyAlias { parent, in_assoc_ty: true } => {
+            let impl_def_id = tcx.local_parent(parent);
+            for assoc in tcx.associated_items(impl_def_id).in_definition_order() {
+                match assoc.kind {
+                    ty::AssocKind::Const | ty::AssocKind::Fn => {
+                        if let ControlFlow::Break(span) = locator.check(assoc.def_id.expect_local())
+                        {
+                            return Some(span);
+                        }
+                    }
+                    ty::AssocKind::Type => {}
+                }
+            }
+
+            None
+        }
+        hir::OpaqueTyOrigin::TyAlias { in_assoc_ty: false, .. } => {
+            let scope = tcx.hir().get_defining_scope(tcx.local_def_id_to_hir_id(opaque_def_id));
+            let found = if scope == hir::CRATE_HIR_ID {
+                tcx.hir().walk_toplevel_module(&mut locator)
+            } else {
+                match tcx.hir_node(scope) {
+                    Node::Item(it) => locator.visit_item(it),
+                    Node::ImplItem(it) => locator.visit_impl_item(it),
+                    Node::TraitItem(it) => locator.visit_trait_item(it),
+                    Node::ForeignItem(it) => locator.visit_foreign_item(it),
+                    other => bug!("{:?} is not a valid scope for an opaque type item", other),
+                }
+            };
+            found.break_value()
+        }
+    }
+}
+
 fn sanity_check_found_hidden_type<'tcx>(
     tcx: TyCtxt<'tcx>,
     key: ty::OpaqueTypeKey<'tcx>,
@@ -1535,11 +1655,8 @@ fn check_type_alias_type_params_are_used<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalD
 ///
 /// If all the return expressions evaluate to `!`, then we explain that the error will go away
 /// after changing it. This can happen when a user uses `panic!()` or similar as a placeholder.
-fn opaque_type_cycle_error(
-    tcx: TyCtxt<'_>,
-    opaque_def_id: LocalDefId,
-    span: Span,
-) -> ErrorGuaranteed {
+fn opaque_type_cycle_error(tcx: TyCtxt<'_>, opaque_def_id: LocalDefId) -> ErrorGuaranteed {
+    let span = tcx.def_span(opaque_def_id);
     let mut err = struct_span_code_err!(tcx.dcx(), span, E0720, "cannot resolve opaque type");
 
     let mut label = false;

compiler/rustc_infer/src/infer/outlives/obligations.rs

@@ -104,6 +104,7 @@ impl<'tcx> InferCtxt<'tcx> {
             infer::RelateParamBound(cause.span, sup_type, match cause.code().peel_derives() {
                 ObligationCauseCode::WhereClause(_, span)
                 | ObligationCauseCode::WhereClauseInExpr(_, span, ..)
+                | ObligationCauseCode::OpaqueTypeBound(span, _)
                     if !span.is_dummy() =>
                 {
                     Some(*span)

compiler/rustc_middle/src/traits/mod.rs

@@ -193,6 +193,11 @@ pub enum ObligationCauseCode<'tcx> {
     /// The span corresponds to the clause.
     WhereClause(DefId, Span),
 
+    /// Represents a bound for an opaque we are checking the well-formedness of.
+    /// The def-id corresponds to a specific definition site that we found the
+    /// hidden type from, if any.
+    OpaqueTypeBound(Span, Option<LocalDefId>),
+
     /// Like `WhereClause`, but also identifies the expression
     /// which requires the `where` clause to be proven, and also
     /// identifies the index of the predicate in the `predicates_of`

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

@@ -2953,6 +2953,22 @@ impl<'a, 'tcx> TypeErrCtxt<'a, 'tcx> {
                 // We hold the `DefId` of the item introducing the obligation, but displaying it
                 // doesn't add user usable information. It always point at an associated item.
             }
+            ObligationCauseCode::OpaqueTypeBound(span, definition_def_id) => {
+                err.span_note(span, "required by a bound in an opaque type");
+                if let Some(definition_def_id) = definition_def_id
+                    // If there are any stalled coroutine obligations, then this
+                    // error may be due to that, and not because the body has more
+                    // where-clauses.
+                    && self.tcx.typeck(definition_def_id).coroutine_stalled_predicates.is_empty()
+                {
+                    // FIXME(compiler-errors): We could probably point to something
+                    // specific here if we tried hard enough...
+                    err.span_note(
+                        tcx.def_span(definition_def_id),
+                        "this definition site has more where clauses than the opaque type",
+                    );
+                }
+            }
             ObligationCauseCode::Coercion { source, target } => {
                 let source =
                     tcx.short_ty_string(self.resolve_vars_if_possible(source), long_ty_file);