Implement method signature suggestion for trait mismatches error #149027
Conversation
rustbot
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
reddevilmidzy
force-pushed
the
suggest
branch
from
a989b57 to
3d5d61f
Compare
This comment has been minimized.
This comment has been minimized.
reddevilmidzy
force-pushed
the
suggest
branch
from
3d5d61f to
3cecb85
Compare
rustbot
added
the
S-waiting-on-review
rustbot
removed
the
S-waiting-on-author
reddevilmidzy
force-pushed
the
suggest
branch
from
3cecb85 to
e408b20
Compare
This comment has been minimized.
This comment has been minimized.
reddevilmidzy
force-pushed
the
suggest
branch
from
e408b20 to
3cecb85
Compare
madsmtm
added
A-diagnostics
There was a problem hiding this comment.
Sorry for the delay in getting back here, this slipped my notifications.
Did you have a look at the previous implementation? #107548
Some of the tests in that one contains //~| HELP attributes, which I think is useful for actually testing that the behaviour is what we desire.
| | | ||
| help: modify the signature to match the trait definition | ||
| | | ||
| LL | fn come_on_a_little_more_effort(_: (), _: (), _: ()) {} |
There was a problem hiding this comment.
I agree that the help message is desirable for the cross-crate case (such as the serde example in the issue), since there the solution for the user is always to change the implementation (vs. e.g. changing the trait).
But here, it feels overly verbose? We're showing come_on_a_little_more_effort three times.
| | | ||
| help: modify the signature to match the trait definition | ||
| | | ||
| LL | fn come_on_a_little_more_effort(_: (), _: (), _: ()) {} |
There was a problem hiding this comment.
This doesn't suggest the proper return type. Is this intentional?
| ); | ||
|
|
||
| let sm = tcx.sess.source_map(); | ||
| let find_param_bounds = |snippet: &str| -> Option<(usize, usize)> { |
There was a problem hiding this comment.
Nit: Maybe make this a function? That makes it clearer that it doesn't capture anything.
| let find_param_bounds = |snippet: &str| -> Option<(usize, usize)> { | |
| fn find_param_bounds(snippet: &str) -> Option<(usize, usize)> { |
| match bytes[i] as char { | ||
| '(' => depth += 1, | ||
| ')' => { |
There was a problem hiding this comment.
Nit: Make it clearer that you're working on bytes
| match bytes[i] as char { | |
| '(' => depth += 1, | |
| ')' => { | |
| match bytes[i] { | |
| b'(' => depth += 1, | |
| b')' => { |
| help: modify the signature to match the trait definition | ||
| | | ||
| LL - fn fmt(&self) -> () { } | ||
| LL + fn fmt(&Self, &mut Formatter<'_>) -> () { } |
There was a problem hiding this comment.
The self type here is wrong.
| let suggestion = trait_m | ||
| .def_id | ||
| .as_local() | ||
| .and_then(|def_id| { | ||
| let (trait_sig, _) = tcx.hir_expect_trait_item(def_id).expect_fn(); | ||
| sm.span_to_snippet(trait_sig.span).ok().and_then(|snippet| { | ||
| find_param_bounds(&snippet) | ||
| .map(|(lo_rel, hi_rel)| snippet[lo_rel..hi_rel].to_string()) | ||
| }) | ||
| }) | ||
| .or_else(|| { | ||
| let signature = trait_m.signature(tcx); | ||
| find_param_bounds(&signature).map(|(lo, hi)| signature[lo..hi].trim().to_string()) | ||
| }); |
There was a problem hiding this comment.
Could we re-use the work done by looking up trait_span here instead?
| impl_number_args | ||
| ), | ||
| ); | ||
|
|
There was a problem hiding this comment.
As I understand this, you're looking up the trait definition, and somewhat blindly copying that to the suggestion. At a bit higher level, I think that maybe right way to implement this is to look at each differing argument, and suggest changing only those?
That would work better with cases like:
trait Foo {
fn foo(a: i32, b: u32);
}
type AliasOfI32 = i32;
impl Foo for () {
fn foo(a: AliasOfI32);
}
// Would only suggest adding `b: u32`, instead of also suggesting changing `AliasOfI32 -> i32`.What do you think?
rustbot
added
S-waiting-on-author
|
Reminder, once the PR becomes ready for a review, use |
4 participants
resolve: #106999