From 34b9594f6d7cecb748a7a88c27fc23137898f417 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sun, 25 Dec 2022 21:50:30 -0800 Subject: [PATCH 1/4] Detect when method call on LHS might be shadowed Address #39232. --- compiler/rustc_hir_typeck/src/demand.rs | 92 +++++++++++++++++++ compiler/rustc_hir_typeck/src/method/probe.rs | 32 +++++++ .../suggestions/shadowed-lplace-method.fixed | 10 ++ .../ui/suggestions/shadowed-lplace-method.rs | 10 ++ .../suggestions/shadowed-lplace-method.stderr | 23 +++++ 5 files changed, 167 insertions(+) create mode 100644 src/test/ui/suggestions/shadowed-lplace-method.fixed create mode 100644 src/test/ui/suggestions/shadowed-lplace-method.rs create mode 100644 src/test/ui/suggestions/shadowed-lplace-method.stderr diff --git a/compiler/rustc_hir_typeck/src/demand.rs b/compiler/rustc_hir_typeck/src/demand.rs index 042ff0b46a5af..f7f492863abc7 100644 --- a/compiler/rustc_hir_typeck/src/demand.rs +++ b/compiler/rustc_hir_typeck/src/demand.rs @@ -1,5 +1,6 @@ use crate::FnCtxt; use rustc_ast::util::parser::PREC_POSTFIX; +use rustc_errors::MultiSpan; use rustc_errors::{Applicability, Diagnostic, DiagnosticBuilder, ErrorGuaranteed}; use rustc_hir as hir; use rustc_hir::def::CtorKind; @@ -36,6 +37,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { return; } + self.annotate_alternative_method_deref(err, expr, error); + // Use `||` to give these suggestions a precedence let _ = self.suggest_missing_parentheses(err, expr) || self.suggest_remove_last_method_call(err, expr, expected) @@ -316,6 +319,95 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } } + fn annotate_alternative_method_deref( + &self, + err: &mut Diagnostic, + expr: &hir::Expr<'_>, + error: Option>, + ) { + let parent = self.tcx.hir().get_parent_node(expr.hir_id); + let Some(TypeError::Sorts(ExpectedFound { expected, .. })) = error else {return;}; + let Some(hir::Node::Expr(hir::Expr { + kind: hir::ExprKind::Assign(lhs, rhs, _), .. + })) = self.tcx.hir().find(parent) else {return; }; + if rhs.hir_id != expr.hir_id || expected.is_closure() { + return; + } + let hir::ExprKind::Unary(hir::UnOp::Deref, deref) = lhs.kind else { return; }; + let hir::ExprKind::MethodCall(path, base, args, _) = deref.kind else { return; }; + let self_ty = self.typeck_results.borrow().expr_ty_adjusted_opt(base).unwrap(); + let pick = self + .probe_for_name( + probe::Mode::MethodCall, + path.ident, + probe::IsSuggestion(true), + self_ty, + deref.hir_id, + probe::ProbeScope::TraitsInScope, + ) + .unwrap(); + let methods = self.probe_for_name_many( + probe::Mode::MethodCall, + path.ident, + probe::IsSuggestion(true), + self_ty, + deref.hir_id, + probe::ProbeScope::AllTraits, + ); + let suggestions: Vec<_> = methods + .into_iter() + .filter(|m| m.def_id != pick.item.def_id) + .map(|m| { + let substs = ty::InternalSubsts::for_item(self.tcx, m.def_id, |param, _| { + self.var_for_def(deref.span, param) + }); + vec![ + ( + deref.span.until(base.span), + format!( + "{}({}", + with_no_trimmed_paths!( + self.tcx.def_path_str_with_substs(m.def_id, substs,) + ), + match self.tcx.fn_sig(m.def_id).input(0).skip_binder().kind() { + ty::Ref(_, _, hir::Mutability::Mut) => "&mut ", + ty::Ref(_, _, _) => "&", + _ => "", + }, + ), + ), + match &args[..] { + [] => (base.span.shrink_to_hi().with_hi(deref.span.hi()), ")".to_string()), + [first, ..] => (base.span.until(first.span), String::new()), + }, + ] + }) + .collect(); + if suggestions.is_empty() { + return; + } + let mut path_span: MultiSpan = path.ident.span.into(); + path_span.push_span_label( + path.ident.span, + format!( + "refers to `{}`", + with_no_trimmed_paths!(self.tcx.def_path_str(pick.item.def_id)), + ), + ); + err.span_note( + path_span, + &format!( + "there are multiple methods with the same name, `{}` refers to `{}` in the method call", + path.ident, + with_no_trimmed_paths!(self.tcx.def_path_str(pick.item.def_id)), + )); + err.multipart_suggestions( + "you might have meant to invoke a different method, you can use the fully-qualified path", + suggestions, + Applicability::MaybeIncorrect, + ); + } + /// If the expected type is an enum (Issue #55250) with any variants whose /// sole field is of the found type, suggest such variants. (Issue #42764) fn suggest_compatible_variants( diff --git a/compiler/rustc_hir_typeck/src/method/probe.rs b/compiler/rustc_hir_typeck/src/method/probe.rs index b9e7830bf0792..a7574d4e1afd4 100644 --- a/compiler/rustc_hir_typeck/src/method/probe.rs +++ b/compiler/rustc_hir_typeck/src/method/probe.rs @@ -322,6 +322,38 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { ) } + #[instrument(level = "debug", skip(self))] + pub fn probe_for_name_many( + &self, + mode: Mode, + item_name: Ident, + is_suggestion: IsSuggestion, + self_ty: Ty<'tcx>, + scope_expr_id: hir::HirId, + scope: ProbeScope, + ) -> Vec { + self.probe_op( + item_name.span, + mode, + Some(item_name), + None, + is_suggestion, + self_ty, + scope_expr_id, + scope, + |probe_cx| { + Ok(probe_cx + .inherent_candidates + .iter() + .chain(&probe_cx.extension_candidates) + // .filter(|candidate| candidate_filter(&candidate.item)) + .map(|candidate| candidate.item) + .collect()) + }, + ) + .unwrap() + } + fn probe_op( &'a self, span: Span, diff --git a/src/test/ui/suggestions/shadowed-lplace-method.fixed b/src/test/ui/suggestions/shadowed-lplace-method.fixed new file mode 100644 index 0000000000000..740ac77ee0c67 --- /dev/null +++ b/src/test/ui/suggestions/shadowed-lplace-method.fixed @@ -0,0 +1,10 @@ +// run-rustfix +#![allow(unused_imports)] +use std::borrow::BorrowMut; +use std::cell::RefCell; +use std::rc::Rc; + +fn main() { + let rc = Rc::new(RefCell::new(true)); + *std::cell::RefCell::<_>::borrow_mut(&rc) = false; //~ ERROR E0308 +} diff --git a/src/test/ui/suggestions/shadowed-lplace-method.rs b/src/test/ui/suggestions/shadowed-lplace-method.rs new file mode 100644 index 0000000000000..6bf12879e6f28 --- /dev/null +++ b/src/test/ui/suggestions/shadowed-lplace-method.rs @@ -0,0 +1,10 @@ +// run-rustfix +#![allow(unused_imports)] +use std::borrow::BorrowMut; +use std::cell::RefCell; +use std::rc::Rc; + +fn main() { + let rc = Rc::new(RefCell::new(true)); + *rc.borrow_mut() = false; //~ ERROR E0308 +} diff --git a/src/test/ui/suggestions/shadowed-lplace-method.stderr b/src/test/ui/suggestions/shadowed-lplace-method.stderr new file mode 100644 index 0000000000000..080600128a3da --- /dev/null +++ b/src/test/ui/suggestions/shadowed-lplace-method.stderr @@ -0,0 +1,23 @@ +error[E0308]: mismatched types + --> $DIR/shadowed-lplace-method.rs:9:24 + | +LL | *rc.borrow_mut() = false; + | ---------------- ^^^^^ expected struct `Rc`, found `bool` + | | + | expected due to the type of this binding + | + = note: expected struct `Rc>` + found type `bool` +note: there are multiple methods with the same name, `borrow_mut` refers to `std::borrow::BorrowMut::borrow_mut` in the method call + --> $DIR/shadowed-lplace-method.rs:9:9 + | +LL | *rc.borrow_mut() = false; + | ^^^^^^^^^^ refers to `std::borrow::BorrowMut::borrow_mut` +help: you might have meant to invoke a different method, you can use the fully-qualified path + | +LL | *std::cell::RefCell::<_>::borrow_mut(&rc) = false; + | +++++++++++++++++++++++++++++++++++++ ~ + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0308`. From 85ff8889e455c1e88723a6d7450042e17f91657a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Mon, 26 Dec 2022 12:00:22 -0800 Subject: [PATCH 2/4] Tweak wording --- compiler/rustc_hir_typeck/src/demand.rs | 101 +++++++++++++++--- compiler/rustc_hir_typeck/src/method/probe.rs | 20 ++-- .../suggestions/shadowed-lplace-method.stderr | 7 +- 3 files changed, 98 insertions(+), 30 deletions(-) diff --git a/compiler/rustc_hir_typeck/src/demand.rs b/compiler/rustc_hir_typeck/src/demand.rs index f7f492863abc7..ea34cd94f8aeb 100644 --- a/compiler/rustc_hir_typeck/src/demand.rs +++ b/compiler/rustc_hir_typeck/src/demand.rs @@ -335,8 +335,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } let hir::ExprKind::Unary(hir::UnOp::Deref, deref) = lhs.kind else { return; }; let hir::ExprKind::MethodCall(path, base, args, _) = deref.kind else { return; }; - let self_ty = self.typeck_results.borrow().expr_ty_adjusted_opt(base).unwrap(); - let pick = self + let Some(self_ty) = self.typeck_results.borrow().expr_ty_adjusted_opt(base) else { return; }; + + let Ok(pick) = self .probe_for_name( probe::Mode::MethodCall, path.ident, @@ -344,9 +345,21 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { self_ty, deref.hir_id, probe::ProbeScope::TraitsInScope, - ) - .unwrap(); - let methods = self.probe_for_name_many( + ) else { + return; + }; + let in_scope_methods = self.probe_for_name_many( + probe::Mode::MethodCall, + path.ident, + probe::IsSuggestion(true), + self_ty, + deref.hir_id, + probe::ProbeScope::TraitsInScope, + ); + let other_methods_in_scope: Vec<_> = + in_scope_methods.iter().filter(|c| c.item.def_id != pick.item.def_id).collect(); + + let all_methods = self.probe_for_name_many( probe::Mode::MethodCall, path.ident, probe::IsSuggestion(true), @@ -354,10 +367,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { deref.hir_id, probe::ProbeScope::AllTraits, ); - let suggestions: Vec<_> = methods + let suggestions: Vec<_> = all_methods .into_iter() - .filter(|m| m.def_id != pick.item.def_id) - .map(|m| { + .filter(|c| c.item.def_id != pick.item.def_id) + .map(|c| { + let m = c.item; let substs = ty::InternalSubsts::for_item(self.tcx, m.def_id, |param, _| { self.var_for_def(deref.span, param) }); @@ -389,21 +403,74 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { let mut path_span: MultiSpan = path.ident.span.into(); path_span.push_span_label( path.ident.span, - format!( + with_no_trimmed_paths!(format!( "refers to `{}`", - with_no_trimmed_paths!(self.tcx.def_path_str(pick.item.def_id)), - ), + self.tcx.def_path_str(pick.item.def_id), + )), ); + let container_id = pick.item.container_id(self.tcx); + let container = with_no_trimmed_paths!(self.tcx.def_path_str(container_id)); + for def_id in pick.import_ids { + let hir_id = self.tcx.hir().local_def_id_to_hir_id(def_id); + path_span.push_span_label( + self.tcx.hir().span(hir_id), + format!("`{container}` imported here"), + ); + } + let tail = with_no_trimmed_paths!(match &other_methods_in_scope[..] { + [] => return, + [candidate] => format!( + "the method of the same name on {} `{}`", + match candidate.kind { + probe::CandidateKind::InherentImplCandidate(..) => "the inherent impl for", + _ => "trait", + }, + self.tcx.def_path_str(candidate.item.container_id(self.tcx)) + ), + [.., last] if other_methods_in_scope.len() < 5 => { + format!( + "the methods of the same name on {} and `{}`", + other_methods_in_scope[..other_methods_in_scope.len() - 1] + .iter() + .map(|c| format!( + "`{}`", + self.tcx.def_path_str(c.item.container_id(self.tcx)) + )) + .collect::>() + .join(", "), + self.tcx.def_path_str(last.item.container_id(self.tcx)) + ) + } + _ => format!( + "the methods of the same name on {} other traits", + other_methods_in_scope.len() + ), + }); err.span_note( path_span, &format!( - "there are multiple methods with the same name, `{}` refers to `{}` in the method call", - path.ident, - with_no_trimmed_paths!(self.tcx.def_path_str(pick.item.def_id)), - )); + "the `{}` call is resolved to the method in `{container}`, shadowing {tail}", + path.ident, + ), + ); + if suggestions.len() > other_methods_in_scope.len() { + err.note(&format!( + "additionally, there are {} other available methods that aren't in scope", + suggestions.len() - other_methods_in_scope.len() + )); + } err.multipart_suggestions( - "you might have meant to invoke a different method, you can use the fully-qualified path", - suggestions, + &format!( + "you might have meant to call {}; you can use the fully-qualified path to call {} \ + explicitly", + if suggestions.len() == 1 { + "the other method" + } else { + "one of the other methods" + }, + if suggestions.len() == 1 { "it" } else { "one of them" }, + ), + suggestions, Applicability::MaybeIncorrect, ); } diff --git a/compiler/rustc_hir_typeck/src/method/probe.rs b/compiler/rustc_hir_typeck/src/method/probe.rs index a7574d4e1afd4..6b3fa664d9c5a 100644 --- a/compiler/rustc_hir_typeck/src/method/probe.rs +++ b/compiler/rustc_hir_typeck/src/method/probe.rs @@ -97,7 +97,7 @@ impl<'a, 'tcx> Deref for ProbeContext<'a, 'tcx> { } #[derive(Debug, Clone)] -struct Candidate<'tcx> { +pub(crate) struct Candidate<'tcx> { // Candidates are (I'm not quite sure, but they are mostly) basically // some metadata on top of a `ty::AssocItem` (without substs). // @@ -131,13 +131,13 @@ struct Candidate<'tcx> { // if `T: Sized`. xform_self_ty: Ty<'tcx>, xform_ret_ty: Option>, - item: ty::AssocItem, - kind: CandidateKind<'tcx>, - import_ids: SmallVec<[LocalDefId; 1]>, + pub(crate) item: ty::AssocItem, + pub(crate) kind: CandidateKind<'tcx>, + pub(crate) import_ids: SmallVec<[LocalDefId; 1]>, } #[derive(Debug, Clone)] -enum CandidateKind<'tcx> { +pub(crate) enum CandidateKind<'tcx> { InherentImplCandidate( SubstsRef<'tcx>, // Normalize obligations @@ -323,7 +323,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { } #[instrument(level = "debug", skip(self))] - pub fn probe_for_name_many( + pub(crate) fn probe_for_name_many( &self, mode: Mode, item_name: Ident, @@ -331,7 +331,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { self_ty: Ty<'tcx>, scope_expr_id: hir::HirId, scope: ProbeScope, - ) -> Vec { + ) -> Vec> { self.probe_op( item_name.span, mode, @@ -344,10 +344,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { |probe_cx| { Ok(probe_cx .inherent_candidates - .iter() - .chain(&probe_cx.extension_candidates) - // .filter(|candidate| candidate_filter(&candidate.item)) - .map(|candidate| candidate.item) + .into_iter() + .chain(probe_cx.extension_candidates) .collect()) }, ) diff --git a/src/test/ui/suggestions/shadowed-lplace-method.stderr b/src/test/ui/suggestions/shadowed-lplace-method.stderr index 080600128a3da..91d0d1200d436 100644 --- a/src/test/ui/suggestions/shadowed-lplace-method.stderr +++ b/src/test/ui/suggestions/shadowed-lplace-method.stderr @@ -8,12 +8,15 @@ LL | *rc.borrow_mut() = false; | = note: expected struct `Rc>` found type `bool` -note: there are multiple methods with the same name, `borrow_mut` refers to `std::borrow::BorrowMut::borrow_mut` in the method call +note: the `borrow_mut` call is resolved to the method in `std::borrow::BorrowMut`, shadowing the method of the same name on the inherent impl for `std::cell::RefCell` --> $DIR/shadowed-lplace-method.rs:9:9 | +LL | use std::borrow::BorrowMut; + | ---------------------- `std::borrow::BorrowMut` imported here +... LL | *rc.borrow_mut() = false; | ^^^^^^^^^^ refers to `std::borrow::BorrowMut::borrow_mut` -help: you might have meant to invoke a different method, you can use the fully-qualified path +help: you might have meant to call the other method; you can use the fully-qualified path to call it explicitly | LL | *std::cell::RefCell::<_>::borrow_mut(&rc) = false; | +++++++++++++++++++++++++++++++++++++ ~ From 8bde5bbc07e94fd8d39c263f873c53131b617d24 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Mon, 26 Dec 2022 12:29:16 -0800 Subject: [PATCH 3/4] Fix suggestion when there are arguments in the method --- compiler/rustc_hir_typeck/src/demand.rs | 2 +- .../suggestions/shadowed-lplace-method-2.rs | 23 +++++++++++++++++ .../shadowed-lplace-method-2.stderr | 25 +++++++++++++++++++ 3 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 src/test/ui/suggestions/shadowed-lplace-method-2.rs create mode 100644 src/test/ui/suggestions/shadowed-lplace-method-2.stderr diff --git a/compiler/rustc_hir_typeck/src/demand.rs b/compiler/rustc_hir_typeck/src/demand.rs index ea34cd94f8aeb..5afd2ad045e89 100644 --- a/compiler/rustc_hir_typeck/src/demand.rs +++ b/compiler/rustc_hir_typeck/src/demand.rs @@ -392,7 +392,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { ), match &args[..] { [] => (base.span.shrink_to_hi().with_hi(deref.span.hi()), ")".to_string()), - [first, ..] => (base.span.until(first.span), String::new()), + [first, ..] => (base.span.between(first.span), ", ".to_string()), }, ] }) diff --git a/src/test/ui/suggestions/shadowed-lplace-method-2.rs b/src/test/ui/suggestions/shadowed-lplace-method-2.rs new file mode 100644 index 0000000000000..dab99fbacd9e6 --- /dev/null +++ b/src/test/ui/suggestions/shadowed-lplace-method-2.rs @@ -0,0 +1,23 @@ +#![allow(unused)] + +struct X { + x: (), +} +pub trait A { + fn foo(&mut self, _: usize) -> &mut (); +} +impl A for X { + fn foo(&mut self, _: usize) -> &mut () { + &mut self.x + } +} +impl X { + fn foo(&mut self, _: usize) -> &mut Self { + self + } +} + +fn main() { + let mut x = X { x: () }; + *x.foo(0) = (); //~ ERROR E0308 +} diff --git a/src/test/ui/suggestions/shadowed-lplace-method-2.stderr b/src/test/ui/suggestions/shadowed-lplace-method-2.stderr new file mode 100644 index 0000000000000..94eef15f3306e --- /dev/null +++ b/src/test/ui/suggestions/shadowed-lplace-method-2.stderr @@ -0,0 +1,25 @@ +error[E0308]: mismatched types + --> $DIR/shadowed-lplace-method-2.rs:22:17 + | +LL | *x.foo(0) = (); + | --------- ^^ expected struct `X`, found `()` + | | + | expected due to the type of this binding + | +note: the `foo` call is resolved to the method in `X`, shadowing the method of the same name on trait `A` + --> $DIR/shadowed-lplace-method-2.rs:22:8 + | +LL | *x.foo(0) = (); + | ^^^ refers to `X::foo` +help: you might have meant to call the other method; you can use the fully-qualified path to call it explicitly + | +LL | *<_ as A>::foo(&mut x, 0) = (); + | ++++++++++++++++++ ~ +help: try wrapping the expression in `X` + | +LL | *x.foo(0) = X { x: () }; + | ++++++ + + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0308`. From 2cc22cee968b008d81aeb05ba2bf93ec5dcd7177 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Mon, 26 Dec 2022 12:47:08 -0800 Subject: [PATCH 4/4] fix rebase --- compiler/rustc_hir_typeck/src/demand.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/rustc_hir_typeck/src/demand.rs b/compiler/rustc_hir_typeck/src/demand.rs index 5afd2ad045e89..0335c45a946d5 100644 --- a/compiler/rustc_hir_typeck/src/demand.rs +++ b/compiler/rustc_hir_typeck/src/demand.rs @@ -31,7 +31,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { expr_ty: Ty<'tcx>, expected: Ty<'tcx>, expected_ty_expr: Option<&'tcx hir::Expr<'tcx>>, - _error: Option>, + error: Option>, ) { if expr_ty == expected { return;