Better method call error messages #71827
Conversation
rust-highfive
added
the
S-waiting-on-review
|
(is there a way to avoid clogging up the build pipeline until I've gathered and responded to feedback? I don't want to waste other people's time) |
|
Also, if anyones curious, here's the scratch pad I used to iterate on the algorithm, and all the test cases I tested it on: https://glot.io/snippets/fmxqlb8dct The algorithm in rust shifted slightly during development, but I'll probably be adding each of the cases at the bottom to the tests for this PR. |
There was a problem hiding this comment.
I like the direction this is going.
I'm only looking at the output and will review the code later, but one preliminary comment would be to split the logic into multiple methods/functions to make it easier to follow. Left some comments about the output. For the structured suggestions (help blogs) you can use the multipart_suggestion method.
Also, for the cases where the problem is type parameter permutation we shouldn't be using the same error code.
| error[E0059]: one or more arguments to this function are incorrect | ||
| --> $DIR/basic.rs:31:5 | ||
| | | ||
| LL | invalid(1.0); | ||
| | ^^^^^^^^---^ | ||
| | | | ||
| | the type of this argument is incorrect |
There was a problem hiding this comment.
The current output is better:
error[E0308]: mismatched types
--> asdf.rs:18:13
|
18 | invalid(1.0); //~ ERROR arguments to this function are incorrect
| ^^^ expected `u32`, found floating-point number
There was a problem hiding this comment.
Yea, I wasn't quite sure how to get human readable names for the error message, and when I tried looking into the existing code it became a very deep rabbit hole and I quickly got lost. Any tips on methods I could reuse to avoid a ton of spurious work?
| error[E0059]: one or more arguments to this function are incorrect | ||
| --> $DIR/basic.rs:32:5 | ||
| | | ||
| LL | extra(&""); | ||
| | ^^^^^^---^ | ||
| | | | ||
| | this argument appears to be extra |
There was a problem hiding this comment.
I would like to keep the current output for this case:
error[E0061]: this function takes 0 arguments but 1 argument was supplied
--> asdf.rs:19:5
|
11 | fn extra() {}
| ---------- defined here
...
19 | extra(&""); //~ ERROR arguments to this function are incorrect
| ^^^^^ --- supplied 1 argument
| |
| expected 0 arguments
For this case we can use a span_suggestion_hidden that will only appear to rustfix and rust-analyzer for the removal of the argument.
There was a problem hiding this comment.
There are two main questions that would help me get better diagnostics:
-
Should we try to blanket these all under one error, with sub-diagnostics for each case, or split things out into separate errors to more closely match "existing" outputs?
-
How should we handle cases where there are multiple instances of an issue? Should we highlight each separately, or aggregate them together?
There was a problem hiding this comment.
You can have multiple .span_note(vec![spans], "message") for each of the cases you handle.
| error[E0059]: one or more arguments to this function are incorrect | ||
| --> $DIR/basic.rs:33:5 | ||
| | | ||
| LL | missing(); | ||
| | ^^^^^^^^^ | ||
| | | ||
| = note: the 1st parameter appears to be missing |
There was a problem hiding this comment.
error[E0061]: this function takes 1 argument but 0 arguments were supplied
--> asdf.rs:20:5
|
12 | fn missing(_i: u32) {}
| ------------------- defined here
...
20 | missing(); //~ ERROR arguments to this function are incorrect
| ^^^^^^^-- supplied 0 arguments
| |
| expected 1 argument of type `u32`
I would like to keep the current output. A possibility of improvement would be to look for local bindings that are of the appropriate type (but that doesn't need to be addressed in this PR).
| error[E0059]: one or more arguments to this function are incorrect | ||
| --> $DIR/basic.rs:34:5 | ||
| | | ||
| LL | swapped(&"", 1); | ||
| | ^^^^^^^^---^^-^ | ||
| | | | | ||
| | | these two arguments appear to be swapped | ||
| | these two arguments appear to be swapped |
There was a problem hiding this comment.
I would prefer this to be closer to
error[E0059]: multiple arguments to this function are incorrect
--> $DIR/basic.rs:34:5
|
LL | swapped(&"", 1);
| ^^^ ^ expected `&str`, found `{integer}`
| |
| expected `i32`, found `&str`
help: the arguments can be reordered to be of the appropriate types in the right positions
|
LL | swapped(1, &"");
| ^ ^^^
| error[E0059]: one or more arguments to this function are incorrect | ||
| --> $DIR/basic.rs:35:5 | ||
| | | ||
| LL | permuted(Y {}, Z {}, X {}); | ||
| | ^^^^^^^^^----^^----^^----^ | ||
| | | | | | ||
| | | | these 3 arguments appear to be in the wrong order | ||
| | | these 3 arguments appear to be in the wrong order | ||
| | these 3 arguments appear to be in the wrong order |
| error[E0059]: one or more arguments to this function are incorrect | ||
| --> $DIR/basic.rs:36:5 | ||
| | | ||
| LL | complex(1.0, H {}, &"", G{}, F::X2, Z {}, X {}, Y {}); | ||
| | ^^^^^^^^---^^----^^^^^^^---^^-----^^----^^----^^----^ | ||
| | | | | | | | | | ||
| | | | | | | | these 3 arguments appear to be in the wrong order | ||
| | | | | | | these 3 arguments appear to be in the wrong order | ||
| | | | | | these 3 arguments appear to be in the wrong order | ||
| | | | | these two arguments appear to be swapped | ||
| | | | these two arguments appear to be swapped | ||
| | | this argument appears to be extra | ||
| | the type of this argument is incorrect | ||
| | | ||
| = note: the 3rd parameter appears to be missing |
There was a problem hiding this comment.
What about something like the following?
error[E0059]: multiple arguments to this function are incorrect
--> $DIR/basic.rs:36:5
|
| fn complex(_i: u32, _s: &str, _e: E, _f: F, _g: G, _x: X, _y: Y, _z: Z ) {}
| ------- function defined here
|
...
LL | complex(1.0, H {}, &"", G{}, F::X2, Z {}, X {}, Y {});
| ^^^ ^^^^ ^^^^ ^^^^^ ^^^^ ^^^^ ^^^^
| | | || | | | |
| | | || | | | expected `Z`, found `Y`
| | | || | | expected `Y`, found `X`
| | | || | expected `X`, found `Z`
| | | || expected `G`, found `F`
| | | |expected `F`, found `G`
| | | missing argument of type `E`
| | no parameter of type `H` is needed in `complex`
| expected `u32`, found `{float}`
help: the arguments can be modified to be of the appropriate types in the right positions
|
LL | complex(<u32>, &"", <E>, F::X2, G{}, X {}, Y {}, Z {});
| ^^^^^ -- ^^^^^^^^^^ ^^^ ^^^^ ^^^^ ^^^^
|
For the float literal we could also use the helper method to suggest changing it to an integer, but first lets get the general case down to the final output.
There was a problem hiding this comment.
I hand rolled some code to construct the diagnostic for this specific case, hard coding in values/assumptions, to get a feel for the methods I might need to use, and was able to produce:
------------------------------------------
error[E0059]: multiple arguments to this function are incorrect
--> /home/vsonline/workspace/rust/src/test/ui/argument-suggestions/basic.rs:12:13
|
LL | fn complex(_i: u32, _s: &str, _e: E, _f: F, _g: G, _x: X, _y: Y, _z: Z ) {}
| ------- function defined here
...
LL | complex(1.0, H {}, &"", G{}, F::X2, Z {}, X {}, Y {});
| ^^^ ^^^^ ^^^^ ^^^^^ ^^^^ ^^^^ ^^^^ expected `Z`, found `Y`
| | | || | | |
| | | || | | expected `Y`, found `X`
| | | || | expected `X`, found `Z`
| | | || expected `G`, found `F`
| | | |expected `F`, found `G`
| | | missing argument of type `E`
| | no parameter of type `H` is needed in `complex`
| expected `u32`, found `{float}`
|
help: the arguments can be modified to be of the appropriate types in the right positions
|
LL | complex(<u32>, &"",<E>, F::X2, G {}, X {}, Y {}, Z {});
| ^^^^^ -- ^^^^ ^^^^^ ^^^^ ^^^^ ^^^^ ^^^^
error: aborting due to previous error
which is pretty close to what you had.
Here's the code that produces that:
if issues.len() > 0 {
// Try to construct a diagnostic that best summarizes the issues we found
// In general, if we found many issues, we'll emit a large diagnostic with
// a multi-part suggestion
let spans = vec![
provided_args[0].span,
provided_args[1].span,
//provided_args[3].span.shrink_to_lo(),
Span::new(BytePos(provided_args[3].span.lo().to_u32() - 1u32), provided_args[3].span.lo(), provided_args[3].span.ctxt()).shrink_to_lo(),
provided_args[3].span,
provided_args[4].span,
provided_args[5].span,
provided_args[6].span,
provided_args[7].span,
];
let highlight_sp = MultiSpan::from_spans(spans.clone());
let mut err = struct_span_err!(
tcx.sess,
highlight_sp,
E0059, // FIXME: Choose a different code
"multiple arguments to this function are incorrect"
);
if let Some(fn_def_span) = _fn_def_span {
let fn_name = Span::new(BytePos(fn_def_span.lo().to_u32() + 3u32), BytePos(fn_def_span.lo().to_u32() + 10u32), fn_def_span.ctxt());
err.span_label(fn_name, "function defined here");
}
err.span_label(spans[0], "expected `u32`, found `{float}`");
err.span_label(spans[1], "no parameter of type `H` is needed in `complex`");
err.span_label(spans[2], "missing argument of type `E`");
err.span_label(spans[3], "expected `F`, found `G`");
err.span_label(spans[4], "expected `G`, found `F`");
err.span_label(spans[5], "expected `X`, found `Z`");
err.span_label(spans[6], "expected `Y`, found `X`");
err.span_label(spans[7], "expected `Z`, found `Y`");
err.multipart_suggestion(
"the arguments can be modified to be of the appropriate types in the right positions",
vec![
(spans[0], "<u32>".to_string()),
(Span::new(spans[1].lo(), BytePos(spans[1].hi().to_u32() + 2u32), spans[1].ctxt()), "".to_string()), // eat the comma
(spans[2], "<E>, ".to_string()),
(spans[3], "F::X2".to_string()),
(spans[4], "G {}".to_string()),
(spans[5], "X {}".to_string()),
(spans[6], "Y {}".to_string()),
(spans[7], "Z {}".to_string()),
],
Applicability::Unspecified,
);
err.emit();
}
Some really awkward things in there, wondering if there's a better way:
- Growing/shrinking spans seems to be really cumbersome, any tips on that being easier?
- I have a span for the whole function definition, any tips for shrinking that to just cover the method name?
- Given a span, how can I get the code snippet for that span? for things like the expression reordered
- Given a type, how can I get a human readable string for it?
There was a problem hiding this comment.
(My plan right now is to write a generic "lots of issues" path, and then special case for some nicer errors in simpler cases (ex: only one missing arg, etc.))
|
Also, it seems like you didn't run |
Quantumplation
force-pushed
the
65853/param-heuristics
branch
from
bf096f0
to
a985712
Compare
JohnCSimon
added
S-waiting-on-author
|
@JohnCSimon I mentioned above, I'm a bit stuck here. I'm not sure how to translate a Ty or an Expr into a human readable string for the error messages. |
|
@estebank the author needs some guidance for this pr. Thanks |
|
@Quantumplation What's the status of this PR? |
|
@Muirrum All of the logic for determining WHAT to say in the error message is done, I just need to build the correct diagnostic. However, I ran into a few stumbling blocks, and when I didn't get any direction on those, it fell off my radar for a while. Specifically, if I could get answers to the following questions (in relation to a code snippet I left in a comment above), I could finish this up fairly quickly:
|
|
@estebank if you can answer the questions put forth in the last comment that unblock the author, we can push this forward :) |
jyn514
added
S-waiting-on-review
This attempts to bring better error messages to invalid method calls, by applying some heuristics to identify common mistakes. The algorithm is inspired by Levenshtein distance and longest common sub-sequence. In essence, we treat the types of the function, and the types of the arguments you provided as two "words" and compute the edits to get from one to the other. We then modify that algorithm to detect 4 cases: - A function input is missing - An extra argument was provided - The type of an argument is straight up invalid - Two arguments have been swapped - A subset of the arguments have been shuffled (We detect the last two as separate cases so that we can detect two swaps, instead of 4 parameters permuted.) It helps to understand this argument by paying special attention to terminology: "inputs" refers to the inputs being *expected* by the function, and "arguments" refers to what has been provided at the call site. The basic sketch of the algorithm is as follows: - Construct a boolean grid, with a row for each argument, and a column for each input. The cell [i, j] is true if the i'th argument could satisfy the j'th input. - If we find an argument that could satisfy no inputs, provided for an input that can't be satisfied by any other argument, we consider this an "invalid type". - Extra arguments are those that can't satisfy any input, provided for an input that *could* be satisfied by another argument. - Missing inputs are inputs that can't be satisfied by any argument, where the provided argument could satisfy another input - Swapped / Permuted arguments are identified with a cycle detection algorithm. As each issue is found, we remove the relevant inputs / arguments and check for more issues. If we find no issues, we match up any "valid" arguments, and start again. Note that there's a lot of extra complexity: - We try to stay efficient on the happy path, only computing the diagonal until we find a problem, and then filling in the rest of the matrix. - Closure arguments are wrapped in a tuple and need to be unwrapped - We need to resolve closure types after the rest, to allow the most specific type constraints - We need to handle imported C functions that might be variadic in their inputs. I tried to document a lot of this in comments in the code and keep the naming clear.
Much more to come as code review progresses.
In a lot of these cases, I prefer the way I formatted the code, but I'll defer to the tidy script :)
I still need to handle this properly, but I just threw this in to avoid the ICE. I expect I'll have several rounds with code reviewers to refine things.
Quantumplation
force-pushed
the
65853/param-heuristics
branch
from
a985712
to
5bf45b3
Compare
|
I just rebased this onto master. I'm still waiting for some answers to the above questions, I kind of forgot about this for a while. 😅 Over the next week I'll try to do some digging on my own, now that I understand Rust better and the rust-analyzer is working better on the code base to help me navigate, but if anyone has tips, it'd be much appreciated! |
For some reason I missed that you could just format!(ty) and get a human readable type for it. There's still lots to work out - What heuristics should we have around when to display the complicated labels? ex: with just one error, it's probably better for the user to provide the error but not the suggestion? Are "missing" parameter suggestions, where we don't have something to stick in there useful? - The semantics around the spans to use; when should we eat a comma, dealing with multiple inserted params, etc.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
| Swap(usize, usize), | ||
| Permutation(Vec<Option<usize>>), |
There was a problem hiding this comment.
Why are Swap and Permutation considered different?
There was a problem hiding this comment.
IIRC I did this so we could provide a better worded error message in the case of swapped arguments.
There was a problem hiding this comment.
Example:
fn my_func(A, B, C, D) {}
my_func(B, A, D, C); // Suggest two swaps, instead of a full permutation
Co-authored-by: Joshua Nelson <joshua@yottadb.com>
|
The job Click to see the possible cause of the failure (guessed by this bot) |
There were some test cases where "" wasn't typechecking against &str in the "soft" typechecking path (the one that doesn't persist new constraints.) I found a method that appears to typecheck without persisting new constraints, but I can't be sure, so I'll need to double check that. For now, this new expression typechecking method works well. This also tries to carefully handle eliminating extra arguments. Spans are tricky, as we need to eliminate commas correctly, and the suggestion code gets angry if we have overlapping spans, so this tries to find contiguous blocks to eliminate. I'll probably simplify this now that I understand the logic a bit better, but it's late, so I'm going to bed.
| | | | | | ||
| | | | this parameter of type {float} isn't needed for one_arg | ||
| | | this parameter of type &'static str isn't needed for one_arg | ||
| | help: removing these arguments may help |
There was a problem hiding this comment.
| | help: removing these arguments may help | |
| | help: remove these arguments |
It would be really nice to give a structured suggestion: one_arg(1);, but I understand if that's tricky.
There was a problem hiding this comment.
as far as I can tell, if you only provide one suggestion, it renders it inline like this, but if you provide more than one, it renders as a separate block. So this is showing up this way because of my refactor to handle commas correctly I'm deleting multiple arguments in one block, rather than each individual block. I might split it up again, the logic for handling the commas correctly is just tricky lol.
There was a problem hiding this comment.
consider using span_suggestion_verbose to make it always render on its own subwindow
There was a problem hiding this comment.
There's no multipart_suggestion_verbose; should I add it? Should I record each change as a separate suggestion?
There was a problem hiding this comment.
I think that multipart_suggestions are always verbose (there is no way to display them without them being shown in a span label unless there's a single suggestion... at which point I think the inline output would be desired).
Edit: adding it is reasonable.
This forces it to always show the subwindow for the suggestion. Also uses HasPlaceholders when appropriate.
The 'extra' spans are one iteration better, but still wrong, especially when there are arguments missing in the middle; Need another pass on this
Rather than dropping individual suggestion spans, I just re-construct the function call from scratch. This makes fiddling with commas sooo much easier. The highlighting of individual errors is temporarily disabled, I'll add that back in temporarily.
@estebank suggested we only annotate these if there are 5 or less. In addition to the suggestion at the bottom of the error, we label the specific problems we encounter without suggesting how to fix it. This also ladders the suggestion text: if there's just one thing to suggest, use specific language about the suggestion; however, if there are multiple suggestions, use a generic "did you mean" Even though the comment on span_suggestion suggests against this, Camelid on zulip pointed out that there are already a few suggestions like this, and any other language sounds slightly awkward.
I left several tests failing, as they are likely cases my code needs to handle
|
@Quantumplation any updates? |
|
@Dylan-DPC Yea; a few weeks ago I prodded people on zulip again and got some answers that unblocked me, so I've been working through this again. The current status is: it works for the obvious cases, and there's a ton of corner cases in tests to handle now, that I'm trying to work through. The things I'm currently trying to figure out:
|
|
@estebank can you answer these questions and help the author? thanks |
| @@ -307,6 +307,28 @@ impl Diagnostic { | |||
| self | |||
| } | |||
|
|
|||
| /// Show a suggestion that has multiple parts to it, always as it's own subwindow. | |||
| /// In other words, multiple changes need to be applied as part of this suggestion. | |||
| pub fn multipart_suggestion_verbose( | |||
There was a problem hiding this comment.
Can you split this into a separate PR?
| @@ -280,6 +280,20 @@ impl<'a> DiagnosticBuilder<'a> { | |||
| self | |||
| } | |||
|
|
|||
| /// See [`Diagnostic::multipart_suggestion_verbose()`]. | |||
| pub fn multipart_suggestion_verbose( | |||
| // Span enclosing the call site | ||
| call_span: Span, | ||
| // Expression of the call site | ||
| call_expr: &'tcx hir::Expr<'tcx>, | ||
| // Types (as defined in the *signature* of the target function) | ||
| formal_input_tys: &[Ty<'tcx>], | ||
| // More specific expected types, after unifying with caller output types | ||
| expected_input_tys: &[Ty<'tcx>], | ||
| // The expressions for each provided argument | ||
| provided_args: &'tcx [hir::Expr<'tcx>], | ||
| // Whether the function is variadic, for example when imported from C |
There was a problem hiding this comment.
I really like the names/comments here. Can you split them into a separate PR too?
|
@Quantumplation are you still around? If so, I'd like to help move this PR forward. To start, there are a few changes that I think would be nice to move to a separate PR and would make this review easier. r? @jackh726 |
|
@jackh726 I would love to! I've been super busy with my startup recently, hence why it's fallen by the wayside, but I'm happy to help get this over the finish line! |
jackh726
added
S-waiting-on-author
|
Btw I've rebased this branch and have fixed some of the error messages (including re-emitting some of the deleted suggestions) here: https://github.com/jackh726/rust/tree/Quantumplation/65853%2Fparam-heuristics I'm planning on starting to submit PRs incrementally (to whatever extent possible) for this. |
…dtwco Some cleanups around check_argument_types Split out in ways from my rebase/continuation of rust-lang#71827 Commits are mostly self-explanatory and these changes should be fairly straightforward
…dtwco Some cleanups around check_argument_types Split out in ways from my rebase/continuation of rust-lang#71827 Commits are mostly self-explanatory and these changes should be fairly straightforward
…dtwco Some cleanups around check_argument_types Split out in ways from my rebase/continuation of rust-lang#71827 Commits are mostly self-explanatory and these changes should be fairly straightforward
|
Closing in favor of #92364 |
…euristics, r=estebank Better method call error messages Rebase/continuation of rust-lang#71827 ~Based on rust-lang#92360~ ~Based on rust-lang#93118~ There's a decent description in rust-lang#71827 that I won't copy here (for now at least) In addition to rebasing, I've tried to restore most of the original suggestions for invalid arguments. Unfortunately, this does make some of the errors a bit verbose. To fix this will require a bit of refactoring to some of the generalized error suggestion functions, and I just don't have the time to go into it right now. I think this is in a state that the error messages are overall better than before without a reduction in the suggestions given. ~I've tried to split out some of the easier and self-contained changes into separate commits (mostly in rust-lang#92360, but also one here). There might be more than can be done here, but again just lacking time.~ r? `@estebank` as the original reviewer of rust-lang#71827
Summary
This attempts to bring better error messages to invalid method calls, by applying some heuristics to identify common mistakes. Closes #65853.
So, as just one example, instead of:
We can provide something along the lines of
That being said, the error messages themselves are what I'm seeking the most feedback on
Terminology
The thing that will probably help the most when reading this code is being strict about the following terms:
Algorithm
Overview
The algorithm is inspired by Levenshtein distance and longest common sub-sequence. In essence, we treat the types of the function, and the types of the arguments you provided as two "words" and compute the edits to get from one to the other.
We then extend that algorithm to distinguish 5 cases:
(We detect the last two as separate cases so that we can detect two swaps, instead of 4 parameters permuted.)
Details
The basic sketch of the algorithm is as follows:
As each issue is found, we remove the relevant inputs / arguments and check for more issues. If we find no issues, we match up any "valid" arguments, and start again.
Even more details
Note that there's a lot of extra complexity:
I tried to document a lot of this in comments in the code and keep the naming clear.
Remaining work for this PR
complexcase from the test I added in particular shows how noisy it can getPotential extensions
r? @estebank since he helped me a lot with this