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

Add method disambiguation help for trait implementation #62921

Merged
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
50 changes: 31 additions & 19 deletions src/librustc_typeck/check/method/suggest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ use rustc::hir::{self, ExprKind, Node, QPath};
use rustc::hir::def::{Res, DefKind};
use rustc::hir::def_id::{CRATE_DEF_INDEX, LOCAL_CRATE, DefId};
use rustc::hir::map as hir_map;
use rustc::hir::print;
use rustc::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind};
use rustc::traits::Obligation;
use rustc::ty::{self, Ty, TyCtxt, ToPolyTraitRef, ToPredicate, TypeFoldable};
Expand Down Expand Up @@ -78,6 +77,33 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
return;
}

let print_disambiguation_help = |
err: &mut DiagnosticBuilder<'_>,
trait_name: String,
| {
err.help(&format!(
"to disambiguate the method call, write `{}::{}({}{})` instead",
trait_name,
item_name,
if rcvr_ty.is_region_ptr() && args.is_some() {
if rcvr_ty.is_mutable_pointer() {
"&mut "
} else {
"&"
}
} else {
""
},
args.map(|arg| arg
.iter()
.map(|arg| self.tcx.sess.source_map().span_to_snippet(arg.span)
.unwrap_or_else(|_| "...".to_owned()))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@estebank This this unwrap fine? When can this happen?

Copy link
Contributor

Choose a reason for hiding this comment

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

It shouldn't happen, but it can if the span is coming from a proc macro or an external crate.

.collect::<Vec<_>>()
.join(", ")
).unwrap_or_else(|| "...".to_owned())
));
};

let report_candidates = |
span: Span,
err: &mut DiagnosticBuilder<'_>,
Expand Down Expand Up @@ -139,6 +165,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
} else {
err.note(&note_str);
}
if let Some(trait_ref) = self.tcx.impl_trait_ref(impl_did) {
print_disambiguation_help(err, self.tcx.def_path_str(trait_ref.def_id));
}
}
CandidateSource::TraitSource(trait_did) => {
let item = match self.associated_item(
Expand All @@ -163,24 +192,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
"the candidate is defined in the trait `{}`",
self.tcx.def_path_str(trait_did));
}
err.help(&format!("to disambiguate the method call, write `{}::{}({}{})` \
instead",
self.tcx.def_path_str(trait_did),
item_name,
if rcvr_ty.is_region_ptr() && args.is_some() {
if rcvr_ty.is_mutable_pointer() {
"&mut "
} else {
"&"
}
} else {
""
},
args.map(|arg| arg.iter()
.map(|arg| print::to_string(print::NO_ANN,
|s| s.print_expr(arg)))
.collect::<Vec<_>>()
.join(", ")).unwrap_or_else(|| "...".to_owned())));
print_disambiguation_help(err, self.tcx.def_path_str(trait_did));
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that you're just moving code that was already there. If changing this to use a span_suggestion feels too much work for you to take on, we can merge this PR as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

feels too much work for you to take on, we can merge this PR as is

It's fine, I'll try to implement your suggestions 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

I created #62922 to track that work. If the tests pass on the current state, I'll just approve and then you can implement those suggestions at your own pace. They aren't difficult but might be time consuming. If you need any help with them, do not hesitate to reach out, and there are plenty of examples of how the span_suggestion api is supposed to be used.

}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@ note: candidate #1 is defined in an impl of the trait `Foo` for the type `i32`
|
LL | const ID: i32 = 1;
| ^^^^^^^^^^^^^^^^^^
= help: to disambiguate the method call, write `Foo::ID(...)` instead
note: candidate #2 is defined in an impl of the trait `Bar` for the type `i32`
--> $DIR/associated-const-ambiguity-report.rs:14:5
|
LL | const ID: i32 = 3;
| ^^^^^^^^^^^^^^^^^^
= help: to disambiguate the method call, write `Bar::ID(...)` instead

error: aborting due to previous error

Expand Down
2 changes: 2 additions & 0 deletions src/test/ui/error-codes/E0034.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@ note: candidate #1 is defined in an impl of the trait `Trait1` for the type `Tes
|
LL | fn foo() {}
| ^^^^^^^^
= help: to disambiguate the method call, write `Trait1::foo(...)` instead
note: candidate #2 is defined in an impl of the trait `Trait2` for the type `Test`
--> $DIR/E0034.rs:16:5
|
LL | fn foo() {}
| ^^^^^^^^
= help: to disambiguate the method call, write `Trait2::foo(...)` instead

