From 8a3f712518ffecb8115d60618654b984adcb8003 Mon Sep 17 00:00:00 2001 From: Esteban Kuber Date: Tue, 7 Sep 2021 11:19:57 +0000 Subject: [PATCH] Refactor `FulfillmentError` to track less data Move the information about pointing at the call argument expression in an unmet obligation span from the `FulfillmentError` to a new `ObligationCauseCode`. --- compiler/rustc_borrowck/src/type_check/mod.rs | 1 - .../mismatched_static_lifetime.rs | 8 ++- compiler/rustc_infer/src/traits/mod.rs | 6 +- compiler/rustc_middle/src/traits/mod.rs | 18 ++++-- .../src/traits/chalk_fulfill.rs | 3 - .../src/traits/error_reporting/mod.rs | 15 +---- .../src/traits/error_reporting/suggestions.rs | 59 +++++++++++++------ compiler/rustc_typeck/src/check/coercion.rs | 8 +-- .../rustc_typeck/src/check/fn_ctxt/checks.rs | 19 +++++- .../negated-auto-traits-error.stderr | 4 +- .../ui/unsized-locals/unsized-exprs.stderr | 2 +- 11 files changed, 86 insertions(+), 57 deletions(-) diff --git a/compiler/rustc_borrowck/src/type_check/mod.rs b/compiler/rustc_borrowck/src/type_check/mod.rs index 3e757827a5e..41c004ea596 100644 --- a/compiler/rustc_borrowck/src/type_check/mod.rs +++ b/compiler/rustc_borrowck/src/type_check/mod.rs @@ -1998,7 +1998,6 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { &obligation, &traits::SelectionError::Unimplemented, false, - false, ); } } diff --git a/compiler/rustc_infer/src/infer/error_reporting/nice_region_error/mismatched_static_lifetime.rs b/compiler/rustc_infer/src/infer/error_reporting/nice_region_error/mismatched_static_lifetime.rs index c60a7149e40..593d06bad7b 100644 --- a/compiler/rustc_infer/src/infer/error_reporting/nice_region_error/mismatched_static_lifetime.rs +++ b/compiler/rustc_infer/src/infer/error_reporting/nice_region_error/mismatched_static_lifetime.rs @@ -29,7 +29,13 @@ impl<'a, 'tcx> NiceRegionError<'a, 'tcx> { SubregionOrigin::Subtype(box TypeTrace { ref cause, .. }) => cause, _ => return None, }; - let (parent, impl_def_id) = match &cause.code { + // If we added a "points at argument expression" obligation, we remove it here, we care + // about the original obligation only. + let code = match &cause.code { + ObligationCauseCode::FunctionArgumentObligation { parent_code, .. } => &*parent_code, + _ => &cause.code, + }; + let (parent, impl_def_id) = match code { ObligationCauseCode::MatchImpl(parent, impl_def_id) => (parent, impl_def_id), _ => return None, }; diff --git a/compiler/rustc_infer/src/traits/mod.rs b/compiler/rustc_infer/src/traits/mod.rs index b450c398946..e1d6982f164 100644 --- a/compiler/rustc_infer/src/traits/mod.rs +++ b/compiler/rustc_infer/src/traits/mod.rs @@ -66,10 +66,6 @@ pub type Selection<'tcx> = ImplSource<'tcx, PredicateObligation<'tcx>>; pub struct FulfillmentError<'tcx> { pub obligation: PredicateObligation<'tcx>, pub code: FulfillmentErrorCode<'tcx>, - /// Diagnostics only: we opportunistically change the `code.span` when we encounter an - /// 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` @@ -128,7 +124,7 @@ impl<'tcx> FulfillmentError<'tcx> { code: FulfillmentErrorCode<'tcx>, root_obligation: PredicateObligation<'tcx>, ) -> FulfillmentError<'tcx> { - FulfillmentError { obligation, code, points_at_arg_span: false, root_obligation } + FulfillmentError { obligation, code, root_obligation } } } diff --git a/compiler/rustc_middle/src/traits/mod.rs b/compiler/rustc_middle/src/traits/mod.rs index 07c2ffac8c7..f63e9a087d9 100644 --- a/compiler/rustc_middle/src/traits/mod.rs +++ b/compiler/rustc_middle/src/traits/mod.rs @@ -253,6 +253,15 @@ pub enum ObligationCauseCode<'tcx> { DerivedObligation(DerivedObligationCause<'tcx>), + FunctionArgumentObligation { + /// The node of the relevant argument in the function call. + arg_hir_id: hir::HirId, + /// The node of the function call. + call_hir_id: hir::HirId, + /// The obligation introduced by this argument. + parent_code: Lrc>, + }, + /// Error derived when matching traits/impls; see ObligationCause for more details CompareImplConstObligation, @@ -368,11 +377,12 @@ impl ObligationCauseCode<'_> { // Return the base obligation, ignoring derived obligations. pub fn peel_derives(&self) -> &Self { let mut base_cause = self; - while let BuiltinDerivedObligation(cause) - | ImplDerivedObligation(cause) - | DerivedObligation(cause) = base_cause + while let BuiltinDerivedObligation(DerivedObligationCause { parent_code, .. }) + | ImplDerivedObligation(DerivedObligationCause { parent_code, .. }) + | DerivedObligation(DerivedObligationCause { parent_code, .. }) + | FunctionArgumentObligation { parent_code, .. } = base_cause { - base_cause = &cause.parent_code; + base_cause = &parent_code; } base_cause } diff --git a/compiler/rustc_trait_selection/src/traits/chalk_fulfill.rs b/compiler/rustc_trait_selection/src/traits/chalk_fulfill.rs index 7a690af0cc6..9c962d30ce0 100644 --- a/compiler/rustc_trait_selection/src/traits/chalk_fulfill.rs +++ b/compiler/rustc_trait_selection/src/traits/chalk_fulfill.rs @@ -57,7 +57,6 @@ impl TraitEngine<'tcx> for FulfillmentContext<'tcx> { .map(|obligation| FulfillmentError { 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(), @@ -112,7 +111,6 @@ impl TraitEngine<'tcx> for FulfillmentContext<'tcx> { 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, @@ -129,7 +127,6 @@ impl TraitEngine<'tcx> for FulfillmentContext<'tcx> { 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, diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs index 761b217c78f..8c9acd3ba73 100644 --- a/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs +++ b/compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs @@ -66,7 +66,6 @@ pub trait InferCtxtExt<'tcx> { root_obligation: &PredicateObligation<'tcx>, error: &SelectionError<'tcx>, fallback_has_occurred: bool, - points_at_arg: bool, ); /// Given some node representing a fn-like thing in the HIR map, @@ -237,7 +236,6 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { root_obligation: &PredicateObligation<'tcx>, error: &SelectionError<'tcx>, fallback_has_occurred: bool, - points_at_arg: bool, ) { let tcx = self.tcx; let mut span = obligation.cause.span; @@ -387,7 +385,6 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { &obligation, &mut err, &trait_ref, - points_at_arg, have_alt_message, ) { self.note_obligation_cause(&mut err, &obligation); @@ -430,8 +427,8 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { err.span_label(enclosing_scope_span, s.as_str()); } - self.suggest_dereferences(&obligation, &mut err, trait_ref, points_at_arg); - self.suggest_fn_call(&obligation, &mut err, trait_ref, points_at_arg); + self.suggest_dereferences(&obligation, &mut err, trait_ref); + self.suggest_fn_call(&obligation, &mut err, trait_ref); self.suggest_remove_reference(&obligation, &mut err, trait_ref); self.suggest_semicolon_removal(&obligation, &mut err, span, trait_ref); self.note_version_mismatch(&mut err, &trait_ref); @@ -500,12 +497,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { // Changing mutability doesn't make a difference to whether we have // an `Unsize` impl (Fixes ICE in #71036) if !is_unsize { - self.suggest_change_mut( - &obligation, - &mut err, - trait_ref, - points_at_arg, - ); + self.suggest_change_mut(&obligation, &mut err, trait_ref); } // If this error is due to `!: Trait` not implemented but `(): Trait` is @@ -1214,7 +1206,6 @@ impl<'a, 'tcx> InferCtxtPrivExt<'tcx> for InferCtxt<'a, 'tcx> { &error.root_obligation, selection_error, fallback_has_occurred, - error.points_at_arg_span, ); } FulfillmentErrorCode::CodeProjectionError(ref e) => { diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs index 9371ff3405e..3116d5cb1fa 100644 --- a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs +++ b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs @@ -54,7 +54,6 @@ pub trait InferCtxtExt<'tcx> { obligation: &PredicateObligation<'tcx>, err: &mut DiagnosticBuilder<'tcx>, trait_ref: ty::PolyTraitRef<'tcx>, - points_at_arg: bool, ); fn get_closure_name( @@ -69,7 +68,6 @@ pub trait InferCtxtExt<'tcx> { obligation: &PredicateObligation<'tcx>, err: &mut DiagnosticBuilder<'_>, trait_ref: ty::Binder<'tcx, ty::TraitRef<'tcx>>, - points_at_arg: bool, ); fn suggest_add_reference_to_arg( @@ -77,7 +75,6 @@ pub trait InferCtxtExt<'tcx> { obligation: &PredicateObligation<'tcx>, err: &mut DiagnosticBuilder<'_>, trait_ref: &ty::Binder<'tcx, ty::TraitRef<'tcx>>, - points_at_arg: bool, has_custom_message: bool, ) -> bool; @@ -93,7 +90,6 @@ pub trait InferCtxtExt<'tcx> { obligation: &PredicateObligation<'tcx>, err: &mut DiagnosticBuilder<'_>, trait_ref: ty::Binder<'tcx, ty::TraitRef<'tcx>>, - points_at_arg: bool, ); fn suggest_semicolon_removal( @@ -490,16 +486,19 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { obligation: &PredicateObligation<'tcx>, err: &mut DiagnosticBuilder<'tcx>, trait_ref: ty::PolyTraitRef<'tcx>, - points_at_arg: bool, ) { // It only make sense when suggesting dereferences for arguments - if !points_at_arg { + let code = if let ObligationCauseCode::FunctionArgumentObligation { parent_code, .. } = + &obligation.cause.code + { + std::rc::Rc::clone(parent_code) + } else { return; - } + }; let param_env = obligation.param_env; let body_id = obligation.cause.body_id; let span = obligation.cause.span; - let real_trait_ref = match &obligation.cause.code { + let real_trait_ref = match &*code { ObligationCauseCode::ImplDerivedObligation(cause) | ObligationCauseCode::DerivedObligation(cause) | ObligationCauseCode::BuiltinDerivedObligation(cause) => cause.parent_trait_ref, @@ -584,7 +583,6 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { obligation: &PredicateObligation<'tcx>, err: &mut DiagnosticBuilder<'_>, trait_ref: ty::Binder<'tcx, ty::TraitRef<'tcx>>, - points_at_arg: bool, ) { let self_ty = match trait_ref.self_ty().no_bound_vars() { None => return, @@ -656,11 +654,11 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { } _ => return, }; - if points_at_arg { + if matches!(obligation.cause.code, ObligationCauseCode::FunctionArgumentObligation { .. }) { // When the obligation error has been ensured to have been caused by // an argument, the `obligation.cause.span` points at the expression - // of the argument, so we can provide a suggestion. This is signaled - // by `points_at_arg`. Otherwise, we give a more general note. + // of the argument, so we can provide a suggestion. Otherwise, we give + // a more general note. err.span_suggestion_verbose( obligation.cause.span.shrink_to_hi(), &msg, @@ -677,7 +675,6 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { obligation: &PredicateObligation<'tcx>, err: &mut DiagnosticBuilder<'_>, trait_ref: &ty::Binder<'tcx, ty::TraitRef<'tcx>>, - points_at_arg: bool, has_custom_message: bool, ) -> bool { let span = obligation.cause.span; @@ -686,9 +683,14 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { ExpnKind::Desugaring(DesugaringKind::ForLoop(ForLoopLoc::IntoIter)) ); - if !points_at_arg && !points_at_for_iter { - return false; - } + let code = + if let (ObligationCauseCode::FunctionArgumentObligation { parent_code, .. }, false) = + (&obligation.cause.code, points_at_for_iter) + { + std::rc::Rc::clone(parent_code) + } else { + return false; + }; // List of traits for which it would be nonsensical to suggest borrowing. // For instance, immutable references are always Copy, so suggesting to @@ -787,7 +789,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { return false; }; - if let ObligationCauseCode::ImplDerivedObligation(obligation) = &obligation.cause.code { + if let ObligationCauseCode::ImplDerivedObligation(obligation) = &*code { let expected_trait_ref = obligation.parent_trait_ref.skip_binder(); let new_imm_trait_ref = ty::TraitRef::new(obligation.parent_trait_ref.def_id(), imm_substs); @@ -799,7 +801,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { return try_borrowing(new_mut_trait_ref, expected_trait_ref, true, &[]); } } else if let ObligationCauseCode::BindingObligation(_, _) - | ObligationCauseCode::ItemObligation(_) = &obligation.cause.code + | ObligationCauseCode::ItemObligation(_) = &*code { if try_borrowing( ty::TraitRef::new(trait_ref.def_id, imm_substs), @@ -891,8 +893,12 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { obligation: &PredicateObligation<'tcx>, err: &mut DiagnosticBuilder<'_>, trait_ref: ty::Binder<'tcx, ty::TraitRef<'tcx>>, - points_at_arg: bool, ) { + let points_at_arg = matches!( + obligation.cause.code, + ObligationCauseCode::FunctionArgumentObligation { .. }, + ); + let span = obligation.cause.span; if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(span) { let refs_number = @@ -2289,6 +2295,21 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { ) }); } + ObligationCauseCode::FunctionArgumentObligation { + arg_hir_id: _, + call_hir_id: _, + ref parent_code, + } => { + ensure_sufficient_stack(|| { + self.note_obligation_cause_code( + err, + predicate, + &parent_code, + obligated_types, + seen_requirements, + ) + }); + } ObligationCauseCode::CompareImplMethodObligation { item_name, trait_item_def_id, diff --git a/compiler/rustc_typeck/src/check/coercion.rs b/compiler/rustc_typeck/src/check/coercion.rs index 6fe96e4cc27..013aecae586 100644 --- a/compiler/rustc_typeck/src/check/coercion.rs +++ b/compiler/rustc_typeck/src/check/coercion.rs @@ -707,13 +707,7 @@ impl<'f, 'tcx> Coerce<'f, 'tcx> { // Object safety violations or miscellaneous. Err(err) => { - self.report_selection_error( - obligation.clone(), - &obligation, - &err, - false, - false, - ); + self.report_selection_error(obligation.clone(), &obligation, &err, false); // Treat this like an obligation and follow through // with the unsizing - the lack of a coercion should // be silent, as it causes a type mismatch later. diff --git a/compiler/rustc_typeck/src/check/fn_ctxt/checks.rs b/compiler/rustc_typeck/src/check/fn_ctxt/checks.rs index f5f1dd42652..9cf00bad10b 100644 --- a/compiler/rustc_typeck/src/check/fn_ctxt/checks.rs +++ b/compiler/rustc_typeck/src/check/fn_ctxt/checks.rs @@ -9,6 +9,7 @@ use crate::check::{ }; use rustc_ast as ast; +use rustc_data_structures::sync::Lrc; use rustc_errors::{Applicability, DiagnosticBuilder, DiagnosticId}; use rustc_hir as hir; use rustc_hir::def::{CtorOf, DefKind, Res}; @@ -324,6 +325,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { self.point_at_arg_instead_of_call_if_possible( errors, &final_arg_types[..], + expr, sp, &args, ); @@ -391,7 +393,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { ) { for error in errors { error.obligation.cause.make_mut().span = arg.span; - error.points_at_arg_span = true; + let code = error.obligation.cause.code.clone(); + error.obligation.cause.make_mut().code = + ObligationCauseCode::FunctionArgumentObligation { + arg_hir_id: arg.hir_id, + call_hir_id: expr.hir_id, + parent_code: Lrc::new(code), + }; } } }, @@ -937,6 +945,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { &self, errors: &mut Vec>, final_arg_types: &[(usize, Ty<'tcx>, Ty<'tcx>)], + expr: &'tcx hir::Expr<'tcx>, call_sp: Span, args: &'tcx [hir::Expr<'tcx>], ) { @@ -986,7 +995,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { // We make sure that only *one* argument matches the obligation failure // and we assign the obligation's span to its expression's. error.obligation.cause.make_mut().span = args[ref_in].span; - error.points_at_arg_span = true; + let code = error.obligation.cause.code.clone(); + error.obligation.cause.make_mut().code = + ObligationCauseCode::FunctionArgumentObligation { + arg_hir_id: args[ref_in].hir_id, + call_hir_id: expr.hir_id, + parent_code: Lrc::new(code), + }; } } } diff --git a/src/test/ui/traits/negative-impls/negated-auto-traits-error.stderr b/src/test/ui/traits/negative-impls/negated-auto-traits-error.stderr index ad95e06eb4e..6b210ed9970 100644 --- a/src/test/ui/traits/negative-impls/negated-auto-traits-error.stderr +++ b/src/test/ui/traits/negative-impls/negated-auto-traits-error.stderr @@ -43,7 +43,7 @@ error[E0277]: `dummy1c::TestType` cannot be sent between threads safely LL | is_send((8, TestType)); | ^^^^^^^^^^^^^ `dummy1c::TestType` cannot be sent between threads safely | - = help: within `({integer}, dummy1c::TestType)`, the trait `Send` is not implemented for `dummy1c::TestType` + = help: the trait `Send` is not implemented for `dummy1c::TestType` = note: required because it appears within the type `({integer}, dummy1c::TestType)` note: required by a bound in `is_send` --> $DIR/negated-auto-traits-error.rs:16:15 @@ -75,7 +75,7 @@ error[E0277]: `dummy3::TestType` cannot be sent between threads safely LL | is_send(Box::new(Outer2(TestType))); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ `dummy3::TestType` cannot be sent between threads safely | - = help: within `Outer2`, the trait `Send` is not implemented for `dummy3::TestType` + = help: the trait `Send` is not implemented for `dummy3::TestType` note: required because it appears within the type `Outer2` --> $DIR/negated-auto-traits-error.rs:12:8 | diff --git a/src/test/ui/unsized-locals/unsized-exprs.stderr b/src/test/ui/unsized-locals/unsized-exprs.stderr index a7f57e3fd15..d81c188df21 100644 --- a/src/test/ui/unsized-locals/unsized-exprs.stderr +++ b/src/test/ui/unsized-locals/unsized-exprs.stderr @@ -14,7 +14,7 @@ error[E0277]: the size for values of type `[u8]` cannot be known at compilation LL | udrop::>(A { 0: *foo() }); | ^^^^^^^^^^^^^^^ doesn't have a size known at compile-time | - = help: within `A<[u8]>`, the trait `Sized` is not implemented for `[u8]` + = help: the trait `Sized` is not implemented for `[u8]` note: required because it appears within the type `A<[u8]>` --> $DIR/unsized-exprs.rs:3:8 |