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

suggest ? when method is missing on Result<T, _> but found on T #96271

Merged
merged 2 commits into from
Jun 1, 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
203 changes: 151 additions & 52 deletions compiler/rustc_typeck/src/check/method/suggest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -978,45 +978,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
label_span_not_found(&mut err);
}

if let SelfSource::MethodCall(expr) = source
&& let Some((fields, substs)) = self.get_field_candidates(span, actual)
{
let call_expr =
self.tcx.hir().expect_expr(self.tcx.hir().get_parent_node(expr.hir_id));
for candidate_field in fields.iter() {
if let Some(field_path) = self.check_for_nested_field_satisfying(
span,
&|_, field_ty| {
self.lookup_probe(
span,
item_name,
field_ty,
call_expr,
ProbeScope::AllTraits,
)
.is_ok()
},
candidate_field,
substs,
vec![],
self.tcx.parent_module(expr.hir_id).to_def_id(),
) {
let field_path_str = field_path
.iter()
.map(|id| id.name.to_ident_string())
.collect::<Vec<String>>()
.join(".");
debug!("field_path_str: {:?}", field_path_str);
self.check_for_field_method(&mut err, source, span, actual, item_name);

err.span_suggestion_verbose(
item_name.span.shrink_to_lo(),
"one of the expressions' fields has a method of the same name",
format!("{field_path_str}."),
Applicability::MaybeIncorrect,
);
}
}
}
self.check_for_unwrap_self(&mut err, source, span, actual, item_name);

bound_spans.sort();
bound_spans.dedup();
Expand Down Expand Up @@ -1343,6 +1307,145 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
false
}

fn check_for_field_method(
&self,
err: &mut DiagnosticBuilder<'tcx, ErrorGuaranteed>,
source: SelfSource<'tcx>,
span: Span,
actual: Ty<'tcx>,
item_name: Ident,
) {
if let SelfSource::MethodCall(expr) = source
&& let Some((fields, substs)) = self.get_field_candidates(span, actual)
{
let call_expr = self.tcx.hir().expect_expr(self.tcx.hir().get_parent_node(expr.hir_id));
for candidate_field in fields.iter() {
if let Some(field_path) = self.check_for_nested_field_satisfying(
span,
&|_, field_ty| {
self.lookup_probe(
span,
item_name,
field_ty,
call_expr,
ProbeScope::AllTraits,
)
.is_ok()
},
candidate_field,
substs,
vec![],
self.tcx.parent_module(expr.hir_id).to_def_id(),
) {
let field_path_str = field_path
.iter()
.map(|id| id.name.to_ident_string())
.collect::<Vec<String>>()
.join(".");
debug!("field_path_str: {:?}", field_path_str);

err.span_suggestion_verbose(
item_name.span.shrink_to_lo(),
"one of the expressions' fields has a method of the same name",
format!("{field_path_str}."),
Applicability::MaybeIncorrect,
);
}
}
}
}

