Auto merge of #83898 - Aaron1011:feature/hir-wf, r=estebank

Add initial implementation of HIR-based WF checking for diagnostics

During well-formed checking, we walk through all types 'nested' in
generic arguments. For example, WF-checking `Option<MyStruct<u8>>`
will cause us to check `MyStruct<u8>` and `u8`. However, this is done
on a `rustc_middle::ty::Ty`, which has no span information. As a result,
any errors that occur will have a very general span (e.g. the
definintion of an associated item).

This becomes a problem when macros are involved. In general, an
associated type like `type MyType = Option<MyStruct<u8>>;` may
have completely different spans for each nested type in the HIR. Using
the span of the entire associated item might end up pointing to a macro
invocation, even though a user-provided span is available in one of the
nested types.

This PR adds a framework for HIR-based well formed checking. This check
is only run during error reporting, and is used to obtain a more precise
span for an existing error. This is accomplished by individually
checking each 'nested' type in the HIR for the type, allowing us to
find the most-specific type (and span) that produces a given error.

The majority of the changes are to the error-reporting code. However,
some of the general trait code is modified to pass through more
information.

Since this has no soundness implications, I've implemented a minimal
version to begin with, which can be extended over time. In particular,
this only works for HIR items with a corresponding `DefId` (e.g. it will
not work for WF-checking performed within function bodies).

Author: bors

compiler/rustc_infer/src/traits/mod.rs

@@ -70,6 +70,10 @@ pub struct FulfillmentError<'tcx> {
     /// obligation error caused by a call argument. When this is the case, we also signal that in
     /// this field to ensure accuracy of suggestions.
     pub points_at_arg_span: bool,
+    /// Diagnostics only: the 'root' obligation which resulted in
+    /// the failure to process `obligation`. This is the obligation
+    /// that was initially passed to `register_predicate_obligation`
+    pub root_obligation: PredicateObligation<'tcx>,
 }
 
 #[derive(Clone)]
@@ -122,8 +126,9 @@ impl<'tcx> FulfillmentError<'tcx> {
     pub fn new(
         obligation: PredicateObligation<'tcx>,
         code: FulfillmentErrorCode<'tcx>,
+        root_obligation: PredicateObligation<'tcx>,
     ) -> FulfillmentError<'tcx> {
-        FulfillmentError { obligation, code, points_at_arg_span: false }
+        FulfillmentError { obligation, code, points_at_arg_span: false, root_obligation }
     }
 }
 

compiler/rustc_middle/src/query/mod.rs

@@ -1713,4 +1713,18 @@ rustc_queries! {
     query limits(key: ()) -> Limits {
         desc { "looking up limits" }
     }
+
+    /// Performs an HIR-based well-formed check on the item with the given `HirId`. If
+    /// we get an `Umimplemented` error that matches the provided `Predicate`, return
+    /// the cause of the newly created obligation.
+    ///
+    /// This is only used by error-reporting code to get a better cause (in particular, a better
+    /// span) for an *existing* error. Therefore, it is best-effort, and may never handle
+    /// all of the cases that the normal `ty::Ty`-based wfcheck does. This is fine,
+    /// because the `ty::Ty`-based wfcheck is always run.
+    query diagnostic_hir_wf_check(key: (ty::Predicate<'tcx>, hir::HirId)) -> Option<traits::ObligationCause<'tcx>> {
+        eval_always
+        no_hash
+        desc { "performing HIR wf-checking for predicate {:?} at item {:?}", key.0, key.1 }
+    }
 }

compiler/rustc_middle/src/traits/mod.rs

@@ -13,6 +13,7 @@ use crate::mir::abstract_const::NotConstEvaluatable;
 use crate::ty::subst::SubstsRef;
 use crate::ty::{self, AdtKind, Ty, TyCtxt};
 
+use rustc_data_structures::sync::Lrc;
 use rustc_errors::{Applicability, DiagnosticBuilder};
 use rustc_hir as hir;
 use rustc_hir::def_id::DefId;
@@ -24,7 +25,6 @@ use smallvec::SmallVec;
 use std::borrow::Cow;
 use std::fmt;
 use std::ops::Deref;
-use std::rc::Rc;
 
 pub use self::select::{EvaluationCache, EvaluationResult, OverflowError, SelectionCache};
 
@@ -87,7 +87,7 @@ pub enum Reveal {
 #[derive(Clone, PartialEq, Eq, Hash, Lift)]
 pub struct ObligationCause<'tcx> {
     /// `None` for `ObligationCause::dummy`, `Some` otherwise.
-    data: Option<Rc<ObligationCauseData<'tcx>>>,
+    data: Option<Lrc<ObligationCauseData<'tcx>>>,
 }
 
 const DUMMY_OBLIGATION_CAUSE_DATA: ObligationCauseData<'static> =
@@ -131,7 +131,7 @@ impl<'tcx> ObligationCause<'tcx> {
         body_id: hir::HirId,
         code: ObligationCauseCode<'tcx>,
     ) -> ObligationCause<'tcx> {
-        ObligationCause { data: Some(Rc::new(ObligationCauseData { span, body_id, code })) }
+        ObligationCause { data: Some(Lrc::new(ObligationCauseData { span, body_id, code })) }
     }
 
     pub fn misc(span: Span, body_id: hir::HirId) -> ObligationCause<'tcx> {
@@ -148,7 +148,7 @@ impl<'tcx> ObligationCause<'tcx> {
     }
 
     pub fn make_mut(&mut self) -> &mut ObligationCauseData<'tcx> {
-        Rc::make_mut(self.data.get_or_insert_with(|| Rc::new(DUMMY_OBLIGATION_CAUSE_DATA)))
+        Lrc::make_mut(self.data.get_or_insert_with(|| Lrc::new(DUMMY_OBLIGATION_CAUSE_DATA)))
     }
 
     pub fn span(&self, tcx: TyCtxt<'tcx>) -> Span {
@@ -326,6 +326,13 @@ pub enum ObligationCauseCode<'tcx> {
 
     /// If `X` is the concrete type of an opaque type `impl Y`, then `X` must implement `Y`
     OpaqueType,
+
+    /// Well-formed checking. If a `HirId` is provided,
+    /// it is used to perform HIR-based wf checking if an error
+    /// occurs, in order to generate a more precise error message.
+    /// This is purely for diagnostic purposes - it is always
+    /// correct to use `MiscObligation` instead
+    WellFormed(Option<hir::HirId>),
 }
 
 impl ObligationCauseCode<'_> {
