Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't do post-method-probe error reporting steps if we're in a suggestion #125100

Merged
merged 2 commits into from
May 14, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions compiler/rustc_hir_typeck/src/demand.rs
Original file line number Diff line number Diff line change
Expand Up @@ -938,14 +938,14 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
);
}

pub fn get_conversion_methods(
pub fn get_conversion_methods_for_diagnostic(
&self,
span: Span,
expected: Ty<'tcx>,
checked_ty: Ty<'tcx>,
hir_id: hir::HirId,
) -> Vec<AssocItem> {
let methods = self.probe_for_return_type(
let methods = self.probe_for_return_type_for_diagnostic(
span,
probe::Mode::MethodCall,
expected,
Expand Down
9 changes: 7 additions & 2 deletions compiler/rustc_hir_typeck/src/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2414,7 +2414,12 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

let guar = if field.name == kw::Empty {
self.dcx().span_delayed_bug(field.span, "field name with no name")
} else if self.method_exists(field, base_ty, expr.hir_id, expected.only_has_type(self)) {
} else if self.method_exists_for_diagnostic(
field,
base_ty,
expr.hir_id,
expected.only_has_type(self),
) {
self.ban_take_value_of_method(expr, base_ty, field)
} else if !base_ty.is_primitive_ty() {
self.ban_nonexisting_field(field, base, expr, base_ty)
Expand Down Expand Up @@ -2600,7 +2605,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let mut err = self.private_field_err(field, base_did);

// Also check if an accessible method exists, which is often what is meant.
if self.method_exists(field, expr_t, expr.hir_id, return_ty)
if self.method_exists_for_diagnostic(field, expr_t, expr.hir_id, return_ty)
&& !self.expr_in_place(expr.hir_id)
{
self.suggest_method_call(
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_hir_typeck/src/fn_ctxt/suggestions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
expected_ty_expr: Option<&'tcx hir::Expr<'tcx>>,
) -> bool {
let expr = expr.peel_blocks();
let methods = self.get_conversion_methods(expr.span, expected, found, expr.hir_id);
let methods =
self.get_conversion_methods_for_diagnostic(expr.span, expected, found, expr.hir_id);

if let Some((suggestion, msg, applicability, verbose, annotation)) =
self.suggest_deref_or_ref(expr, found, expected)
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_hir_typeck/src/method/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ pub enum CandidateSource {
impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
/// Determines whether the type `self_ty` supports a visible method named `method_name` or not.
#[instrument(level = "debug", skip(self))]
pub fn method_exists(
pub fn method_exists_for_diagnostic(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find the for_diagnostic suffix naming convention confusing. I think it means "method exists (as considered for diagnostic purposes)", rather than "method exists for this diagnostic"? It feels like there should be a contrasting method_exists_for_non_diagnostic method. I'm just not sure how to read it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it means "and you should only call this method if you're doing diagnostics".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kinda don't want to change it though now lol

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I can live with it.

&self,
method_name: Ident,
self_ty: Ty<'tcx>,
Expand All @@ -102,7 +102,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
probe::Mode::MethodCall,
method_name,
return_type,
IsSuggestion(false),
IsSuggestion(true),
self_ty,
call_expr_id,
ProbeScope::TraitsInScope,
Expand Down
23 changes: 22 additions & 1 deletion compiler/rustc_hir_typeck/src/method/probe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,11 @@ pub(crate) struct ProbeContext<'a, 'tcx> {
>,

scope_expr_id: HirId,

/// Is this probe being done for a diagnostic? This will skip some error reporting
/// machinery, since we don't particularly care about, for example, similarly named
/// candidates if we're *reporting* similarly named candidates.
is_suggestion: IsSuggestion,
}

impl<'a, 'tcx> Deref for ProbeContext<'a, 'tcx> {
Expand Down Expand Up @@ -220,7 +225,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
/// would use to decide if a method is a plausible fit for
/// ambiguity purposes).
#[instrument(level = "debug", skip(self, candidate_filter))]
pub fn probe_for_return_type(
pub fn probe_for_return_type_for_diagnostic(
&self,
span: Span,
mode: Mode,
Expand Down Expand Up @@ -459,6 +464,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
&orig_values,
steps.steps,
scope_expr_id,
is_suggestion,
);

probe_cx.assemble_inherent_candidates();
Expand Down Expand Up @@ -553,6 +559,7 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
orig_steps_var_values: &'a OriginalQueryValues<'tcx>,
steps: &'tcx [CandidateStep<'tcx>],
scope_expr_id: HirId,
is_suggestion: IsSuggestion,
) -> ProbeContext<'a, 'tcx> {
ProbeContext {
fcx,
Expand All @@ -570,6 +577,7 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
static_candidates: RefCell::new(Vec::new()),
unsatisfied_predicates: RefCell::new(Vec::new()),
scope_expr_id,
is_suggestion,
}
}

Expand Down Expand Up @@ -944,6 +952,18 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
return r;
}

// If it's a `lookup_probe_for_diagnostic`, then quit early. No need to
// probe for other candidates.
if self.is_suggestion.0 {
return Err(MethodError::NoMatch(NoMatchData {
static_candidates: vec![],
unsatisfied_predicates: vec![],
out_of_scope_traits: vec![],
similar_candidate: None,
mode: self.mode,
}));
}

debug!("pick: actual search failed, assemble diagnostics");

let static_candidates = std::mem::take(self.static_candidates.get_mut());
Expand Down Expand Up @@ -1631,6 +1651,7 @@ impl<'a, 'tcx> ProbeContext<'a, 'tcx> {
self.orig_steps_var_values,
self.steps,
self.scope_expr_id,
IsSuggestion(true),
);
pcx.allow_similar_names = true;
pcx.assemble_inherent_candidates();
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_hir_typeck/src/method/suggest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2814,7 +2814,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
Some(output_ty) => self.resolve_vars_if_possible(output_ty),
_ => return,
};
let method_exists = self.method_exists(item_name, output_ty, call.hir_id, return_type);
let method_exists =
self.method_exists_for_diagnostic(item_name, output_ty, call.hir_id, return_type);
debug!("suggest_await_before_method: is_method_exist={}", method_exists);
if method_exists {
err.span_suggestion_verbose(
Expand Down
Loading