fn check_for_unwrap_self(
&self,
err: &mut DiagnosticBuilder<'tcx, ErrorGuaranteed>,
source: SelfSource<'tcx>,
span: Span,
actual: Ty<'tcx>,
item_name: Ident,
) {
let tcx = self.tcx;
let SelfSource::MethodCall(expr) = source else { return; };
let call_expr = tcx.hir().expect_expr(tcx.hir().get_parent_node(expr.hir_id));

let ty::Adt(kind, substs) = actual.kind() else { return; };
if !kind.is_enum() {
return;
}

let matching_variants: Vec<_> = kind
.variants()
.iter()
.flat_map(|variant| {
let [field] = &variant.fields[..] else { return None; };
let field_ty = field.ty(tcx, substs);

// Skip `_`, since that'll just lead to ambiguity.
if self.resolve_vars_if_possible(field_ty).is_ty_var() {
return None;
}

self.lookup_probe(span, item_name, field_ty, call_expr, ProbeScope::AllTraits)
.ok()
.map(|pick| (variant, field, pick))
})
.collect();

let ret_ty_matches = |diagnostic_item| {
if let Some(ret_ty) = self
.ret_coercion
.as_ref()
.map(|c| self.resolve_vars_if_possible(c.borrow().expected_ty()))
&& let ty::Adt(kind, _) = ret_ty.kind()
&& tcx.get_diagnostic_item(diagnostic_item) == Some(kind.did())
{
true
} else {
false
}
};

match &matching_variants[..] {
[(_, field, pick)] => {
let self_ty = field.ty(tcx, substs);
err.span_note(
tcx.def_span(pick.item.def_id),
&format!("the method `{item_name}` exists on the type `{self_ty}`"),
);
let (article, kind, variant, question) =
if Some(kind.did()) == tcx.get_diagnostic_item(sym::Result) {
("a", "Result", "Err", ret_ty_matches(sym::Result))
} else if Some(kind.did()) == tcx.get_diagnostic_item(sym::Option) {
("an", "Option", "None", ret_ty_matches(sym::Option))
} else {
return;
};
if question {
err.span_suggestion_verbose(
expr.span.shrink_to_hi(),
format!(
"use the `?` operator to extract the `{self_ty}` value, propagating \
{article} `{kind}::{variant}` value to the caller"
),
"?".to_owned(),
Applicability::MachineApplicable,
);
} else {
err.span_suggestion_verbose(
expr.span.shrink_to_hi(),
format!(
"consider using `{kind}::expect` to unwrap the `{self_ty}` value, \
panicking if the value is {article} `{kind}::{variant}`"
),
".expect(\"REASON\")".to_owned(),
Applicability::HasPlaceholders,
);
}
}
// FIXME(compiler-errors): Support suggestions for other matching enum variants
Copy link
Contributor

Choose a reason for hiding this comment

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

This would entail looking for methods equivalent to .expect based on heuristics, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, or something like suggesting if let Pattern(destructure) = value { .. }. Not sure how useful that is, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it'd be worth it

_ => {}
}
}

