Fix indices and remove some unwraps in arg mismatch algorithm #97557

compiler-errors · 2022-05-30T18:45:05Z

This is a more conservative fix than #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 #92364, and @estebank as reviewer of that PR.
fixes #97484
r? @jackh726 this should be much easier to review than the other PR 😅

compiler-errors · 2022-05-30T18:47:08Z

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

                let expected_ty = expected_input_tys[*arg_idx];
-                let provided_ty = final_arg_types[*input_idx].map(|ty| ty.0).unwrap();
+                let provided_ty = final_arg_types[*input_idx].map(|ty| ty.0).unwrap_or_else(|| tcx.ty_error());


final_arg_types is a vector of length provided_args.len(), but is only populated with Some (by demand_compatible) up to expected_input_tys.len(). This unwrap was panicking in a bunch of cases.

compiler-errors · 2022-05-30T18:48:11Z

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

-                                self.resolve_vars_if_possible(expected_input_tys[dest_input]);
-                            let provided_ty = if let Some((ty, _)) = final_arg_types[dst_arg] {
-                                format!(",found `{}`", ty)
+                                self.resolve_vars_if_possible(expected_input_tys[dst_arg]);


I think these were swapped -- dst_arg is an "expected argument"-like index, and dst_input is a "provided argument"-like index.

compiler-errors · 2022-05-30T18:49:50Z

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

@@ -585,7 +583,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                )) = errors.iter().next()
                {
                    let expected_ty = expected_input_tys[*arg_idx];
-                    let provided_ty = final_arg_types[*arg_idx].map(|ty| ty.0).unwrap();
+                    let provided_ty = final_arg_types[*input_idx]


Should be using input_idx. Not sure if this is "correct" (though I think it is), but regardless, if there are more expected args than provided args, then using arg_idx might be out of bounds.

compiler-errors · 2022-05-30T18:51:05Z

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

-                            final_arg_types[input_idx].map(|ty| ty.1).unwrap(),
+                            provided_ty,
+                            // FIXME(compiler-errors): expected_ty?
+                            final_arg_types[input_idx]


This should actually probably be expected_ty instead of final_arg_types[input_idx].1, but this will just provide a bad suggestion, and not panic.

What about changing emit_coerce_suggestions to take an Option here instead (and make it not emit the invalid suggestion when it is None)?

but emit_coerce_suggestions can't do anything if we don't have a target type to coerce to, right?

estebank · 2022-05-31T16:36:22Z

src/test/ui/argument-suggestions/issue-97484.stderr

+help: consider removing the ``
+   |
+LL -     foo(&&A, B, C, D, E, F, G);
+LL +     foo(&&A, B, C, D, E, F, G);
+   | 


This is buggy (which, fair, you're sidestepping the ICE). Given that after this we suggest removing some args, it tells me that we can better handle this case to not emit it at all.

I actually know how to fix this one, and I think I fixed it on the more extensive fix.

The technical explanation is that we're trying to coerce &&A into &&A, and the current suggestion logic goes "ah yes, dereference it zero times" (well, actually, remove & zero times).

estebank · 2022-05-31T16:36:47Z

src/test/ui/argument-suggestions/issue-97484.stderr

+help: remove the extra arguments
+   |
+LL |     foo(&&A, D, {&E}, G);
+   |     ~~~~~~~~~~~~~~~~~~~~


This tells me that we should teach the suggestion to add and remove borrows as well :)

This should fall out of coercion suggestions which we attempt when we see an "invalid" argument.

apiraino · 2022-06-08T18:51:44Z

Beta backport not accepted in favor of #97701. T-compiler Zulip discussion, especially around this comment.

@rustbot label -beta-nominated

compiler-errors · 2022-06-08T19:13:13Z

@estebank and @jackh726, would appreciate a review from either of y'all sometime so we could land this on nightly, especially if the larger rewrite is going to take a longer time to review

jackh726 · 2022-06-08T19:37:18Z

@bors r+

bors · 2022-06-08T19:37:20Z

📌 Commit 4b26d41 has been approved by jackh726

…piler-errors Rollup of 9 pull requests Successful merges: - rust-lang#97557 (Fix indices and remove some unwraps in arg mismatch algorithm) - rust-lang#97830 (Add std::alloc::set_alloc_error_hook example) - rust-lang#97856 (Don't suggest adding `let` in certain `if` conditions) - rust-lang#97857 (Suggest escaping `box` as identifier) - rust-lang#97871 (Suggest using `iter()` or `into_iter()` for `Vec`) - rust-lang#97882 (Add regresion test for rust-lang#67498) - rust-lang#97883 (Remove `ignore-compare-mode-nll` annotations from tests) - rust-lang#97891 (Update books) - rust-lang#97894 (Fix polonius compare mode.) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

…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

compiler-errors added the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 30, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 30, 2022

rust-highfive assigned jackh726 May 30, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 30, 2022

compiler-errors commented May 30, 2022

View reviewed changes

compiler-errors added 3 commits May 30, 2022 12:08

fix indexing and remove some unwraps from arg mismatch diagnostic

6c9bd8d

fix typo

fc93461

add regression test

4b26d41

compiler-errors force-pushed the arg-mismatch-mini branch from 8986bd2 to 4b26d41 Compare May 30, 2022 19:08

compiler-errors mentioned this pull request May 30, 2022

Use typed indices in argument mismatch algorithm #97542

Merged

estebank reviewed May 31, 2022

View reviewed changes

compiler-errors mentioned this pull request Jun 3, 2022

[beta] Revert arg matrix algorithm from check_argument_types #97701

Merged

rustbot removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 8, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 8, 2022

compiler-errors mentioned this pull request Jun 8, 2022

Rollup of 9 pull requests #97896

Merged

bors merged commit 1922f0b into rust-lang:master Jun 9, 2022

rustbot added this to the 1.63.0 milestone Jun 9, 2022

compiler-errors deleted the arg-mismatch-mini branch August 11, 2023 19:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix indices and remove some unwraps in arg mismatch algorithm #97557

Fix indices and remove some unwraps in arg mismatch algorithm #97557

compiler-errors commented May 30, 2022

compiler-errors May 30, 2022

compiler-errors May 30, 2022

compiler-errors May 30, 2022 •

edited

Loading

compiler-errors May 30, 2022 •

edited

Loading

estebank May 31, 2022

compiler-errors May 31, 2022

estebank May 31, 2022

compiler-errors May 31, 2022

compiler-errors May 31, 2022

estebank May 31, 2022

compiler-errors May 31, 2022

apiraino commented Jun 8, 2022

compiler-errors commented Jun 8, 2022

jackh726 commented Jun 8, 2022

bors commented Jun 8, 2022

Fix indices and remove some unwraps in arg mismatch algorithm #97557

Fix indices and remove some unwraps in arg mismatch algorithm #97557

Conversation

compiler-errors commented May 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compiler-errors May 30, 2022 • edited Loading

Choose a reason for hiding this comment

compiler-errors May 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apiraino commented Jun 8, 2022

compiler-errors commented Jun 8, 2022

jackh726 commented Jun 8, 2022

bors commented Jun 8, 2022

compiler-errors May 30, 2022 •

edited

Loading

compiler-errors May 30, 2022 •

edited

Loading