Skip to content
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.

Sign up for GitHub

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

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

Merged
merged 3 commits into from
Jun 9, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c9bd8d
fix indexing and remove some unwraps from arg mismatch diagnostic
compiler-errors May 30, 2022
fc93461
fix typo
compiler-errors May 30, 2022
4b26d41
add regression test
compiler-errors May 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 25 additions & 18 deletions compiler/rustc_typeck/src/check/fn_ctxt/checks.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -445,16 +445,14 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let found_errors = !errors.is_empty();

errors.drain_filter(|error| {
let Error::Invalid(input_idx, arg_idx, Compatibility::Incompatible(error)) = error else { return false };
let Error::Invalid(input_idx, arg_idx, Compatibility::Incompatible(Some(e))) = error else { return false };
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());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let cause = &self.misc(provided_args[*input_idx].span);
let trace = TypeTrace::types(cause, true, expected_ty, provided_ty);
if let Some(e) = error {
if !matches!(trace.cause.as_failure_code(e), FailureCode::Error0308(_)) {
self.report_and_explain_type_error(trace, e).emit();
return true;
}
if !matches!(trace.cause.as_failure_code(e), FailureCode::Error0308(_)) {
self.report_and_explain_type_error(trace, e).emit();
return true;
}
false
});
Expand Down Expand Up @@ -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]
Copy link
Member Author