pub(crate) fn note_unmet_impls_on_type(
&self,
err: &mut Diagnostic,
Expand Down Expand Up @@ -1662,13 +1765,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
(self.tcx.mk_mut_ref(self.tcx.lifetimes.re_erased, rcvr_ty), "&mut "),
(self.tcx.mk_imm_ref(self.tcx.lifetimes.re_erased, rcvr_ty), "&"),
] {
match self.lookup_probe(
span,
item_name,
*rcvr_ty,
rcvr,
crate::check::method::probe::ProbeScope::AllTraits,
) {
match self.lookup_probe(span, item_name, *rcvr_ty, rcvr, ProbeScope::AllTraits) {
Ok(pick) => {
// If the method is defined for the receiver we have, it likely wasn't `use`d.
// We point at the method, but we just skip the rest of the check for arbitrary
Expand Down Expand Up @@ -1700,13 +1797,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
(self.tcx.mk_diagnostic_item(*rcvr_ty, sym::Arc), "Arc::new"),
(self.tcx.mk_diagnostic_item(*rcvr_ty, sym::Rc), "Rc::new"),
] {
if let Some(new_rcvr_t) = *rcvr_ty && let Ok(pick) = self.lookup_probe(
span,
item_name,
new_rcvr_t,
rcvr,
crate::check::method::probe::ProbeScope::AllTraits,
) {
if let Some(new_rcvr_t) = *rcvr_ty
&& let Ok(pick) = self.lookup_probe(
span,
item_name,
new_rcvr_t,
rcvr,
ProbeScope::AllTraits,
)
{
debug!("try_alt_rcvr: pick candidate {:?}", pick);
let did = Some(pick.item.container.id());
// We don't want to suggest a container type when the missing
Expand Down
59 changes: 59 additions & 0 deletions src/test/ui/suggestions/enum-method-probe.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// compile-flags: --edition=2021
// run-rustfix

#![allow(unused)]

struct Foo;

impl Foo {
fn get(&self) -> u8 {
42
}
}

fn test_result_in_result() -> Result<(), ()> {
let res: Result<_, ()> = Ok(Foo);
res?.get();
//~^ ERROR no method named `get` found for enum `Result` in the current scope
//~| HELP use the `?` operator
Ok(())
}

async fn async_test_result_in_result() -> Result<(), ()> {
let res: Result<_, ()> = Ok(Foo);
res?.get();
//~^ ERROR no method named `get` found for enum `Result` in the current scope
//~| HELP use the `?` operator
Ok(())
}

fn test_result_in_unit_return() {
let res: Result<_, ()> = Ok(Foo);
res.expect("REASON").get();
//~^ ERROR no method named `get` found for enum `Result` in the current scope
//~| HELP consider using `Result::expect` to unwrap the `Foo` value, panicking if the value is a `Result::Err`
}

async fn async_test_result_in_unit_return() {
let res: Result<_, ()> = Ok(Foo);
res.expect("REASON").get();
Copy link
Member Author

Choose a reason for hiding this comment

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

should this just fixup to .unwrap? I kinda prefer .expect just because it feels like better practice out of the two.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know what you mean, expect is better style, but unwrap ends up looking cleaner... I can be convinced either way.

//~^ ERROR no method named `get` found for enum `Result` in the current scope
//~| HELP consider using `Result::expect` to unwrap the `Foo` value, panicking if the value is a `Result::Err`
}

fn test_option_in_option() -> Option<()> {
let res: Option<_> = Some(Foo);
res?.get();
//~^ ERROR no method named `get` found for enum `Option` in the current scope
//~| HELP use the `?` operator
Some(())
}

fn test_option_in_unit_return() {
let res: Option<_> = Some(Foo);
res.expect("REASON").get();
//~^ ERROR no method named `get` found for enum `Option` in the current scope
//~| HELP consider using `Option::expect` to unwrap the `Foo` value, panicking if the value is an `Option::None`
}

fn main() {}
59 changes: 59 additions & 0 deletions src/test/ui/suggestions/enum-method-probe.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// compile-flags: --edition=2021
// run-rustfix
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a check for async fn as well. I'm intrigued if they work without further changes.


#![allow(unused)]

struct Foo;

impl Foo {
fn get(&self) -> u8 {
42
}
}

fn test_result_in_result() -> Result<(), ()> {
let res: Result<_, ()> = Ok(Foo);
res.get();
//~^ ERROR no method named `get` found for enum `Result` in the current scope
//~| HELP use the `?` operator
Ok(())
}

async fn async_test_result_in_result() -> Result<(), ()> {
let res: Result<_, ()> = Ok(Foo);
res.get();
//~^ ERROR no method named `get` found for enum `Result` in the current scope
//~| HELP use the `?` operator
Ok(())
}

fn test_result_in_unit_return() {
let res: Result<_, ()> = Ok(Foo);
res.get();
//~^ ERROR no method named `get` found for enum `Result` in the current scope
//~| HELP consider using `Result::expect` to unwrap the `Foo` value, panicking if the value is a `Result::Err`
}

async fn async_test_result_in_unit_return() {
let res: Result<_, ()> = Ok(Foo);
res.get();
//~^ ERROR no method named `get` found for enum `Result` in the current scope
//~| HELP consider using `Result::expect` to unwrap the `Foo` value, panicking if the value is a `Result::Err`
}

fn test_option_in_option() -> Option<()> {
let res: Option<_> = Some(Foo);
res.get();
//~^ ERROR no method named `get` found for enum `Option` in the current scope
//~| HELP use the `?` operator
Some(())
}

fn test_option_in_unit_return() {
let res: Option<_> = Some(Foo);
res.get();
//~^ ERROR no method named `get` found for enum `Option` in the current scope
//~| HELP consider using `Option::expect` to unwrap the `Foo` value, panicking if the value is an `Option::None`
}

fn main() {}
Loading