@@ -389,7 +396,7 @@ pub struct DerivedObligationCause<'tcx> {
     pub parent_trait_ref: ty::PolyTraitRef<'tcx>,
 
     /// The parent trait had this cause.
-    pub parent_code: Rc<ObligationCauseCode<'tcx>>,
+    pub parent_code: Lrc<ObligationCauseCode<'tcx>>,
 }
 
 #[derive(Clone, Debug, TypeFoldable, Lift)]

compiler/rustc_mir/src/borrow_check/type_check/mod.rs

@@ -2070,24 +2070,26 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
                                 debug!("check_rvalue: is_const_fn={:?}", is_const_fn);
 
                                 let def_id = body.source.def_id().expect_local();
-                                self.infcx.report_selection_error(
-                                    &traits::Obligation::new(
-                                        ObligationCause::new(
-                                            span,
-                                            self.tcx().hir().local_def_id_to_hir_id(def_id),
-                                            traits::ObligationCauseCode::RepeatVec(is_const_fn),
-                                        ),
-                                        self.param_env,
-                                        ty::Binder::dummy(ty::TraitRef::new(
-                                            self.tcx().require_lang_item(
-                                                LangItem::Copy,
-                                                Some(self.last_span),
-                                            ),
-                                            tcx.mk_substs_trait(ty, &[]),
-                                        ))
-                                        .without_const()
-                                        .to_predicate(self.tcx()),
+                                let obligation = traits::Obligation::new(
+                                    ObligationCause::new(
+                                        span,
+                                        self.tcx().hir().local_def_id_to_hir_id(def_id),
+                                        traits::ObligationCauseCode::RepeatVec(is_const_fn),
                                     ),
+                                    self.param_env,
+                                    ty::Binder::dummy(ty::TraitRef::new(
+                                        self.tcx().require_lang_item(
+                                            LangItem::Copy,
+                                            Some(self.last_span),
+                                        ),
+                                        tcx.mk_substs_trait(ty, &[]),
+                                    ))
+                                    .without_const()
+                                    .to_predicate(self.tcx()),
+                                );
+                                self.infcx.report_selection_error(
+                                    obligation.clone(),
+                                    &obligation,
                                     &traits::SelectionError::Unimplemented,
                                     false,
                                     false,

compiler/rustc_query_impl/src/keys.rs

@@ -1,6 +1,7 @@
 //! Defines the set of legal keys that can be used in queries.
 
 use rustc_hir::def_id::{CrateNum, DefId, LocalDefId, LOCAL_CRATE};
+use rustc_hir::HirId;
 use rustc_middle::infer::canonical::Canonical;
 use rustc_middle::mir;
 use rustc_middle::ty::fast_reject::SimplifiedType;
@@ -395,3 +396,14 @@ impl<'tcx> Key for (DefId, Ty<'tcx>, SubstsRef<'tcx>, ty::ParamEnv<'tcx>) {
         DUMMY_SP
     }
 }
+
+impl<'tcx> Key for (ty::Predicate<'tcx>, HirId) {
+    #[inline(always)]
+    fn query_crate_is_local(&self) -> bool {
+        true
+    }
+
+    fn default_span(&self, _tcx: TyCtxt<'_>) -> Span {
+        DUMMY_SP
+    }
+}

