mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Rollup merge of #96823 - jackh726:params-heuristics-fix, r=estebank

Browse Source 
Properly fix #96638

Closes #96638

The main part of this change is `Error::Invalid` now returns both the input and arg indices. However, I realized the code here was kind of confusing and not internally consistent (and thus I was having trouble getting the right behavior). So I've also switched `input_indices` and `arg_indices` to more closely match some naming in `checks` (although I think a more thorough cleanup there could be beneficial). I've added comments, but essentially `input_indices` refers to *user provided* inputs and `arg_indices` refers to *expected* args.
This commit is contained in:
Dylan DPC 2022-05-10 08:24:04 +02:00 committed by GitHub
commit 9a3f17b34d
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 93 additions and 90 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

102
compiler/rustc_typeck/src/check/fn_ctxt/arg_matrix.rs
View File

@ -3,6 +3,7 @@
use rustc_middle::ty::error::TypeError; use rustc_middle::ty::error::TypeError;
// An issue that might be found in the compatibility matrix // An issue that might be found in the compatibility matrix
#[derive(Debug)]
enum Issue { enum Issue {
/// The given argument is the invalid type for the input /// The given argument is the invalid type for the input
Invalid(usize), Invalid(usize),
@ -23,9 +24,10 @@ pub(crate) enum Compatibility<'tcx> {
} }
/// Similar to `Issue`, but contains some extra information /// Similar to `Issue`, but contains some extra information
#[derive(Debug)]
pub(crate) enum Error<'tcx> { pub(crate) enum Error<'tcx> {
/// The given argument is the invalid type for the input /// The provided argument is the invalid type for the expected input
Invalid(usize, Compatibility<'tcx>), Invalid(usize, usize, Compatibility<'tcx>), // provided, expected
/// There is a missing input /// There is a missing input
Missing(usize), Missing(usize),
/// There's a superfluous argument /// There's a superfluous argument
@ -37,8 +39,15 @@ pub(crate) enum Error<'tcx> {
} }
pub(crate) struct ArgMatrix<'tcx> { pub(crate) struct ArgMatrix<'tcx> {
/// Maps the indices in the `compatibility_matrix` rows to the indices of
/// the *user provided* inputs
input_indexes: Vec<usize>, input_indexes: Vec<usize>,
/// Maps the indices in the `compatibility_matrix` columns to the indices
/// of the *expected* args
arg_indexes: Vec<usize>, arg_indexes: Vec<usize>,
/// The first dimension (rows) are the remaining user provided inputs to
/// match and the second dimension (cols) are the remaining expected args
/// to match
compatibility_matrix: Vec<Vec<Compatibility<'tcx>>>, compatibility_matrix: Vec<Vec<Compatibility<'tcx>>>,
} }
@ -52,8 +61,8 @@ pub(crate) fn new<F: FnMut(usize, usize) -> Compatibility<'tcx>>(
.map(|i| (0..minimum_input_count).map(|j| is_compatible(i, j)).collect()) .map(|i| (0..minimum_input_count).map(|j| is_compatible(i, j)).collect())
.collect(); .collect();
ArgMatrix { ArgMatrix {
input_indexes: (0..minimum_input_count).collect(), input_indexes: (0..provided_arg_count).collect(),
arg_indexes: (0..provided_arg_count).collect(), arg_indexes: (0..minimum_input_count).collect(),
compatibility_matrix, compatibility_matrix,
} }
} }
@ -61,15 +70,15 @@ pub(crate) fn new<F: FnMut(usize, usize) -> Compatibility<'tcx>>(
/// Remove a given input from consideration /// Remove a given input from consideration
fn eliminate_input(&mut self, idx: usize) { fn eliminate_input(&mut self, idx: usize) {
self.input_indexes.remove(idx); self.input_indexes.remove(idx);
for row in &mut self.compatibility_matrix { self.compatibility_matrix.remove(idx);
row.remove(idx);
}
} }
/// Remove a given argument from consideration /// Remove a given argument from consideration
fn eliminate_arg(&mut self, idx: usize) { fn eliminate_arg(&mut self, idx: usize) {
self.arg_indexes.remove(idx); self.arg_indexes.remove(idx);
self.compatibility_matrix.remove(idx); for row in &mut self.compatibility_matrix {
row.remove(idx);
}
} }
/// "satisfy" an input with a given arg, removing both from consideration /// "satisfy" an input with a given arg, removing both from consideration
@ -78,13 +87,15 @@ fn satisfy_input(&mut self, input_idx: usize, arg_idx: usize) {
self.eliminate_arg(arg_idx); self.eliminate_arg(arg_idx);
} }
// Returns a `Vec` of (user input, expected arg) of matched arguments. These
// are inputs on the remaining diagonal that match.
fn eliminate_satisfied(&mut self) -> Vec<(usize, usize)> { fn eliminate_satisfied(&mut self) -> Vec<(usize, usize)> {
let mut i = cmp::min(self.input_indexes.len(), self.arg_indexes.len()); let mut i = cmp::min(self.input_indexes.len(), self.arg_indexes.len());
let mut eliminated = vec![]; let mut eliminated = vec![];
while i > 0 { while i > 0 {
let idx = i - 1; let idx = i - 1;
if matches!(self.compatibility_matrix[idx][idx], Compatibility::Compatible) { if matches!(self.compatibility_matrix[idx][idx], Compatibility::Compatible) {
eliminated.push((self.arg_indexes[idx], self.input_indexes[idx])); eliminated.push((self.input_indexes[idx], self.arg_indexes[idx]));
self.satisfy_input(idx, idx); self.satisfy_input(idx, idx);
} }
i -= 1; i -= 1;
@ -92,7 +103,7 @@ fn eliminate_satisfied(&mut self) -> Vec<(usize, usize)> {
return eliminated; return eliminated;
} }
// Check for the above mismatch cases // Find some issue in the compatibility matrix
fn find_issue(&self) -> Option<Issue> { fn find_issue(&self) -> Option<Issue> {
let mat = &self.compatibility_matrix; let mat = &self.compatibility_matrix;
let ai = &self.arg_indexes; let ai = &self.arg_indexes;
@ -121,16 +132,6 @@ fn find_issue(&self) -> Option<Issue> {
if is_arg { if is_arg {
for j in 0..ii.len() { for j in 0..ii.len() {
// If we find at least one input this argument could satisfy // If we find at least one input this argument could satisfy
// this argument isn't completely useless
if matches!(mat[i][j], Compatibility::Compatible) {
useless = false;
break;
}
}
}
if is_input {
for j in 0..ai.len() {
// If we find at least one argument that could satisfy this input
// this argument isn't unsatisfiable // this argument isn't unsatisfiable
if matches!(mat[j][i], Compatibility::Compatible) { if matches!(mat[j][i], Compatibility::Compatible) {
unsatisfiable = false; unsatisfiable = false;
@ -138,9 +139,19 @@ fn find_issue(&self) -> Option<Issue> {
} }
} }
} }
if is_input {
for j in 0..ai.len() {
// If we find at least one argument that could satisfy this input
// this argument isn't useless
if matches!(mat[i][j], Compatibility::Compatible) {
useless = false;
break;
}
}
}
match (is_arg, is_input, useless, unsatisfiable) { match (is_input, is_arg, useless, unsatisfiable) {
// If an input is unsatisfied, and the argument in its position is useless // 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 // then the most likely explanation is that we just got the types wrong
(true, true, true, true) => return Some(Issue::Invalid(i)), (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 argument
@ -167,7 +178,7 @@ fn find_issue(&self) -> Option<Issue> {
_ => { _ => {
continue; continue;
} }
}; }
} }
// We didn't find any of the individual issues above, but // We didn't find any of the individual issues above, but
@ -254,11 +265,11 @@ fn find_issue(&self) -> Option<Issue> {
// We'll want to know which arguments and inputs these rows and columns correspond to // We'll want to know which arguments and inputs these rows and columns correspond to
// even after we delete them. // even after we delete them.
pub(crate) fn find_errors(mut self) -> (Vec<Error<'tcx>>, Vec<Option<usize>>) { pub(crate) fn find_errors(mut self) -> (Vec<Error<'tcx>>, Vec<Option<usize>>) {
let provided_arg_count = self.arg_indexes.len(); let provided_arg_count = self.input_indexes.len();
let mut errors: Vec<Error<'tcx>> = vec![]; let mut errors: Vec<Error<'tcx>> = vec![];
// For each expected argument, the matched *actual* input // For each expected argument, the matched *actual* input
let mut matched_inputs: Vec<Option<usize>> = vec![None; self.input_indexes.len()]; let mut matched_inputs: Vec<Option<usize>> = vec![None; self.arg_indexes.len()];
// Before we start looking for issues, eliminate any arguments that are already satisfied, // Before we start looking for issues, eliminate any arguments that are already satisfied,
// so that an argument which is already spoken for by the input it's in doesn't // so that an argument which is already spoken for by the input it's in doesn't
@ -269,28 +280,28 @@ pub(crate) fn find_errors(mut self) -> (Vec<Error<'tcx>>, Vec<Option<usize>>) {
// Without this elimination, the first argument causes the second argument // Without this elimination, the first argument causes the second argument
// to show up as both a missing input and extra argument, rather than // to show up as both a missing input and extra argument, rather than
// just an invalid type. // just an invalid type.
for (arg, inp) in self.eliminate_satisfied() { for (inp, arg) in self.eliminate_satisfied() {
matched_inputs[inp] = Some(arg); matched_inputs[arg] = Some(inp);
} }
while self.input_indexes.len() > 0 || self.arg_indexes.len() > 0 { while self.input_indexes.len() > 0 || self.arg_indexes.len() > 0 {
// Check for the first relevant issue
match self.find_issue() { match self.find_issue() {
Some(Issue::Invalid(idx)) => { Some(Issue::Invalid(idx)) => {
let compatibility = self.compatibility_matrix[idx][idx].clone(); let compatibility = self.compatibility_matrix[idx][idx].clone();
let input_idx = self.input_indexes[idx]; let input_idx = self.input_indexes[idx];
let arg_idx = self.arg_indexes[idx];
self.satisfy_input(idx, idx); self.satisfy_input(idx, idx);
errors.push(Error::Invalid(input_idx, compatibility)); errors.push(Error::Invalid(input_idx, arg_idx, compatibility));
} }
Some(Issue::Extra(idx)) => { Some(Issue::Extra(idx)) => {
let arg_idx = self.arg_indexes[idx];
self.eliminate_arg(idx);
errors.push(Error::Extra(arg_idx));
}
Some(Issue::Missing(idx)) => {
let input_idx = self.input_indexes[idx]; let input_idx = self.input_indexes[idx];
self.eliminate_input(idx); self.eliminate_input(idx);
errors.push(Error::Missing(input_idx)); errors.push(Error::Extra(input_idx));
}
Some(Issue::Missing(idx)) => {
let arg_idx = self.arg_indexes[idx];
self.eliminate_arg(idx);
errors.push(Error::Missing(arg_idx));
} }
Some(Issue::Swap(idx, other)) => { Some(Issue::Swap(idx, other)) => {
let input_idx = self.input_indexes[idx]; let input_idx = self.input_indexes[idx];
@ -302,24 +313,21 @@ pub(crate) fn find_errors(mut self) -> (Vec<Error<'tcx>>, Vec<Option<usize>>) {
// Subtract 1 because we already removed the "min" row // Subtract 1 because we already removed the "min" row
self.satisfy_input(max - 1, min); self.satisfy_input(max - 1, min);
errors.push(Error::Swap(input_idx, other_input_idx, arg_idx, other_arg_idx)); errors.push(Error::Swap(input_idx, other_input_idx, arg_idx, other_arg_idx));
matched_inputs[input_idx] = Some(other_arg_idx); matched_inputs[other_arg_idx] = Some(input_idx);
matched_inputs[other_input_idx] = Some(arg_idx); matched_inputs[arg_idx] = Some(other_input_idx);
} }
Some(Issue::Permutation(args)) => { Some(Issue::Permutation(args)) => {
// FIXME: If satisfy_input ever did anything non-trivial (emit obligations to help type checking, for example)
// we'd want to call this function with the correct arg/input pairs, but for now, we just throw them in a bucket.
// This works because they force a cycle, so each row is guaranteed to also be a column
let mut idxs: Vec<usize> = args.iter().filter_map(|&a| a).collect(); let mut idxs: Vec<usize> = args.iter().filter_map(|&a| a).collect();
let mut real_idxs = vec![None; provided_arg_count]; let mut real_idxs = vec![None; provided_arg_count];
for (src, dst) in for (src, dst) in
args.iter().enumerate().filter_map(|(src, dst)| dst.map(|dst| (src, dst))) args.iter().enumerate().filter_map(|(src, dst)| dst.map(|dst| (src, dst)))
{ {
let src_arg = self.arg_indexes[src]; let src_input_idx = self.input_indexes[src];
let dst_arg = self.arg_indexes[dst]; let dst_input_idx = self.input_indexes[dst];
let dest_input = self.input_indexes[dst]; let dest_arg_idx = self.arg_indexes[dst];
real_idxs[src_arg] = Some((dst_arg, dest_input)); real_idxs[src_input_idx] = Some((dest_arg_idx, dst_input_idx));
matched_inputs[dest_input] = Some(src_arg); matched_inputs[dest_arg_idx] = Some(src_input_idx);
} }
idxs.sort(); idxs.sort();
idxs.reverse(); idxs.reverse();
@ -331,8 +339,8 @@ pub(crate) fn find_errors(mut self) -> (Vec<Error<'tcx>>, Vec<Option<usize>>) {
None => { None => {
// We didn't find any issues, so we need to push the algorithm forward // We didn't find any issues, so we need to push the algorithm forward
// First, eliminate any arguments that currently satisfy their inputs // First, eliminate any arguments that currently satisfy their inputs
for (arg, inp) in self.eliminate_satisfied() { for (inp, arg) in self.eliminate_satisfied() {
matched_inputs[inp] = Some(arg); matched_inputs[arg] = Some(inp);
} }
} }
}; };

81
compiler/rustc_typeck/src/check/fn_ctxt/checks.rs
View File

@ -274,9 +274,9 @@ pub(in super::super) fn check_argument_types(
// A "softer" version of the helper above, which checks types without persisting them, // A "softer" version of the helper above, which checks types without persisting them,
// and treats error types differently // and treats error types differently
// This will allow us to "probe" for other argument orders that would likely have been correct // This will allow us to "probe" for other argument orders that would likely have been correct
let check_compatible = |arg_idx, input_idx| { let check_compatible = |input_idx, arg_idx| {
let formal_input_ty: Ty<'tcx> = formal_input_tys[input_idx]; let formal_input_ty: Ty<'tcx> = formal_input_tys[arg_idx];
let expected_input_ty: Ty<'tcx> = expected_input_tys[input_idx]; let expected_input_ty: Ty<'tcx> = expected_input_tys[arg_idx];
// If either is an error type, we defy the usual convention and consider them to *not* be // If either is an error type, we defy the usual convention and consider them to *not* be
// coercible. This prevents our error message heuristic from trying to pass errors into // coercible. This prevents our error message heuristic from trying to pass errors into
@ -285,7 +285,7 @@ pub(in super::super) fn check_argument_types(
return Compatibility::Incompatible(None); return Compatibility::Incompatible(None);
} }
let provided_arg: &hir::Expr<'tcx> = &provided_args[arg_idx]; let provided_arg: &hir::Expr<'tcx> = &provided_args[input_idx];
let expectation = Expectation::rvalue_hint(self, expected_input_ty); let expectation = Expectation::rvalue_hint(self, expected_input_ty);
// FIXME: check that this is safe; I don't believe this commits any of the obligations, but I can't be sure. // FIXME: check that this is safe; I don't believe this commits any of the obligations, but I can't be sure.
// //
@ -429,11 +429,11 @@ pub(in super::super) fn check_argument_types(
let found_errors = !errors.is_empty(); let found_errors = !errors.is_empty();
errors.drain_filter(|error| { errors.drain_filter(|error| {
let Error::Invalid(input_idx, Compatibility::Incompatible(error)) = error else { return false }; let Error::Invalid(input_idx, arg_idx, Compatibility::Incompatible(error)) = error else { return false };
let expected_ty = expected_input_tys[*input_idx]; let expected_ty = expected_input_tys[*arg_idx];
let Some(Some((provided_ty, _))) = final_arg_types.get(*input_idx) else { return false }; let provided_ty = final_arg_types[*input_idx].map(|ty| ty.0).unwrap();
let cause = &self.misc(provided_args[*input_idx].span); let cause = &self.misc(provided_args[*input_idx].span);
let trace = TypeTrace::types(cause, true, expected_ty, *provided_ty); let trace = TypeTrace::types(cause, true, expected_ty, provided_ty);
if let Some(e) = error { if let Some(e) = error {
if !matches!(trace.cause.as_failure_code(e), FailureCode::Error0308(_)) { if !matches!(trace.cause.as_failure_code(e), FailureCode::Error0308(_)) {
self.report_and_explain_type_error(trace, e).emit(); self.report_and_explain_type_error(trace, e).emit();
@ -562,11 +562,14 @@ pub(in super::super) fn check_argument_types(
// Next special case: if there is only one "Incompatible" error, just emit that // Next special case: if there is only one "Incompatible" error, just emit that
if errors.len() == 1 { if errors.len() == 1 {
if let Some(Error::Invalid(input_idx, Compatibility::Incompatible(Some(error)))) = if let Some(Error::Invalid(
errors.iter().next() input_idx,
arg_idx,
Compatibility::Incompatible(Some(error)),
)) = errors.iter().next()
{ {
let expected_ty = expected_input_tys[*input_idx]; 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[*arg_idx].map(|ty| ty.0).unwrap();
let expected_ty = self.resolve_vars_if_possible(expected_ty); let expected_ty = self.resolve_vars_if_possible(expected_ty);
let provided_ty = self.resolve_vars_if_possible(provided_ty); let provided_ty = self.resolve_vars_if_possible(provided_ty);
let cause = &self.misc(provided_args[*input_idx].span); let cause = &self.misc(provided_args[*input_idx].span);
@ -631,19 +634,13 @@ enum SuggestionText {
let mut errors = errors.into_iter().peekable(); let mut errors = errors.into_iter().peekable();
while let Some(error) = errors.next() { while let Some(error) = errors.next() {
match error { match error {
Error::Invalid(input_idx, compatibility) => { Error::Invalid(input_idx, arg_idx, compatibility) => {
let expected_ty = expected_input_tys[input_idx]; let expected_ty = expected_input_tys[arg_idx];
let provided_ty = final_arg_types let provided_ty = final_arg_types[input_idx].map(|ty| ty.0).unwrap();
.get(input_idx)
.and_then(|x| x.as_ref())
.map(|ty| ty.0)
.unwrap_or(tcx.ty_error());
let expected_ty = self.resolve_vars_if_possible(expected_ty); let expected_ty = self.resolve_vars_if_possible(expected_ty);
let provided_ty = self.resolve_vars_if_possible(provided_ty); let provided_ty = self.resolve_vars_if_possible(provided_ty);
if let Compatibility::Incompatible(error) = &compatibility { if let Compatibility::Incompatible(error) = &compatibility {
let cause = &self.misc( let cause = &self.misc(provided_args[input_idx].span);
provided_args.get(input_idx).map(|i| i.span).unwrap_or(call_span),
);
let trace = TypeTrace::types(cause, true, expected_ty, provided_ty); let trace = TypeTrace::types(cause, true, expected_ty, provided_ty);
if let Some(e) = error { if let Some(e) = error {
self.note_type_err( self.note_type_err(
@ -658,16 +655,14 @@ enum SuggestionText {
} }
} }
if let Some(expr) = provided_args.get(input_idx) { self.emit_coerce_suggestions(
self.emit_coerce_suggestions( &mut err,
&mut err, &provided_args[input_idx],
&expr, final_arg_types[input_idx].map(|ty| ty.0).unwrap(),
final_arg_types[input_idx].map(|ty| ty.0).unwrap(), final_arg_types[input_idx].map(|ty| ty.1).unwrap(),
final_arg_types[input_idx].map(|ty| ty.1).unwrap(), None,
None, None,
None, );
);
}
} }
Error::Extra(arg_idx) => { Error::Extra(arg_idx) => {
let arg_type = if let Some((_, ty)) = final_arg_types[arg_idx] { let arg_type = if let Some((_, ty)) = final_arg_types[arg_idx] {
@ -843,12 +838,12 @@ enum SuggestionText {
} }
} }
Error::Swap(input_idx, other_input_idx, arg_idx, other_arg_idx) => { Error::Swap(input_idx, other_input_idx, arg_idx, other_arg_idx) => {
let first_span = provided_args[arg_idx].span; let first_span = provided_args[input_idx].span;
let second_span = provided_args[other_arg_idx].span; let second_span = provided_args[other_input_idx].span;
let first_expected_ty = let first_expected_ty =
self.resolve_vars_if_possible(expected_input_tys[input_idx]); self.resolve_vars_if_possible(expected_input_tys[arg_idx]);
let first_provided_ty = if let Some((ty, _)) = final_arg_types[arg_idx] { let first_provided_ty = if let Some((ty, _)) = final_arg_types[input_idx] {
format!(",found `{}`", ty) format!(",found `{}`", ty)
} else { } else {
"".into() "".into()
@ -858,9 +853,9 @@ enum SuggestionText {
format!("expected `{}`{}", first_expected_ty, first_provided_ty), format!("expected `{}`{}", first_expected_ty, first_provided_ty),
)); ));
let other_expected_ty = let other_expected_ty =
self.resolve_vars_if_possible(expected_input_tys[other_input_idx]); self.resolve_vars_if_possible(expected_input_tys[other_arg_idx]);
let other_provided_ty = let other_provided_ty =
if let Some((ty, _)) = final_arg_types[other_arg_idx] { if let Some((ty, _)) = final_arg_types[other_input_idx] {
format!(",found `{}`", ty) format!(",found `{}`", ty)
} else { } else {
"".into() "".into()
@ -926,14 +921,14 @@ enum SuggestionText {
"{}(", "{}(",
source_map.span_to_snippet(full_call_span).unwrap_or_else(|_| String::new()) source_map.span_to_snippet(full_call_span).unwrap_or_else(|_| String::new())
); );
for (idx, arg) in matched_inputs.iter().enumerate() { for (arg_index, input_idx) in matched_inputs.iter().enumerate() {
let suggestion_text = if let Some(arg) = arg { let suggestion_text = if let Some(input_idx) = input_idx {
let arg_span = provided_args[*arg].span.source_callsite(); let arg_span = provided_args[*input_idx].span.source_callsite();
let arg_text = source_map.span_to_snippet(arg_span).unwrap(); let arg_text = source_map.span_to_snippet(arg_span).unwrap();
arg_text arg_text
} else { } else {
// Propose a placeholder of the correct type // Propose a placeholder of the correct type
let expected_ty = expected_input_tys[idx]; let expected_ty = expected_input_tys[arg_index];
let input_ty = self.resolve_vars_if_possible(expected_ty); let input_ty = self.resolve_vars_if_possible(expected_ty);
if input_ty.is_unit() { if input_ty.is_unit() {
"()".to_string() "()".to_string()
@ -942,7 +937,7 @@ enum SuggestionText {
} }
}; };
suggestion += &suggestion_text; suggestion += &suggestion_text;
if idx < minimum_input_count - 1 { if arg_index < minimum_input_count - 1 {
suggestion += ", "; suggestion += ", ";
} }
} }