Auto merge of #92364 - jackh726:Quantumplation/65853/param-heuristics, r=estebank

Better method call error messages

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

compiler/rustc_typeck/src/check/coercion.rs

@@ -1499,7 +1499,7 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
                         found,
                         expected,
                         None,
-                        coercion_error,
+                        Some(coercion_error),
                     );
                 }
 

compiler/rustc_typeck/src/check/compare_method.rs

@@ -384,6 +384,7 @@ fn compare_predicate_entailment<'tcx>(
                 })),
                 &terr,
                 false,
+                false,
             );
 
             return Err(diag.emit());
@@ -1072,6 +1073,7 @@ crate fn compare_const_impl<'tcx>(
                 })),
                 &terr,
                 false,
+                false,
             );
             diag.emit();
         }

compiler/rustc_typeck/src/check/demand.rs

@@ -28,7 +28,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         expr_ty: Ty<'tcx>,
         expected: Ty<'tcx>,
         expected_ty_expr: Option<&'tcx hir::Expr<'tcx>>,
-        error: TypeError<'tcx>,
+        error: Option<TypeError<'tcx>>,
     ) {
         self.annotate_expected_due_to_let_ty(err, expr, error);
         self.suggest_deref_ref_or_into(err, expr, expected, expr_ty, expected_ty_expr);
@@ -150,7 +150,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         let expr_ty = self.resolve_vars_with_obligations(checked_ty);
         let mut err = self.report_mismatched_types(&cause, expected, expr_ty, e.clone());
 
-        self.emit_coerce_suggestions(&mut err, expr, expr_ty, expected, expected_ty_expr, e);
+        self.emit_coerce_suggestions(&mut err, expr, expr_ty, expected, expected_ty_expr, Some(e));
 
         (expected, Some(err))
     }
@@ -159,7 +159,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         &self,
         err: &mut Diagnostic,
         expr: &hir::Expr<'_>,
-        error: TypeError<'_>,
+        error: Option<TypeError<'_>>,
     ) {
         let parent = self.tcx.hir().get_parent_node(expr.hir_id);
         match (self.tcx.hir().find(parent), error) {
@@ -173,7 +173,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 Some(hir::Node::Expr(hir::Expr {
                     kind: hir::ExprKind::Assign(lhs, rhs, _), ..
                 })),
-                TypeError::Sorts(ExpectedFound { expected, .. }),
+                Some(TypeError::Sorts(ExpectedFound { expected, .. })),
             ) if rhs.hir_id == expr.hir_id && !expected.is_closure() => {
                 // We ignore closures explicitly because we already point at them elsewhere.
                 // Point at the assigned-to binding.

compiler/rustc_typeck/src/check/expr.rs

@@ -259,7 +259,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     }
 
     #[instrument(skip(self, expr), level = "debug")]
-    fn check_expr_kind(
+    pub(super) fn check_expr_kind(
         &self,
         expr: &'tcx hir::Expr<'tcx>,
         expected: Expectation<'tcx>,
@@ -1366,11 +1366,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     ) {
         let tcx = self.tcx;
 
-        let adt_ty_hint = self
-            .expected_inputs_for_expected_output(span, expected, adt_ty, &[adt_ty])
-            .get(0)
-            .cloned()
-            .unwrap_or(adt_ty);
+        let expected_inputs =
+            self.expected_inputs_for_expected_output(span, expected, adt_ty, &[adt_ty]);
+        let adt_ty_hint = if let Some(expected_inputs) = expected_inputs {
+            expected_inputs.get(0).cloned().unwrap_or(adt_ty)
+        } else {
+            adt_ty
+        };
         // re-link the regions that EIfEO can erase.
         self.demand_eqtype(span, adt_ty_hint, adt_ty);
 

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

@@ -755,9 +755,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         expected_ret: Expectation<'tcx>,
         formal_ret: Ty<'tcx>,
         formal_args: &[Ty<'tcx>],
-    ) -> Vec<Ty<'tcx>> {
+    ) -> Option<Vec<Ty<'tcx>>> {
         let formal_ret = self.resolve_vars_with_obligations(formal_ret);
-        let Some(ret_ty) = expected_ret.only_has_type(self) else { return Vec::new() };
+        let Some(ret_ty) = expected_ret.only_has_type(self) else { return None };
 
         // HACK(oli-obk): This is a hack to keep RPIT and TAIT in sync wrt their behaviour.
         // Without it, the inference
@@ -779,7 +779,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 if let ty::subst::GenericArgKind::Type(ty) = ty.unpack() {
                     if let ty::Opaque(def_id, _) = *ty.kind() {
                         if self.infcx.opaque_type_origin(def_id, DUMMY_SP).is_some() {
-                            return Vec::new();
+                            return None;
                         }
                     }
                 }
@@ -820,7 +820,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
 
                 // Record all the argument types, with the substitutions
                 // produced from the above subtyping unification.
-                Ok(formal_args.iter().map(|&ty| self.resolve_vars_if_possible(ty)).collect())
+                Ok(Some(formal_args.iter().map(|&ty| self.resolve_vars_if_possible(ty)).collect()))
             })
             .unwrap_or_default();
         debug!(?formal_args, ?formal_ret, ?expect_args, ?expected_ret);

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

