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.

Sign up for GitHub

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

Jump to bottom

fix: correct the args for disambiguate the associated function diagnostic #118911

Merged
merged 3 commits into from Dec 29, 2023
Merged

fix: correct the args for disambiguate the associated function diagnostic #118911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c6a2c1
fix: correct the args for `disambiguate the associated function` diag…
Young-Flash Dec 13, 2023
ffd619a
test: add test case for `disambiguate the associated function` diagno…
Young-Flash Dec 13, 2023
9e9353c
fix broken CI and code review
Young-Flash Dec 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
48 changes: 28 additions & 20 deletions compiler/rustc_hir_typeck/src/method/suggest.rs
View file
Open in desktop
Expand Up @@ -3312,6 +3312,7 @@ fn print_disambiguation_help<'tcx>(
span: Span,
item: ty::AssocItem,
) -> Option<String> {
let trait_impl_type = trait_ref.self_ty().peel_refs();
let trait_ref = if item.fn_has_self_parameter {
trait_ref.print_only_trait_name().to_string()
} else {
Expand All @@ -3324,27 +3325,34 @@ fn print_disambiguation_help<'tcx>(
{
let def_kind_descr = tcx.def_kind_descr(item.kind.as_def_kind(), item.def_id);
let item_name = item.ident(tcx);
let rcvr_ref = tcx
.fn_sig(item.def_id)
.skip_binder()
.skip_binder()
.inputs()
.get(0)
.and_then(|ty| ty.ref_mutability())
.map_or("", |mutbl| mutbl.ref_prefix_str());
let args = format!(
"({}{})",
rcvr_ref,
std::iter::once(receiver)
.chain(args.iter())
.map(|arg| tcx
.sess
.source_map()
.span_to_snippet(arg.span)
.unwrap_or_else(|_| { "_".to_owned() }))
.collect::<Vec<_>>()
.join(", "),
let first_input =
tcx.fn_sig(item.def_id).instantiate_identity().skip_binder().inputs().get(0);
let (first_arg_type, rcvr_ref) = (
first_input.map(|first| first.peel_refs()),
first_input
.and_then(|ty| ty.ref_mutability())
.map_or("", |mutbl| mutbl.ref_prefix_str()),
);

// If the type of first arg of this assoc function is `Self` or current trait impl type or `arbitrary_self_types`, we need to take the receiver as args. Otherwise, we don't.
let args = if let Some(first_arg_type) = first_arg_type
&& (first_arg_type == tcx.types.self_param
|| first_arg_type == trait_impl_type
Comment on lines +3339 to +3340
Copy link
Member

@fmease fmease Dec 21, 2023 •
edited

Choose a reason for hiding this comment

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

While this check works for a lot of cases, it doesn't work for more complex ones. E.g., here

struct S;

trait TraitA { fn f(this: Box<Self>); }
trait TraitB { fn f(this: Box<Self>); }

impl TraitA for S { fn f(this: Box<Self>) {} }
impl TraitB for S { fn f(this: Box<Self>) {} }

fn main() {
    Box::new(S).f(); // trait_impl_type `Box<S>` != first_arg_type `Box<Self>`
}

However, I don't think you need to address this in this PR since it's super broken anyway at the moment, it suggests <Box<S> as TraitA>::f(); & <Box<S> as TraitB>::f() while it should suggest TraitA::f(Box::new(S))/<S as TraitA>::f(Box::new(S)) & TraitB::f(Box::new(S))/<S as TraitB>::f(Box::new(S)).

I think this could be solved by using FnCtxt::can_eq instead of ==.

Copy link
Member

@fmease fmease Dec 21, 2023 •
edited

Choose a reason for hiding this comment

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

Further, with plain == we will never consider type parameters to be valid receiver types while they very well could be:

struct S;

trait TraitA { fn f(self); }
trait TraitB { fn f(self); }

impl<T> TraitA for T { fn f(self) {} }
impl<T> TraitB for T { fn f(self) {} }

fn main() { S.f(); }

You could think about instantiate'ing the fn_sig with fresh_args_for_item instead of calling skip_binder and using FnCtxt::can_eq with the ParamEnv of the assoc fn. Well, it can still have false positives due to autoref/autoderef. This way, we would rely less on the check arbitrary_self_types_check(…) / item.fn_has_self_parameter.

|| item.fn_has_self_parameter)
{
Some(receiver)
} else {
None
}
.into_iter()
.chain(args)
.map(|arg| {
tcx.sess.source_map().span_to_snippet(arg.span).unwrap_or_else(|_| "_".to_owned())
})
.collect::<Vec<_>>()
.join(", ");

let args = format!("({}{})", rcvr_ref, args);
err.span_suggestion_verbose(
span,
format!(
Expand Down
49 changes: 49 additions & 0 deletions tests/ui/methods/disambiguate-associated-function-first-arg.rs
View file
Open in desktop
@@ -0,0 +1,49 @@
struct A {}

fn main() {
let _a = A {};
_a.new(1);
//~^ ERROR no method named `new` found for struct `A` in the current scope
}

trait M {
fn new(_a: i32);
}
impl M for A {
fn new(_a: i32) {}
}

trait N {
fn new(_a: Self, _b: i32);
}
impl N for A {
fn new(_a: Self, _b: i32) {}
}

trait O {
fn new(_a: Self, _b: i32);
}
impl O for A {
fn new(_a: A, _b: i32) {}
}

struct S;

trait TraitA {
fn f(self);
}
trait TraitB {
fn f(self);
}

impl<T> TraitA for T {
fn f(self) {}
}
impl<T> TraitB for T {
fn f(self) {}
}

fn test() {
S.f();
//~^ multiple applicable items in scope
}
67 changes: 67 additions & 0 deletions tests/ui/methods/disambiguate-associated-function-first-arg.stderr
View file
Open in desktop
@@ -0,0 +1,67 @@
error[E0599]: no method named `new` found for struct `A` in the current scope
--> $DIR/disambiguate-associated-function-first-arg.rs:5:8
|
LL | struct A {}
| -------- method `new` not found for this struct
...
LL | _a.new(1);
| ^^^ this is an associated function, not a method
|
= note: found the following associated functions; to be used as methods, functions must have a `self` parameter
note: candidate #1 is defined in the trait `M`
--> $DIR/disambiguate-associated-function-first-arg.rs:10:5
|
LL | fn new(_a: i32);
| ^^^^^^^^^^^^^^^^
note: candidate #2 is defined in the trait `N`
--> $DIR/disambiguate-associated-function-first-arg.rs:17:5
|
LL | fn new(_a: Self, _b: i32);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
note: candidate #3 is defined in the trait `O`
--> $DIR/disambiguate-associated-function-first-arg.rs:24:5
|
LL | fn new(_a: Self, _b: i32);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^
help: disambiguate the associated function for candidate #1
|
LL | <A as M>::new(1);
| ~~~~~~~~~~~~~~~~
help: disambiguate the associated function for candidate #2
|
LL | <A as N>::new(_a, 1);
| ~~~~~~~~~~~~~~~~~~~~
help: disambiguate the associated function for candidate #3
|
LL | <A as O>::new(_a, 1);
| ~~~~~~~~~~~~~~~~~~~~

error[E0034]: multiple applicable items in scope
--> $DIR/disambiguate-associated-function-first-arg.rs:47:7
|
LL | S.f();
| ^ multiple `f` found
|
note: candidate #1 is defined in an impl of the trait `TraitA` for the type `T`
--> $DIR/disambiguate-associated-function-first-arg.rs:40:5
|
LL | fn f(self) {}
| ^^^^^^^^^^
note: candidate #2 is defined in an impl of the trait `TraitB` for the type `T`
--> $DIR/disambiguate-associated-function-first-arg.rs:43:5
|
LL | fn f(self) {}
| ^^^^^^^^^^
help: disambiguate the method for candidate #1
|
LL | TraitA::f(S);
| ~~~~~~~~~~~~
help: disambiguate the method for candidate #2
|
LL | TraitB::f(S);
| ~~~~~~~~~~~~

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0034, E0599.
For more information about an error, try `rustc --explain E0034`.
8 changes: 4 additions & 4 deletions tests/ui/methods/method-ambiguity-no-rcvr.stderr
View file
Open in desktop
Expand Up @@ -20,12 +20,12 @@ LL | fn foo() {}
| ^^^^^^^^
help: disambiguate the associated function for candidate #1
|
LL | <Qux as Foo>::foo(Qux);
| ~~~~~~~~~~~~~~~~~~~~~~
LL | <Qux as Foo>::foo();
| ~~~~~~~~~~~~~~~~~~~
help: disambiguate the associated function for candidate #2
|
LL | <Qux as FooBar>::foo(Qux);
| ~~~~~~~~~~~~~~~~~~~~~~~~~
LL | <Qux as FooBar>::foo();
| ~~~~~~~~~~~~~~~~~~~~~~

error: aborting due to 1 previous error

Expand Down