error: aborting due to previous error

Expand Down
2 changes: 2 additions & 0 deletions src/test/ui/inference/inference_unstable_featured.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@ LL | assert_eq!('x'.ipu_flatten(), 0);
| ^^^^^^^^^^^ multiple `ipu_flatten` found
|
= note: candidate #1 is defined in an impl of the trait `inference_unstable_iterator::IpuIterator` for the type `char`
= help: to disambiguate the method call, write `inference_unstable_iterator::IpuIterator::ipu_flatten('x')` instead
= note: candidate #2 is defined in an impl of the trait `inference_unstable_itertools::IpuItertools` for the type `char`
= help: to disambiguate the method call, write `inference_unstable_itertools::IpuItertools::ipu_flatten('x')` instead

error: aborting due to previous error

Expand Down
2 changes: 2 additions & 0 deletions src/test/ui/issues/issue-3702-2.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@ note: candidate #1 is defined in an impl of the trait `ToPrimitive` for the type
|
LL | fn to_int(&self) -> isize { 0 }
| ^^^^^^^^^^^^^^^^^^^^^^^^^
= help: to disambiguate the method call, write `ToPrimitive::to_int(&self)` instead
note: candidate #2 is defined in an impl of the trait `Add` for the type `isize`
--> $DIR/issue-3702-2.rs:14:5
|
LL | fn to_int(&self) -> isize { *self }
| ^^^^^^^^^^^^^^^^^^^^^^^^^
= help: to disambiguate the method call, write `Add::to_int(&self)` instead

error: aborting due to previous error

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@ note: candidate #1 is defined in an impl of the trait `Me2` for the type `usize`
|
LL | impl Me2 for usize { fn me(&self) -> usize { *self } }
| ^^^^^^^^^^^^^^^^^^^^^
= help: to disambiguate the method call, write `Me2::me(1_usize)` instead
= note: candidate #2 is defined in an impl of the trait `ambig_impl_2_lib::Me` for the type `usize`
= help: to disambiguate the method call, write `ambig_impl_2_lib::Me::me(1_usize)` instead

error: aborting due to previous error

Expand Down
16 changes: 16 additions & 0 deletions src/test/ui/methods/method-ambig-two-traits-from-impls.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
trait A { fn foo(self); }
trait B { fn foo(self); }

struct AB {}

impl A for AB {
fn foo(self) {}
}

impl B for AB {
fn foo(self) {}
}

fn main() {
AB {}.foo(); //~ ERROR E0034
}
22 changes: 22 additions & 0 deletions src/test/ui/methods/method-ambig-two-traits-from-impls.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
error[E0034]: multiple applicable items in scope
--> $DIR/method-ambig-two-traits-from-impls.rs:15:11
|
LL | AB {}.foo();
| ^^^ multiple `foo` found
|
note: candidate #1 is defined in an impl of the trait `A` for the type `AB`
--> $DIR/method-ambig-two-traits-from-impls.rs:7:5
|
LL | fn foo(self) {}
| ^^^^^^^^^^^^
= help: to disambiguate the method call, write `A::foo(AB {})` instead
note: candidate #2 is defined in an impl of the trait `B` for the type `AB`
--> $DIR/method-ambig-two-traits-from-impls.rs:11:5
|
LL | fn foo(self) {}
| ^^^^^^^^^^^^
= help: to disambiguate the method call, write `B::foo(AB {})` instead

error: aborting due to previous error
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 that the ideal output for this would instead be:

error[E0034]: multiple applicable items in scope
  --> $DIR/method-ambig-two-traits-from-impls.rs:15:11
   |
LL |     AB {}.foo();
   |           ^^^ multiple `foo` found
   |
note: candidate #1 is defined in an impl of the trait `A` for the type `AB`
  --> $DIR/method-ambig-two-traits-from-impls2.rs:7:5
   |
LL |     fn foo() {}
   |     ^^^^^^^^
note: candidate #2 is defined in an impl of the trait `B` for the type `AB`
  --> $DIR/method-ambig-two-traits-from-impls2.rs:11:5
   |
LL |     fn foo() {}
   |     ^^^^^^^^
help: disambiguate the method call to a specific trait:
  --> $DIR/method-ambig-two-traits-from-impls.rs:15:4
   |
LL |     A::foo(AB {})
   |     ^^^^^^^^^^^^^
LL |     B::foo(AB {})
   |     ^^^^^^^^^^^^^

The only reason I'm leaving the notes it's because that information is still useful and we can't (yet, at least) interleave suggestions and notes.