@compiler-errors compiler-errors May 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.map(|ty| ty.0)
.unwrap_or_else(|| tcx.ty_error());
let expected_ty = self.resolve_vars_if_possible(expected_ty);
let provided_ty = self.resolve_vars_if_possible(provided_ty);
let cause = &self.misc(provided_args[*input_idx].span);
Expand All @@ -595,7 +595,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
&mut err,
&provided_args[*input_idx],
provided_ty,
final_arg_types[*input_idx].map(|ty| ty.1).unwrap(),
final_arg_types[*input_idx]
.map(|ty| ty.1)
.unwrap_or_else(|| tcx.ty_error()),
None,
None,
);
Expand Down Expand Up @@ -652,7 +654,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
match error {
Error::Invalid(input_idx, arg_idx, compatibility) => {
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());
let expected_ty = self.resolve_vars_if_possible(expected_ty);
let provided_ty = self.resolve_vars_if_possible(provided_ty);
if let Compatibility::Incompatible(error) = &compatibility {
Expand All @@ -674,8 +678,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
self.emit_coerce_suggestions(
&mut err,
&provided_args[input_idx],
final_arg_types[input_idx].map(|ty| ty.0).unwrap(),
final_arg_types[input_idx].map(|ty| ty.1).unwrap(),
provided_ty,
// FIXME(compiler-errors): expected_ty?
final_arg_types[input_idx]
Copy link
Member Author

@compiler-errors compiler-errors May 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

.map(|ty| ty.1)
.unwrap_or_else(|| tcx.ty_error()),
None,
None,
);
Expand Down Expand Up @@ -860,7 +867,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let first_expected_ty =
self.resolve_vars_if_possible(expected_input_tys[arg_idx]);
let first_provided_ty = if let Some((ty, _)) = final_arg_types[input_idx] {
format!(",found `{}`", ty)
format!(", found `{}`", ty)
} else {
String::new()
};
Expand All @@ -872,7 +879,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
self.resolve_vars_if_possible(expected_input_tys[other_arg_idx]);
let other_provided_ty =
if let Some((ty, _)) = final_arg_types[other_input_idx] {
format!(",found `{}`", ty)
format!(", found `{}`", ty)
} else {
String::new()
};
Expand All @@ -888,14 +895,14 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
Error::Permutation(args) => {
for (dst_arg, dest_input) in args {
let expected_ty =
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]);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

let provided_ty = if let Some((ty, _)) = final_arg_types[dest_input] {
format!(", found `{}`", ty)
} else {
String::new()
};
labels.push((
provided_args[dst_arg].span,
provided_args[dest_input].span,
format!("expected `{}`{}", expected_ty, provided_ty),
));
}
Expand Down
10 changes: 5 additions & 5 deletions src/test/ui/argument-suggestions/basic.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,9 @@ error[E0308]: arguments to this function are incorrect
--> $DIR/basic.rs:23:5
|
LL | swapped("", 1);
| ^^^^^^^ -- - expected `&str`,found `{integer}`
| ^^^^^^^ -- - expected `&str`, found `{integer}`
| |
| expected `u32`,found `&'static str`
| expected `u32`, found `&'static str`
|
note: function defined here
--> $DIR/basic.rs:16:4
Expand All @@ -66,10 +66,10 @@ error[E0308]: arguments to this function are incorrect
--> $DIR/basic.rs:24:5
|
LL | permuted(Y {}, Z {}, X {});
| ^^^^^^^^ ---- ---- ---- expected `Z`,found `X`
| ^^^^^^^^ ---- ---- ---- expected `Z`, found `X`
| | |
| | expected `Y`,found `Z`
| expected `X`,found `Y`
| | expected `Y`, found `Z`
| expected `X`, found `Y`
|
note: function defined here
--> $DIR/basic.rs:17:4
Expand Down
14 changes: 14 additions & 0 deletions src/test/ui/argument-suggestions/issue-97484.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
struct A;
struct B;
struct C;
struct D;
struct E;
struct F;
struct G;

fn foo(a: &A, d: D, e: &E, g: G) {}

fn main() {
foo(&&A, B, C, D, E, F, G);
//~^ ERROR this function takes 4 arguments but 7 arguments were supplied
}
27 changes: 27 additions & 0 deletions src/test/ui/argument-suggestions/issue-97484.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
error[E0061]: this function takes 4 arguments but 7 arguments were supplied
--> $DIR/issue-97484.rs:12:5
|
LL | foo(&&A, B, C, D, E, F, G);
| ^^^ - - - argument unexpected
| | |
| | argument of type `&E` unexpected
| argument of type `D` unexpected
|
note: function defined here
--> $DIR/issue-97484.rs:9:4
|
LL | fn foo(a: &A, d: D, e: &E, g: G) {}
| ^^^ ----- ---- ----- ----
help: consider removing the ``
|
LL - foo(&&A, B, C, D, E, F, G);
LL + foo(&&A, B, C, D, E, F, G);
|
Comment on lines +15 to +19
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

help: remove the extra arguments
|
LL | foo(&&A, D, {&E}, G);
| ~~~~~~~~~~~~~~~~~~~~
Comment on lines +20 to +23
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


error: aborting due to previous error

For more information about this error, try `rustc --explain E0061`.
8 changes: 4 additions & 4 deletions src/test/ui/argument-suggestions/mixed_cases.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,10 @@ error[E0308]: arguments to this function are incorrect
--> $DIR/mixed_cases.rs:20:3
|
LL | three_args("", X {}, 1);
| ^^^^^^^^^^ -- ---- - expected `&str`,found `{integer}`
| ^^^^^^^^^^ -- ---- - expected `&str`, found `{integer}`
| | |
| | expected `f32`, found struct `X`
| expected `i32`,found `&'static str`
| expected `i32`, found `&'static str`
|
note: function defined here
--> $DIR/mixed_cases.rs:6:4
Expand All @@ -98,8 +98,8 @@ LL | three_args("", 1);
| ^^^^^^^^^^ -- -
| | |
| | an argument of type `f32` is missing
| | expected `&str`,found `{integer}`
| expected `i32`,found `&'static str`
| | expected `&str`, found `{integer}`
| expected `i32`, found `&'static str`
|
note: function defined here
--> $DIR/mixed_cases.rs:6:4
Expand Down
16 changes: 8 additions & 8 deletions src/test/ui/argument-suggestions/permuted_arguments.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@ error[E0308]: arguments to this function are incorrect
--> $DIR/permuted_arguments.rs:10:3
|
LL | three_args(1.0, "", 1);
| ^^^^^^^^^^ --- -- - expected `&str`,found `{integer}`
| ^^^^^^^^^^ --- -- - expected `&str`, found `{integer}`
| | |
| | expected `f32`,found `&'static str`
| expected `i32`,found `{float}`
| | expected `f32`, found `&'static str`
| expected `i32`, found `{float}`
|
note: function defined here
--> $DIR/permuted_arguments.rs:5:4
Expand All @@ -21,12 +21,12 @@ error[E0308]: arguments to this function are incorrect
--> $DIR/permuted_arguments.rs:12:3
|
LL | many_args(X {}, Y {}, 1, 1.0, "");
| ^^^^^^^^^ ---- ---- - --- -- expected `Y`,found `&'static str`
| ^^^^^^^^^ ---- ---- - --- -- expected `Y`, found `&'static str`
| | | | |
| | | | expected `X`,found `{float}`
| | | expected `&str`,found `{integer}`
| | expected `f32`,found `Y`
| expected `i32`,found `X`
| | | | expected `X`, found `{float}`
| | | expected `&str`, found `{integer}`
| | expected `f32`, found `Y`
| expected `i32`, found `X`
|
note: function defined here
--> $DIR/permuted_arguments.rs:6:4
Expand Down
24 changes: 12 additions & 12 deletions src/test/ui/argument-suggestions/swapped_arguments.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ error[E0308]: arguments to this function are incorrect
--> $DIR/swapped_arguments.rs:8:3
|
LL | two_args(1.0, 1);
| ^^^^^^^^ --- - expected `f32`,found `{integer}`
| ^^^^^^^^ --- - expected `f32`, found `{integer}`
| |
| expected `i32`,found `{float}`
| expected `i32`, found `{float}`
|
note: function defined here
--> $DIR/swapped_arguments.rs:3:4
Expand All @@ -20,9 +20,9 @@ error[E0308]: arguments to this function are incorrect
--> $DIR/swapped_arguments.rs:9:3
|
LL | three_args(1.0, 1, "");
| ^^^^^^^^^^ --- - expected `f32`,found `{integer}`
| ^^^^^^^^^^ --- - expected `f32`, found `{integer}`
| |
| expected `i32`,found `{float}`
| expected `i32`, found `{float}`
|
note: function defined here
--> $DIR/swapped_arguments.rs:4:4
Expand All @@ -38,9 +38,9 @@ error[E0308]: arguments to this function are incorrect
--> $DIR/swapped_arguments.rs:10:3
|
LL | three_args( 1, "", 1.0);
| ^^^^^^^^^^ -- --- expected `&str`,found `{float}`
| ^^^^^^^^^^ -- --- expected `&str`, found `{float}`
| |
| expected `f32`,found `&'static str`
| expected `f32`, found `&'static str`
|
note: function defined here
--> $DIR/swapped_arguments.rs:4:4
Expand All @@ -56,9 +56,9 @@ error[E0308]: arguments to this function are incorrect
--> $DIR/swapped_arguments.rs:11:3
|
LL | three_args( "", 1.0, 1);
| ^^^^^^^^^^ -- - expected `&str`,found `{integer}`
| ^^^^^^^^^^ -- - expected `&str`, found `{integer}`
| |
| expected `i32`,found `&'static str`
| expected `i32`, found `&'static str`
|
note: function defined here
--> $DIR/swapped_arguments.rs:4:4
Expand All @@ -74,11 +74,11 @@ error[E0308]: arguments to this function are incorrect
--> $DIR/swapped_arguments.rs:13:3
|
LL | four_args(1.0, 1, X {}, "");
| ^^^^^^^^^ --- - ---- -- expected `X`,found `&'static str`
| ^^^^^^^^^ --- - ---- -- expected `X`, found `&'static str`
| | | |
| | | expected `&str`,found `X`
| | expected `f32`,found `{integer}`
| expected `i32`,found `{float}`
| | | expected `&str`, found `X`
| | expected `f32`, found `{integer}`
| expected `i32`, found `{float}`
|
note: function defined here
--> $DIR/swapped_arguments.rs:5:4
Expand Down