Better method call error messages #92364

jackh726 · 2021-12-28T18:37:49Z

Rebase/continuation of #71827

~~Based on #92360~~
~~Based on #93118~~

There's a decent description in #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 #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 #71827

jackh726 · 2021-12-29T00:41:40Z

I will resolve the build failures after first round of review ~~or after #92360 gets merged~~

bors · 2022-01-14T21:08:03Z

☔ The latest upstream changes (presumably #92883) made this pull request unmergeable. Please resolve the merge conflicts.

compiler/rustc_typeck/src/check/fn_ctxt/checks.rs

estebank · 2022-01-19T00:10:23Z

compiler/rustc_typeck/src/check/fn_ctxt/checks.rs

+                        if input_ty.is_unit() {
+                            suggested_inputs[input_idx] = Some("()".to_string());
+                        } else {
+                            suggested_inputs[input_idx] = Some(format!("{{{}}}", input_ty));


we have some code to suggest default values for some types, that could be used here.

Can we leave this for followup, or do you think it needs to be done in this PR?

Follow up PR is fine.

estebank · 2022-01-19T00:17:04Z

compiler/rustc_typeck/src/check/fn_ctxt/checks.rs

+                            &compatibility_matrix[idx][idx]
+                        {
+                            let provided_ty = final_arg_types[input_idx].map(|ty| ty.0).unwrap();
+                            let cause = &self.misc(provided_args[input_idx].span);


We might want to create a specific ObligationCauseCode for this case, IIF there's value in that.

compiler/rustc_typeck/src/check/fn_ctxt/checks.rs

estebank · 2022-01-19T00:20:32Z

compiler/rustc_typeck/src/check/fn_ctxt/checks.rs

+                        ));
+                        suggestion_text = match suggestion_text {
+                            None => Some("remove the extra argument"),
+                            Some(_) => Some("did you mean"),


Let's think about an alternative to "did you mean"

Can we leave this as a followup?

compiler/rustc_typeck/src/check/fn_ctxt/checks.rs

src/test/ui/argument-suggestions/basic.stderr

compiler/rustc_typeck/src/check/fn_ctxt/checks.rs

src/test/ui/argument-suggestions/basic.stderr

estebank · 2022-01-19T21:23:20Z

src/test/ui/argument-suggestions/basic.stderr

+help: remove the extra argument
+   |
+LL |     extra();
+   |     ~~~~~~~


Ideally, given the conversation we had yesterday, I would like this to be, but it doesn't have to be in this PR

Suggested change

help: remove the extra argument

|

LL | extra();

| ~~~~~~~

help: remove the extra argument

|

LL - extra("");

LL + extra();

|

Let's leave this for followup.

estebank · 2022-04-15T02:18:50Z

src/test/ui/hrtb/issue-58451.stderr

+help: provide the argument
+   |
+LL |     f(&[f({_})]);
+   |         ~~~~~~


I'm worried that the {_} placeholder might be a bit abstract, but we can iterate on this after landing the PR.

estebank · 2022-04-15T02:19:28Z

src/test/ui/fn/fn-item-type.rs

@@ -45,7 +45,7 @@ fn main() {

    eq(foo::<u8>, bar::<u8> as fn(isize) -> isize);
    //~^ ERROR mismatched types
-    //~| expected fn item `fn(_) -> _ {foo::<u8>}`
+    //~| expected type `fn(_) -> _ {foo::<u8>}`


Slightly surprised at this change.

estebank

I'm ok with the current changes, modulo the invalid suggestions. There's some outstanding work to bring this to par, but lets land this asap.

r=me after fixing the span for incorrect method args suggestion.

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.

jackh726 · 2022-04-16T06:44:29Z

I've rebased and fixed the suggestions. Rebasing over the tuple suggestion changes in #91530 was a bit difficult, and the code is a bit duplicated. But I'd rather land this now and cleanup in-tree, rather than risk bitrotting even more.

@bors r=estebank p=1 rollup=never

bors · 2022-04-16T06:44:31Z

📌 Commit b6c87c5 has been approved by estebank

jackh726 · 2022-04-16T06:46:08Z

@bors p=10 (bitrotty and I want this to land before the rollup)

bors · 2022-04-16T06:55:32Z

⌛ Testing commit b6c87c5 with merge 07bb916...

bors · 2022-04-16T09:19:20Z

☀️ Test successful - checks-actions
Approved by: estebank
Pushing 07bb916 to master...

rust-timer · 2022-04-16T10:33:05Z

Finished benchmarking commit (07bb916): comparison url.

Summary:

Primary benchmarks: 🎉 relevant improvements found
Secondary benchmarks: 😿 relevant regressions found

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	0	4	3	0	3
mean²	N/A	0.8%	-1.1%	N/A	-1.1%
max	N/A	1.2%	-1.5%	N/A	-1.5%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

number of relevant changes ↩
the arithmetic mean of the percent change ↩

pnkfelix · 2022-04-21T02:23:45Z

Visiting for weekly performance triage.

The main regression here was to externs but there's a lot of historical noise in the data for externs, and I do not think it is a trustworthy benchmark for our purposes here at this point.

@rustbot label: +perf-regression-triaged

flip1995 · 2022-05-02T12:42:46Z

compiler/rustc_typeck/src/check/fn_ctxt/checks.rs

+            errors.drain_filter(|error| {
+                let Error::Invalid(input_idx, Compatibility::Incompatible(error)) = error else { return false };
+                let expected_ty = expected_input_tys[*input_idx];
+                let provided_ty = final_arg_types[*input_idx].map(|ty| ty.0).unwrap();


@jackh726 This line causes an ICE when compiling chalk with cargo check --all-targets --all-features. AFAICT the issue here is that input_idx is in the range of 0..expected_input_tys.len(), but the amount of passed types could be smaller. I don't have a reproducer yet.

I don't think returning the input_idx based on the final_arg_types.len() would fix the issue, but rather reverse the issue with expected_input_tys, if more arguments are passed than expected.

Maybe this Error kind should hold both indices 🤔

Minimal reproducer:

fn f(_: usize, _: &usize, _: usize) {} fn arg<T>() -> T { todo!() } fn main() { let x = arg(); // `x` must be inferred f(&x, ""); // The reference on `&x` is important to reproduce the ICE }

Opened #96638

…r=jackh726 Fix indices and remove some unwraps in arg mismatch algorithm This is a more conservative fix than rust-lang#97542, addressing some indices which were used incorectly and unwraps which are bound to panic (e.g. when the provided and expected arg counts differ). Beta nominating this as it's quite easy to cause ICEs -- I wrote a fuzzer and found hundreds of examples of ICEs. cc `@jackh726` as author of rust-lang#92364, and `@estebank` as reviewer of that PR. fixes rust-lang#97484 r? `@jackh726` this should be _much_ easier to review than the other PR 😅

…kh726 Use typed indices in argument mismatch algorithm I kinda went overboard with the renames, but in general, "arg" is renamed to "expected", and "input" is renamed to "provided", and we use new typed indices to make sure we're indexing into the right sized array. Other drive-by changes: 1. Factor this logic into a new function, so we don't need to `break 'label` to escape it. 1. Factored out dependence on `final_arg_types`, which is never populated for arguments greater than the number of expected args. Instead, we just grab the final coerced expression type from `in_progress_typeck_results`. 1. Adjust the criteria we use to print (provided) type names, before we didn't suggest anything that had infer vars, but now we suggest thing that have infer vars but aren't `_`. ~Also, sorry in advance, I kinda want to backport this but I know I have folded in a lot of unnecessary drive-by changes that might discourage that. I would be open to brainstorming how to get some of these changes on beta at least.~ edit: Minimized the ICE-fixing changes to rust-lang#97557 cc `@jackh726` as author of rust-lang#92364, and `@estebank` as reviewer of the PR. fixes rust-lang#97484

rust-highfive assigned estebank Dec 28, 2021

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 28, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 28, 2021

This comment has been minimized.

Sign in to view

jackh726 force-pushed the Quantumplation/65853/param-heuristics branch from 476c1a3 to b9e7f90 Compare January 7, 2022 04:17

This comment has been minimized.

Sign in to view