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

Detect when method call on LHS might be shadowed #106150

Merged
merged 4 commits into from
Dec 27, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
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
161 changes: 160 additions & 1 deletion compiler/rustc_hir_typeck/src/demand.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -30,12 +31,14 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
expr_ty: Ty<'tcx>,
expected: Ty<'tcx>,
expected_ty_expr: Option<&'tcx hir::Expr<'tcx>>,
_error: Option<TypeError<'tcx>>,
error: Option<TypeError<'tcx>>,
) {
if expr_ty == expected {
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)
Expand Down Expand Up @@ -316,6 +319,162 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
}

fn annotate_alternative_method_deref(
&self,
err: &mut Diagnostic,
expr: &hir::Expr<'_>,
error: Option<TypeError<'tcx>>,
) {
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 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,
probe::IsSuggestion(true),
self_ty,
deref.hir_id,
probe::ProbeScope::TraitsInScope,
) 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();
Comment on lines +359 to +360
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here and below we should also filter out methods whose return types wouldn't can_eq with the type rhs type after dereferencing.


let all_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<_> = all_methods
.into_iter()
.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)
});
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.between(first.span), ", ".to_string()),
},
]
})
.collect();
if suggestions.is_empty() {
return;
}
let mut path_span: MultiSpan = path.ident.span.into();
path_span.push_span_label(
path.ident.span,
with_no_trimmed_paths!(format!(
"refers to `{}`",
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::<Vec<String>>()
.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!(
"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(
&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,
);
}

/// 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(
Expand Down
40 changes: 35 additions & 5 deletions compiler/rustc_hir_typeck/src/method/probe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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).
//
Expand Down Expand Up @@ -131,13 +131,13 @@ struct Candidate<'tcx> {
// if `T: Sized`.
xform_self_ty: Ty<'tcx>,
xform_ret_ty: Option<Ty<'tcx>>,
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
Expand Down Expand Up @@ -322,6 +322,36 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
)
}

#[instrument(level = "debug", skip(self))]
pub(crate) 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<Candidate<'tcx>> {
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
.into_iter()
.chain(probe_cx.extension_candidates)
.collect())
},
)
.unwrap()
}

fn probe_op<OP, R>(
&'a self,
span: Span,
Expand Down
23 changes: 23 additions & 0 deletions src/test/ui/suggestions/shadowed-lplace-method-2.rs
Original file line number Diff line number Diff line change
@@ -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
}
25 changes: 25 additions & 0 deletions src/test/ui/suggestions/shadowed-lplace-method-2.stderr
Original file line number Diff line number Diff line change
@@ -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`.
10 changes: 10 additions & 0 deletions src/test/ui/suggestions/shadowed-lplace-method.fixed
Original file line number Diff line number Diff line change
@@ -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
}
10 changes: 10 additions & 0 deletions src/test/ui/suggestions/shadowed-lplace-method.rs
Original file line number Diff line number Diff line change
@@ -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
}
26 changes: 26 additions & 0 deletions src/test/ui/suggestions/shadowed-lplace-method.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
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<RefCell<bool>>`
found type `bool`
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<T>`
--> $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 call the other method; you can use the fully-qualified path to call it explicitly
|
LL | *std::cell::RefCell::<_>::borrow_mut(&rc) = false;
| +++++++++++++++++++++++++++++++++++++ ~

error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.