@@ -0,0 +1,343 @@
+use std::cmp;
+
+use rustc_middle::ty::error::TypeError;
+
+// An issue that might be found in the compatibility matrix
+enum Issue {
+    /// The given argument is the invalid type for the input
+    Invalid(usize),
+    /// There is a missing input
+    Missing(usize),
+    /// There's a superfluous argument
+    Extra(usize),
+    /// Two arguments should be swapped
+    Swap(usize, usize),
+    /// Several arguments should be reordered
+    Permutation(Vec<Option<usize>>),
+}
+
+#[derive(Clone, Debug)]
+pub(crate) enum Compatibility<'tcx> {
+    Compatible,
+    Incompatible(Option<TypeError<'tcx>>),
+}
+
+/// Similar to `Issue`, but contains some extra information
+pub(crate) enum Error<'tcx> {
+    /// The given argument is the invalid type for the input
+    Invalid(usize, Compatibility<'tcx>),
+    /// There is a missing input
+    Missing(usize),
+    /// There's a superfluous argument
+    Extra(usize),
+    /// Two arguments should be swapped
+    Swap(usize, usize, usize, usize),
+    /// Several arguments should be reordered
+    Permutation(Vec<(usize, usize)>), // dest_arg, dest_input
+}
+
+pub(crate) struct ArgMatrix<'tcx> {
+    input_indexes: Vec<usize>,
+    arg_indexes: Vec<usize>,
+    compatibility_matrix: Vec<Vec<Compatibility<'tcx>>>,
+}
+
+impl<'tcx> ArgMatrix<'tcx> {
+    pub(crate) fn new<F: FnMut(usize, usize) -> Compatibility<'tcx>>(
+        minimum_input_count: usize,
+        provided_arg_count: usize,
+        mut is_compatible: F,
+    ) -> Self {
+        let compatibility_matrix = (0..provided_arg_count)
+            .map(|i| (0..minimum_input_count).map(|j| is_compatible(i, j)).collect())
+            .collect();
+        ArgMatrix {
+            input_indexes: (0..minimum_input_count).collect(),
+            arg_indexes: (0..provided_arg_count).collect(),
+            compatibility_matrix,
+        }
+    }
+
+    /// Remove a given input from consideration
+    fn eliminate_input(&mut self, idx: usize) {
+        self.input_indexes.remove(idx);
+        for row in &mut self.compatibility_matrix {
+            row.remove(idx);
+        }
+    }
+
+    /// Remove a given argument from consideration
+    fn eliminate_arg(&mut self, idx: usize) {
+        self.arg_indexes.remove(idx);
+        self.compatibility_matrix.remove(idx);
+    }
+
+    /// "satisfy" an input with a given arg, removing both from consideration
+    fn satisfy_input(&mut self, input_idx: usize, arg_idx: usize) {
+        self.eliminate_input(input_idx);
+        self.eliminate_arg(arg_idx);
+    }
+
+    fn eliminate_satisfied(&mut self) -> Vec<(usize, usize)> {
+        let mut i = cmp::min(self.input_indexes.len(), self.arg_indexes.len());
+        let mut eliminated = vec![];
+        while i > 0 {
+            let idx = i - 1;
+            if matches!(self.compatibility_matrix[idx][idx], Compatibility::Compatible) {
+                eliminated.push((self.arg_indexes[idx], self.input_indexes[idx]));
+                self.satisfy_input(idx, idx);
+            }
+            i -= 1;
+        }
+        return eliminated;
+    }
+
+    // Check for the above mismatch cases
+    fn find_issue(&self) -> Option<Issue> {
+        let mat = &self.compatibility_matrix;
+        let ai = &self.arg_indexes;
+        let ii = &self.input_indexes;
+
+        for i in 0..cmp::max(ai.len(), ii.len()) {
+            // If we eliminate the last row, any left-over inputs are considered missing
+            if i >= mat.len() {
+                return Some(Issue::Missing(i));
+            }
+            // If we eliminate the last column, any left-over arguments are extra
+            if mat[i].len() == 0 {
+                return Some(Issue::Extra(i));
+            }
+
+            // Make sure we don't pass the bounds of our matrix
+            let is_arg = i < ai.len();
+            let is_input = i < ii.len();
+            if is_arg && is_input && matches!(mat[i][i], Compatibility::Compatible) {
+                // This is a satisfied input, so move along
+                continue;
+            }
+
+            let mut useless = true;
+            let mut unsatisfiable = true;
+            if is_arg {
+                for j in 0..ii.len() {
+                    // 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
+                    if matches!(mat[j][i], Compatibility::Compatible) {
+                        unsatisfiable = false;
+                        break;
+                    }
+                }
+            }
+
+            match (is_arg, is_input, useless, unsatisfiable) {
+                // If an input is unsatisfied, and the argument 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
+                (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)),
+                (true, true, _, _) => {
+                    // The argument isn't useless, and the input isn't unsatisfied,
+                    // so look for a parameter we might swap it with
+                    // We look for swaps explicitly, instead of just falling back on permutations
+                    // so that cases like (A,B,C,D) given (B,A,D,C) show up as two swaps,
+                    // instead of a large permutation of 4 elements.
+                    for j in 0..cmp::min(ai.len(), ii.len()) {
+                        if i == j || matches!(mat[j][j], Compatibility::Compatible) {
+                            continue;
+                        }
+                        if matches!(mat[i][j], Compatibility::Compatible)
+                            && matches!(mat[j][i], Compatibility::Compatible)
+                        {
+                            return Some(Issue::Swap(i, j));
+                        }
+                    }
+                }
+                _ => {
+                    continue;
+                }
+            };
+        }
+
+        // We didn't find any of the individual issues above, but
+        // there might be a larger permutation of parameters, so we now check for that
+        // by checking for cycles
+        // We use a double option at position i in this vec to represent:
+        // - None: We haven't computed anything about this argument yet
+        // - Some(None): This argument definitely doesn't participate in a cycle
+        // - Some(Some(x)): the i-th argument could permute to the x-th position
+        let mut permutation: Vec<Option<Option<usize>>> = vec![None; mat.len()];
+        let mut permutation_found = false;
+        for i in 0..mat.len() {
+            if permutation[i].is_some() {
+                // We've already decided whether this argument is or is not in a loop
+                continue;
+            }
+
+            let mut stack = vec![];
+            let mut j = i;
+            let mut last = i;
+            let mut is_cycle = true;
+            loop {
+                stack.push(j);
+                // Look for params this one could slot into
+                let compat: Vec<_> =
+                    mat[j]
+                        .iter()
+                        .enumerate()
+                        .filter_map(|(i, c)| {
+                            if matches!(c, Compatibility::Compatible) { Some(i) } else { None }
+                        })
+                        .collect();
+                if compat.len() != 1 {
+                    // this could go into multiple slots, don't bother exploring both
+                    is_cycle = false;
+                    break;
+                }
+                j = compat[0];
+                if stack.contains(&j) {
+                    last = j;
+                    break;
+                }
+            }
+            if stack.len() <= 2 {
+                // If we encounter a cycle of 1 or 2 elements, we'll let the
+                // "satisfy" and "swap" code above handle those
+                is_cycle = false;
+            }
+            // We've built up some chain, some of which might be a cycle
+            // ex: [1,2,3,4]; last = 2; j = 2;
+            // So, we want to mark 4, 3, and 2 as part of a permutation
+            permutation_found = is_cycle;
+            while let Some(x) = stack.pop() {
+                if is_cycle {
+                    permutation[x] = Some(Some(j));
+                    j = x;
+                    if j == last {
+                        // From here on out, we're a tail leading into a cycle,
+                        // not the cycle itself
+                        is_cycle = false;
+                    }
+                } else {
+                    // Some(None) ensures we save time by skipping this argument again
+                    permutation[x] = Some(None);
+                }
+            }
+        }
+
+        if permutation_found {
+            // Map unwrap to remove the first layer of Some
+            let final_permutation: Vec<Option<usize>> =
+                permutation.into_iter().map(|x| x.unwrap()).collect();
+            return Some(Issue::Permutation(final_permutation));
+        }
+        return None;
+    }
+
+    // Obviously, detecting exact user intention is impossible, so the goal here is to
+    // come up with as likely of a story as we can to be helpful.
+    //
+    // We'll iteratively removed "satisfied" input/argument pairs,
+    // then check for the cases above, until we've eliminated the entire grid
+    //
+    // We'll want to know which arguments and inputs these rows and columns correspond to
+    // even after we delete them.
+    pub(crate) fn find_errors(mut self) -> (Vec<Error<'tcx>>, Vec<Option<usize>>) {
+        let provided_arg_count = self.arg_indexes.len();
+
+        let mut errors: Vec<Error<'tcx>> = vec![];
+        // For each expected argument, the matched *actual* input
+        let mut matched_inputs: Vec<Option<usize>> = vec![None; self.input_indexes.len()];
+
+        // 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
+        // spill over into another similarly typed input
+        // ex:
+        //   fn some_func(_a: i32, _b: i32) {}
+        //   some_func(1, "");
+        // Without this elimination, the first argument causes the second argument
+        // to show up as both a missing input and extra argument, rather than
+        // just an invalid type.
+        for (arg, inp) in self.eliminate_satisfied() {
+            matched_inputs[inp] = Some(arg);
+        }
+
+        while self.input_indexes.len() > 0 || self.arg_indexes.len() > 0 {
+            // Check for the first relevant issue
+            match self.find_issue() {
+                Some(Issue::Invalid(idx)) => {
+                    let compatibility = self.compatibility_matrix[idx][idx].clone();
+                    let input_idx = self.input_indexes[idx];
+                    self.satisfy_input(idx, idx);
+                    errors.push(Error::Invalid(input_idx, compatibility));
+                }
+                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];
+                    self.eliminate_input(idx);
+                    errors.push(Error::Missing(input_idx));
+                }
+                Some(Issue::Swap(idx, other)) => {
+                    let input_idx = self.input_indexes[idx];
+                    let other_input_idx = self.input_indexes[other];
+                    let arg_idx = self.arg_indexes[idx];
+                    let other_arg_idx = self.arg_indexes[other];
+                    let (min, max) = (cmp::min(idx, other), cmp::max(idx, other));
+                    self.satisfy_input(min, max);
+                    // Subtract 1 because we already removed the "min" row
+                    self.satisfy_input(max - 1, min);
+                    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_input_idx] = Some(arg_idx);
+                }
+                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 real_idxs = vec![None; provided_arg_count];
+                    for (src, dst) in
+                        args.iter().enumerate().filter_map(|(src, dst)| dst.map(|dst| (src, dst)))
+                    {
+                        let src_arg = self.arg_indexes[src];
+                        let dst_arg = self.arg_indexes[dst];
+                        let dest_input = self.input_indexes[dst];
+                        real_idxs[src_arg] = Some((dst_arg, dest_input));
+                        matched_inputs[dest_input] = Some(src_arg);
+                    }
+                    idxs.sort();
+                    idxs.reverse();
+                    for i in idxs {
+                        self.satisfy_input(i, i);
+                    }
+                    errors.push(Error::Permutation(real_idxs.into_iter().flatten().collect()));
+                }
+                None => {
+                    // We didn't find any issues, so we need to push the algorithm forward
+                    // First, eliminate any arguments that currently satisfy their inputs
+                    for (arg, inp) in self.eliminate_satisfied() {
+                        matched_inputs[inp] = Some(arg);
+                    }
+                }
+            };
+        }
+
+        return (errors, matched_inputs);
+    }
+}

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

@@ -1,9 +1,9 @@
 mod _impl;
+mod arg_matrix;
 mod checks;
 mod suggestions;
 
 pub use _impl::*;
-pub use checks::*;
 pub use suggestions::*;
 
 use crate::astconv::AstConv;

compiler/rustc_typeck/src/lib.rs

@@ -61,9 +61,11 @@ This API is completely unstable and subject to change.
 #![feature(box_patterns)]
 #![feature(control_flow_enum)]
 #![feature(crate_visibility_modifier)]
+#![feature(drain_filter)]
 #![feature(hash_drain_filter)]
 #![feature(if_let_guard)]
 #![feature(is_sorted)]
+#![feature(label_break_value)]
 #![feature(let_chains)]
 #![feature(let_else)]
 #![feature(min_specialization)]