-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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}; | ||
|
@@ -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())) | ||
.collect::<Vec<_>>() | ||
.join(", ") | ||
).unwrap_or_else(|| "...".to_owned()) | ||
)); | ||
}; | ||
|
||
let report_candidates = | | ||
span: Span, | ||
err: &mut DiagnosticBuilder<'_>, | ||
|
@@ -139,6 +165,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { | |
} else { | ||
err.note(¬e_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( | ||
|
@@ -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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It's fine, I'll try to implement your suggestions 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
} | ||
} | ||
} | ||
|
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 | ||
} |
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that the ideal output for this would instead be:
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`. |
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 | ||
} |
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that ideally the output would be closer to
Note that the |
||
|
||
For more information about this error, try `rustc --explain E0034`. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.