Skip to content

Commit

Permalink
Refactor FulfillmentError to track less data
Browse files Browse the repository at this point in the history
Move the information about pointing at the call argument expression in
an unmet obligation span from the `FulfillmentError` to a new
`ObligationCauseCode`.
  • Loading branch information
estebank committed Sep 16, 2021
1 parent 284a8a9 commit 8a3f712
Show file tree
Hide file tree
Showing 11 changed files with 86 additions and 57 deletions.
1 change: 0 additions & 1 deletion compiler/rustc_borrowck/src/type_check/mod.rs
Expand Up @@ -1998,7 +1998,6 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> {
&obligation,
&traits::SelectionError::Unimplemented,
false,
false,
);
}
}
Expand Down
Expand Up @@ -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,
};
Expand Down
6 changes: 1 addition & 5 deletions compiler/rustc_infer/src/traits/mod.rs
Expand Up @@ -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`
Expand Down Expand Up @@ -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 }
}
}

Expand Down
18 changes: 14 additions & 4 deletions compiler/rustc_middle/src/traits/mod.rs
Expand Up @@ -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<ObligationCauseCode<'tcx>>,
},

/// Error derived when matching traits/impls; see ObligationCause for more details
CompareImplConstObligation,

Expand Down Expand Up @@ -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
}
Expand Down
3 changes: 0 additions & 3 deletions compiler/rustc_trait_selection/src/traits/chalk_fulfill.rs
Expand Up @@ -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(),
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down
15 changes: 3 additions & 12 deletions compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) => {
Expand Down
Expand Up @@ -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(
Expand All @@ -69,15 +68,13 @@ 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(
&self,
obligation: &PredicateObligation<'tcx>,
err: &mut DiagnosticBuilder<'_>,
trait_ref: &ty::Binder<'tcx, ty::TraitRef<'tcx>>,
points_at_arg: bool,
has_custom_message: bool,
) -> bool;

Expand All @@ -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(
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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);
Expand All @@ -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),
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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,
Expand Down
8 changes: 1 addition & 7 deletions compiler/rustc_typeck/src/check/coercion.rs
Expand Up @@ -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.
Expand Down
19 changes: 17 additions & 2 deletions compiler/rustc_typeck/src/check/fn_ctxt/checks.rs
Expand Up @@ -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};
Expand Down Expand Up @@ -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,
);
Expand Down Expand Up @@ -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),
};
}
}
},
Expand Down Expand Up @@ -937,6 +945,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
&self,
errors: &mut Vec<traits::FulfillmentError<'tcx>>,
final_arg_types: &[(usize, Ty<'tcx>, Ty<'tcx>)],
expr: &'tcx hir::Expr<'tcx>,
call_sp: Span,
args: &'tcx [hir::Expr<'tcx>],
) {
Expand Down Expand Up @@ -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),
};
}
}
}
Expand Down

0 comments on commit 8a3f712

Please sign in to comment.