compiler/rustc_trait_selection/src/infer.rs

@@ -30,8 +30,7 @@ pub trait InferCtxtExt<'tcx> {
 
     fn partially_normalize_associated_types_in<T>(
         &self,
-        span: Span,
-        body_id: hir::HirId,
+        cause: ObligationCause<'tcx>,
         param_env: ty::ParamEnv<'tcx>,
         value: T,
     ) -> InferOk<'tcx, T>
@@ -79,8 +78,7 @@ impl<'cx, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'cx, 'tcx> {
     /// new obligations that must further be processed.
     fn partially_normalize_associated_types_in<T>(
         &self,
-        span: Span,
-        body_id: hir::HirId,
+        cause: ObligationCause<'tcx>,
         param_env: ty::ParamEnv<'tcx>,
         value: T,
     ) -> InferOk<'tcx, T>
@@ -89,7 +87,6 @@ impl<'cx, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'cx, 'tcx> {
     {
         debug!("partially_normalize_associated_types_in(value={:?})", value);
         let mut selcx = traits::SelectionContext::new(self);
-        let cause = ObligationCause::misc(span, body_id);
         let traits::Normalized { value, obligations } =
             traits::normalize(&mut selcx, param_env, cause, value);
         debug!(

compiler/rustc_trait_selection/src/opaque_types.rs

@@ -1,5 +1,5 @@
 use crate::infer::InferCtxtExt as _;
-use crate::traits::{self, PredicateObligation};
+use crate::traits::{self, ObligationCause, PredicateObligation};
 use rustc_data_structures::fx::FxHashMap;
 use rustc_data_structures::sync::Lrc;
 use rustc_data_structures::vec_map::VecMap;
@@ -1049,8 +1049,11 @@ impl<'a, 'tcx> Instantiator<'a, 'tcx> {
             item_bounds.iter().map(|(bound, _)| bound.subst(tcx, substs)).collect();
 
         let param_env = tcx.param_env(def_id);
-        let InferOk { value: bounds, obligations } =
-            infcx.partially_normalize_associated_types_in(span, self.body_id, param_env, bounds);
+        let InferOk { value: bounds, obligations } = infcx.partially_normalize_associated_types_in(
+            ObligationCause::misc(span, self.body_id),
+            param_env,
+            bounds,
+        );
         self.obligations.extend(obligations);
 
         debug!("instantiate_opaque_types: bounds={:?}", bounds);

compiler/rustc_trait_selection/src/traits/chalk_fulfill.rs

@@ -58,6 +58,9 @@ impl TraitEngine<'tcx> for FulfillmentContext<'tcx> {
                     obligation: obligation.clone(),
                     code: FulfillmentErrorCode::CodeAmbiguity,
                     points_at_arg_span: false,
+                    // FIXME - does Chalk have a notation of 'root obligation'?
+                    // This is just for diagnostics, so it's okay if this is wrong
+                    root_obligation: obligation.clone(),
                 })
                 .collect();
             Err(errors)
@@ -105,11 +108,14 @@ impl TraitEngine<'tcx> for FulfillmentContext<'tcx> {
                                 ),
 
                                 Err(_err) => errors.push(FulfillmentError {
-                                    obligation,
+                                    obligation: obligation.clone(),
                                     code: FulfillmentErrorCode::CodeSelectionError(
                                         SelectionError::Unimplemented,
                                     ),
                                     points_at_arg_span: false,
+                                    // FIXME - does Chalk have a notation of 'root obligation'?
+                                    // This is just for diagnostics, so it's okay if this is wrong
+                                    root_obligation: obligation,
                                 }),
                             }
                         } else {
@@ -119,11 +125,14 @@ impl TraitEngine<'tcx> for FulfillmentContext<'tcx> {
                     }
 
                     Err(NoSolution) => errors.push(FulfillmentError {
-                        obligation,
+                        obligation: obligation.clone(),
                         code: FulfillmentErrorCode::CodeSelectionError(
                             SelectionError::Unimplemented,
                         ),
                         points_at_arg_span: false,
+                        // FIXME - does Chalk have a notation of 'root obligation'?
+                        // This is just for diagnostics, so it's okay if this is wrong
+                        root_obligation: obligation,
                     }),
                 }
             }

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

