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.

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

Sort the errors from arguments checking so that suggestions are handled properly #112762

Merged
merged 2 commits into from
Jun 20, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 10 additions & 3 deletions compiler/rustc_hir_typeck/src/fn_ctxt/arg_matrix.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::cmp;

use core::cmp::Ordering;
use rustc_index::IndexVec;
use rustc_middle::ty::error::TypeError;
use std::cmp;

rustc_index::newtype_index! {
#[debug_format = "ExpectedIdx({})"]
Expand Down Expand Up @@ -177,7 +177,7 @@ impl<'tcx> ArgMatrix<'tcx> {
// If an argument is unsatisfied, and the input in its position is useless
// then the most likely explanation is that we just got the types wrong
(true, true, true, true) => return Some(Issue::Invalid(i)),
// Otherwise, if an input is useless, then indicate that this is an extra argument
// Otherwise, if an input is useless then indicate that this is an extra input
(true, _, true, _) => return Some(Issue::Extra(i)),
// Otherwise, if an argument is unsatisfiable, indicate that it's missing
(_, true, _, true) => return Some(Issue::Missing(i)),
Expand Down Expand Up @@ -376,6 +376,13 @@ impl<'tcx> ArgMatrix<'tcx> {
};
}

// sort errors with same type by the order they appear in the source
// so that suggestion will be handled properly, see #112507
errors.sort_by(|a, b| match (a, b) {
(Error::Missing(i), Error::Missing(j)) => i.cmp(j),
Copy link
Member

Choose a reason for hiding this comment

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

I think you could probably just derive Ord on Error and just use .sort()?

Copy link
Member Author

Choose a reason for hiding this comment

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

we need to customize PartialOrd for Error and I think only ordering on Missing and Extra is enough(which will involve adding or removing arguments).

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it hurts to just totally sort the list. I think right now the sort implementation is not actually valid because it returns Equal for other elements 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, I just validated with some playground code, it won't sort in some cases, let me fix it 👍

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 actually, this implementation is enough, because it makes sure the consecutive Extra and Missing ordered 😆, which is the scenario that makes sub-suggestions buggy.

Anyway, let's make it all ordered.

(Error::Extra(i), Error::Extra(j)) => i.cmp(j),
_ => Ordering::Equal,
});
return (errors, matched_inputs);
}
}
12 changes: 12 additions & 0 deletions tests/ui/argument-suggestions/issue-112507.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
pub enum Value {
Float(Option<f64>),
}

fn main() {
let _a = Value::Float( //~ ERROR this enum variant takes 1 argument but 4 arguments were supplied
0,
None,
None,
0,
);
}
27 changes: 27 additions & 0 deletions tests/ui/argument-suggestions/issue-112507.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
error[E0061]: this enum variant takes 1 argument but 4 arguments were supplied
--> $DIR/issue-112507.rs:6:14
|
LL | let _a = Value::Float(
| ^^^^^^^^^^^^
LL | 0,
| - unexpected argument of type `{integer}`
LL | None,
LL | None,
| ---- unexpected argument of type `Option<_>`
LL | 0,
| - unexpected argument of type `{integer}`
|
note: tuple variant defined here
--> $DIR/issue-112507.rs:2:5
|
LL | Float(Option<f64>),
| ^^^^^
help: remove the extra arguments
|
LL ~ ,
LL ~ None);
|

error: aborting due to previous error

For more information about this error, try `rustc --explain E0061`.