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
On method chain expression failure, look for missing method in earlier segments of the chain #115229
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @wesleywiser (or someone else) soon. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
r? @oli-obk Please include some high level information about this PR in the main message (e.g. link to the issue and state what new message you are adding to which existing message) |
it looks like there are tests other than yours that are affected by this change. If you bless all ui tests, you should see some changes in your git status view |
This comment has been minimized.
This comment has been minimized.
@rustbot review |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
r? @estebank |
@estebank Contents of tests/ui/structs/method-chain-expression-failure.stdout:
|
In this case, should we check for return type of |
If we can filter the method with the right return type, I would prefer to suggest it, otherwise I would mention all of them. Give me a minute to check the rest of the PR and correlate with the logs you have here. |
Ah! I see what's happening. |
acf08eb
to
d7214df
Compare
prev_ty, | ||
rcvr_expr.hir_id, | ||
ProbeScope::TraitsInScope,) { | ||
err.span_label(method_span, format!("{item_kind} `{item_name}` is available on `{prev_ty}`")); |
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.
If you use parent_expr.span
, it would make the error be correct, but when you have more than one element in the chain you would end up with overlapping spans which won't look great.
An option we have is to iterate once using the while let
as you have it now, but collect the messages and spans in a vec and emit the diagnostics afterwards by iterating over the vec and peeking at the next span.
// Only if an appropriate error source is not found, check method chain for possible candiates | ||
let error_source_not_found = unsatisfied_predicates.is_empty(); | ||
if error_source_not_found && let Mode::MethodCall = mode && let SelfSource::MethodCall(mut rcvr_expr) = source { | ||
while let hir::ExprKind::MethodCall(_path_segment, parent_expr, _args, method_span) = |
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.
I would prefer we called parent_expr
to something that includes rcvr
/receiver
in the name, because the parent
concept is highly overloaded :)
LL | A.b().c().d().foo(); | ||
| --- ^^^ method not found in `D` | ||
| | | ||
| method `foo` is available on `&B` |
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.
To drive the point home, if you look at the impl the span label should be pointing at the .b()
which returns B
and not at the method call .c()
which returns C
.
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.
I love the results so far! On your next change, please squash your commits as I don't think we need the prior attempts anymore and expect us to land this shortly.
d7214df
to
39c4312
Compare
This comment has been minimized.
This comment has been minimized.
You'll need to run |
This comment has been minimized.
This comment has been minimized.
763194d
to
7441d1e
Compare
Had found an issue. Done now. |
…s receiver to align spans
7441d1e
to
56a109d
Compare
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (edf0b1d): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 674.395s -> 675.48s (0.16%) |
This PR tries to fix the issue: #115222
As suggested by @estebank , I did the following:
tests/ui/structs/method-chain-expression-failure.rs
compiler/rusct_hir_tycheck/src/method/suggest.rs
walking up the method chain and calling
probe_for_name
with the method name. But the call fails to returnOk
.