@@ -55,9 +55,13 @@ pub trait InferCtxtExt<'tcx> {
 
     fn report_overflow_error_cycle(&self, cycle: &[PredicateObligation<'tcx>]) -> !;
 
+    /// The `root_obligation` parameter should be the `root_obligation` field
+    /// from a `FulfillmentError`. If no `FulfillmentError` is available,
+    /// then it should be the same as `obligation`.
     fn report_selection_error(
         &self,
-        obligation: &PredicateObligation<'tcx>,
+        obligation: PredicateObligation<'tcx>,
+        root_obligation: &PredicateObligation<'tcx>,
         error: &SelectionError<'tcx>,
         fallback_has_occurred: bool,
         points_at_arg: bool,
@@ -225,16 +229,29 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
 
     fn report_selection_error(
         &self,
-        obligation: &PredicateObligation<'tcx>,
+        mut obligation: PredicateObligation<'tcx>,
+        root_obligation: &PredicateObligation<'tcx>,
         error: &SelectionError<'tcx>,
         fallback_has_occurred: bool,
         points_at_arg: bool,
     ) {
         let tcx = self.tcx;
-        let span = obligation.cause.span;
+        let mut span = obligation.cause.span;
 
         let mut err = match *error {
             SelectionError::Unimplemented => {
+                // If this obligation was generated as a result of well-formed checking, see if we
+                // can get a better error message by performing HIR-based well formed checking.
+                if let ObligationCauseCode::WellFormed(Some(wf_hir_id)) =
+                    root_obligation.cause.code.peel_derives()
+                {
+                    if let Some(cause) =
+                        self.tcx.diagnostic_hir_wf_check((obligation.predicate, *wf_hir_id))
+                    {
+                        obligation.cause = cause;
+                        span = obligation.cause.span;
+                    }
+                }
                 if let ObligationCauseCode::CompareImplMethodObligation {
                     item_name,
                     impl_item_def_id,
@@ -279,7 +296,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
                             .unwrap_or_default();
 
                         let OnUnimplementedNote { message, label, note, enclosing_scope } =
-                            self.on_unimplemented_note(trait_ref, obligation);
+                            self.on_unimplemented_note(trait_ref, &obligation);
                         let have_alt_message = message.is_some() || label.is_some();
                         let is_try_conversion = self.is_try_conversion(span, trait_ref.def_id());
                         let is_unsize =
@@ -338,7 +355,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
                                     Applicability::MachineApplicable,
                                 );
                             }
-                            if let Some(ret_span) = self.return_type_span(obligation) {
+                            if let Some(ret_span) = self.return_type_span(&obligation) {
                                 err.span_label(
                                     ret_span,
                                     &format!(
@@ -368,7 +385,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
                             points_at_arg,
                             have_alt_message,
                         ) {
-                            self.note_obligation_cause(&mut err, obligation);
+                            self.note_obligation_cause(&mut err, &obligation);
                             err.emit();
                             return;
                         }
@@ -821,7 +838,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
             }
         };
 
-        self.note_obligation_cause(&mut err, obligation);
+        self.note_obligation_cause(&mut err, &obligation);
         self.point_at_returns_when_relevant(&mut err, &obligation);
 
         err.emit();
@@ -1168,7 +1185,8 @@ impl<'a, 'tcx> InferCtxtPrivExt<'tcx> for InferCtxt<'a, 'tcx> {
         match error.code {
             FulfillmentErrorCode::CodeSelectionError(ref selection_error) => {
                 self.report_selection_error(
-                    &error.obligation,
+                    error.obligation.clone(),
+                    &error.root_obligation,
                     selection_error,
                     fallback_has_occurred,
                     error.points_at_arg_span,

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

@@ -1902,7 +1902,8 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
             | ObligationCauseCode::ReturnNoExpression
             | ObligationCauseCode::UnifyReceiver(..)
             | ObligationCauseCode::OpaqueType
-            | ObligationCauseCode::MiscObligation => {}
+            | ObligationCauseCode::MiscObligation
+            | ObligationCauseCode::WellFormed(..) => {}
             ObligationCauseCode::SliceOrArrayElem => {
                 err.note("slice and array elements must have `Sized` type");
             }

compiler/rustc_trait_selection/src/traits/fulfill.rs

@@ -717,6 +717,10 @@ fn substs_infer_vars<'a, 'tcx>(
 fn to_fulfillment_error<'tcx>(
     error: Error<PendingPredicateObligation<'tcx>, FulfillmentErrorCode<'tcx>>,
 ) -> FulfillmentError<'tcx> {
-    let obligation = error.backtrace.into_iter().next().unwrap().obligation;
-    FulfillmentError::new(obligation, error.error)
+    let mut iter = error.backtrace.into_iter();
+    let obligation = iter.next().unwrap().obligation;
+    // The root obligation is the last item in the backtrace - if there's only
+    // one item, then it's the same as the main obligation
+    let root_obligation = iter.next_back().map_or_else(|| obligation.clone(), |e| e.obligation);
+    FulfillmentError::new(obligation, error.error, root_obligation)
 }