For more information about this error, try `rustc --explain E0034`.
16 changes: 16 additions & 0 deletions src/test/ui/methods/method-ambig-two-traits-from-impls2.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
trait A { fn foo(); }
trait B { fn foo(); }

struct AB {}

impl A for AB {
fn foo() {}
}

impl B for AB {
fn foo() {}
}

fn main() {
AB::foo(); //~ ERROR E0034
}
22 changes: 22 additions & 0 deletions src/test/ui/methods/method-ambig-two-traits-from-impls2.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
error[E0034]: multiple applicable items in scope
--> $DIR/method-ambig-two-traits-from-impls2.rs:15:5
|
LL | AB::foo();
| ^^^^^^^ multiple `foo` found
|
note: candidate #1 is defined in an impl of the trait `A` for the type `AB`
--> $DIR/method-ambig-two-traits-from-impls2.rs:7:5
|
LL | fn foo() {}
| ^^^^^^^^
= help: to disambiguate the method call, write `A::foo(...)` instead
note: candidate #2 is defined in an impl of the trait `B` for the type `AB`
--> $DIR/method-ambig-two-traits-from-impls2.rs:11:5
|
LL | fn foo() {}
| ^^^^^^^^
= help: to disambiguate the method call, write `B::foo(...)` instead

error: aborting due to previous error
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 that ideally the output would be closer to

error[E0034]: multiple applicable items in scope
  --> $DIR/method-ambig-two-traits-from-impls.rs:15:11
   |
LL |     AB {}.foo();
   |           ^^^ multiple `foo` found
   |
note: candidate #1 is defined in an impl of the trait `A` for the type `AB`
  --> $DIR/method-ambig-two-traits-from-impls2.rs:7:5
   |
LL |     fn foo() {}
   |     ^^^^^^^^
note: candidate #2 is defined in an impl of the trait `B` for the type `AB`
  --> $DIR/method-ambig-two-traits-from-impls2.rs:11:5
   |
LL |     fn foo() {}
   |     ^^^^^^^^
help: disambiguate the method call to a specific trait:
  --> $DIR/method-ambig-two-traits-from-impls.rs:15:4
   |
LL |     A::foo(AB {})
   |     ^^^^^^^^^^^^^
LL |     B::foo(AB {})
   |     ^^^^^^^^^^^^^

Note that the help here would be created with err.span_suggestions, not err.span_note.


For more information about this error, try `rustc --explain E0034`.
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@ note: candidate #1 is defined in an impl of the trait `Foo` for the type `usize`
|
LL | trait Foo { fn method(&self) {} }
| ^^^^^^^^^^^^^^^^
= help: to disambiguate the method call, write `Foo::method(1_usize)` instead
note: candidate #2 is defined in an impl of the trait `Bar` for the type `usize`
--> $DIR/method-ambig-two-traits-with-default-method.rs:6:13
|
LL | trait Bar { fn method(&self) {} }
| ^^^^^^^^^^^^^^^^
= help: to disambiguate the method call, write `Bar::method(1_usize)` instead

error: aborting due to previous error

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,13 @@ note: candidate #1 is defined in an impl of the trait `internal::X` for the type
|
LL | fn foo(self: Smaht<Self, u64>) -> u64 {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
= help: to disambiguate the method call, write `internal::X::foo(x)` instead
note: candidate #2 is defined in an impl of the trait `nuisance_foo::NuisanceFoo` for the type `_`
--> $DIR/method-deref-to-same-trait-object-with-separate-params.rs:70:9
|
LL | fn foo(self) {}
| ^^^^^^^^^^^^
= help: to disambiguate the method call, write `nuisance_foo::NuisanceFoo::foo(x)` instead
note: candidate #3 is defined in the trait `FinalFoo`
--> $DIR/method-deref-to-same-trait-object-with-separate-params.rs:57:5
|
Expand Down
2 changes: 2 additions & 0 deletions src/test/ui/traits/trait-alias-ambiguous.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,13 @@ note: candidate #1 is defined in an impl of the trait `inner::A` for the type `u
|
LL | fn foo(&self) {}
| ^^^^^^^^^^^^^
= help: to disambiguate the method call, write `inner::A::foo(t)` instead
note: candidate #2 is defined in an impl of the trait `inner::B` for the type `u8`
--> $DIR/trait-alias-ambiguous.rs:11:9
|
LL | fn foo(&self) {}
| ^^^^^^^^^^^^^
= help: to disambiguate the method call, write `inner::B::foo(t)` instead

error: aborting due to previous error

Expand Down