From 57de75050a9e2aeddfe6a9f4c47776809167b0f7 Mon Sep 17 00:00:00 2001 From: Jesse Rusak Date: Mon, 19 Aug 2024 13:10:26 -0400 Subject: [PATCH] When calling a method on Fn* traits explicitly, argument diagnostics should point at the called method (eg Fn::call_once), not the underlying callee. Fixes 128848 --- compiler/rustc_hir_typeck/src/callee.rs | 2 +- .../rustc_hir_typeck/src/fn_ctxt/checks.rs | 413 +++++++++--------- tests/crashes/128848.rs | 5 - .../mismatch-args-crash-issue-128848.rs | 10 + .../mismatch-args-crash-issue-128848.stderr | 16 + 5 files changed, 245 insertions(+), 201 deletions(-) delete mode 100644 tests/crashes/128848.rs create mode 100644 tests/ui/mismatched_types/mismatch-args-crash-issue-128848.rs create mode 100644 tests/ui/mismatched_types/mismatch-args-crash-issue-128848.stderr diff --git a/compiler/rustc_hir_typeck/src/callee.rs b/compiler/rustc_hir_typeck/src/callee.rs index 9863d0364498e..3dc8759a9edee 100644 --- a/compiler/rustc_hir_typeck/src/callee.rs +++ b/compiler/rustc_hir_typeck/src/callee.rs @@ -58,7 +58,7 @@ pub(crate) fn check_legal_trait_for_method_call( enum CallStep<'tcx> { Builtin(Ty<'tcx>), DeferredClosure(LocalDefId, ty::FnSig<'tcx>), - /// E.g., enum variant constructors. + /// Call overloading when callee implements one of the Fn* traits. Overloaded(MethodCallee<'tcx>), } diff --git a/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs b/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs index 21b040bd0bcb3..8348c6e9a169e 100644 --- a/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs +++ b/compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs @@ -506,6 +506,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { fn_def_id, call_span, call_expr, + tuple_arguments, ); } } @@ -520,6 +521,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { fn_def_id: Option, call_span: Span, call_expr: &'tcx hir::Expr<'tcx>, + tuple_arguments: TupleArgumentsFlag, ) -> ErrorGuaranteed { // Next, let's construct the error let (error_span, call_ident, full_call_span, call_name, is_method) = match &call_expr.kind { @@ -865,6 +867,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { &matched_inputs, &formal_and_expected_inputs, is_method, + tuple_arguments, ); suggest_confusable(&mut err); return err.emit(); @@ -1001,6 +1004,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { &matched_inputs, &formal_and_expected_inputs, is_method, + tuple_arguments, ); suggest_confusable(&mut err); return err.emit(); @@ -1448,6 +1452,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { &matched_inputs, &formal_and_expected_inputs, is_method, + tuple_arguments, ); // And add a suggestion block for all of the parameters @@ -2219,21 +2224,27 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { matched_inputs: &IndexVec>, formal_and_expected_inputs: &IndexVec, Ty<'tcx>)>, is_method: bool, + tuple_arguments: TupleArgumentsFlag, ) { let Some(mut def_id) = callable_def_id else { return; }; + // If we're calling a method of a Fn/FnMut/FnOnce trait object implicitly + // (eg invoking a closure) we want to point at the underlying callable, + // not the method implicitly invoked (eg call_once). if let Some(assoc_item) = self.tcx.opt_associated_item(def_id) - // Possibly points at either impl or trait item, so try to get it - // to point to trait item, then get the parent. - // This parent might be an impl in the case of an inherent function, - // but the next check will fail. + // Since this is an associated item, it might point at either an impl or a trait item. + // We want it to always point to the trait item. + // If we're pointing at an inherent function, we don't need to do anything, + // so we fetch the parent and verify if it's a trait item. && let maybe_trait_item_def_id = assoc_item.trait_item_def_id.unwrap_or(def_id) && let maybe_trait_def_id = self.tcx.parent(maybe_trait_item_def_id) // Just an easy way to check "trait_def_id == Fn/FnMut/FnOnce" && let Some(call_kind) = self.tcx.fn_trait_kind_from_def_id(maybe_trait_def_id) && let Some(callee_ty) = callee_ty + // TupleArguments is set only when this is an implicit call (my_closure(...)) rather than explicit (my_closure.call(...)) + && tuple_arguments == TupleArguments { let callee_ty = callee_ty.peel_refs(); match *callee_ty.kind() { @@ -2303,166 +2314,173 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { { let mut spans: MultiSpan = def_span.into(); - let params_with_generics = self.get_hir_params_with_generics(def_id, is_method); - let mut generics_with_unmatched_params = Vec::new(); + if let Some(params_with_generics) = self.get_hir_params_with_generics(def_id, is_method) + { + debug_assert_eq!(params_with_generics.len(), matched_inputs.len()); - let check_for_matched_generics = || { - if matched_inputs.iter().any(|x| x.is_some()) - && params_with_generics.iter().any(|x| x.0.is_some()) - { - for (idx, (generic, _)) in params_with_generics.iter().enumerate() { - // Param has to have a generic and be matched to be relevant - if matched_inputs[idx.into()].is_none() { - continue; - } + let mut generics_with_unmatched_params = Vec::new(); - let Some(generic) = generic else { - continue; - }; + let check_for_matched_generics = || { + if matched_inputs.iter().any(|x| x.is_some()) + && params_with_generics.iter().any(|x| x.0.is_some()) + { + for (idx, (generic, _)) in params_with_generics.iter().enumerate() { + // Param has to have a generic and be matched to be relevant + if matched_inputs[idx.into()].is_none() { + continue; + } - for unmatching_idx in idx + 1..params_with_generics.len() { - if matched_inputs[unmatching_idx.into()].is_none() - && let Some(unmatched_idx_param_generic) = - params_with_generics[unmatching_idx].0 - && unmatched_idx_param_generic.name.ident() == generic.name.ident() - { - // We found a parameter that didn't match that needed to - return true; + let Some(generic) = generic else { + continue; + }; + + for unmatching_idx in idx + 1..params_with_generics.len() { + if matched_inputs[unmatching_idx.into()].is_none() + && let Some(unmatched_idx_param_generic) = + params_with_generics[unmatching_idx].0 + && unmatched_idx_param_generic.name.ident() + == generic.name.ident() + { + // We found a parameter that didn't match that needed to + return true; + } } } } - } - false - }; - - let check_for_matched_generics = check_for_matched_generics(); - - for (idx, (generic_param, param)) in - params_with_generics.iter().enumerate().filter(|(idx, _)| { - check_for_matched_generics - || expected_idx.is_none_or(|expected_idx| expected_idx == *idx) - }) - { - let Some(generic_param) = generic_param else { - spans.push_span_label(param.span, ""); - continue; + false }; - let other_params_matched: Vec<(usize, &hir::Param<'_>)> = params_with_generics - .iter() - .enumerate() - .filter(|(other_idx, (other_generic_param, _))| { - if *other_idx == idx { - return false; - } - let Some(other_generic_param) = other_generic_param else { - return false; - }; - if matched_inputs[idx.into()].is_none() - && matched_inputs[(*other_idx).into()].is_none() - { - return false; - } - if matched_inputs[idx.into()].is_some() - && matched_inputs[(*other_idx).into()].is_some() - { - return false; - } - other_generic_param.name.ident() == generic_param.name.ident() + let check_for_matched_generics = check_for_matched_generics(); + + for (idx, (generic_param, param)) in + params_with_generics.iter().enumerate().filter(|(idx, _)| { + check_for_matched_generics + || expected_idx.is_none_or(|expected_idx| expected_idx == *idx) }) - .map(|(other_idx, (_, other_param))| (other_idx, *other_param)) - .collect(); + { + let Some(generic_param) = generic_param else { + spans.push_span_label(param.span, ""); + continue; + }; - if !other_params_matched.is_empty() { - let other_param_matched_names: Vec = other_params_matched + let other_params_matched: Vec<(usize, &hir::Param<'_>)> = params_with_generics .iter() - .map(|(_, other_param)| { - if let hir::PatKind::Binding(_, _, ident, _) = other_param.pat.kind { - format!("`{ident}`") - } else { - "{unknown}".to_string() + .enumerate() + .filter(|(other_idx, (other_generic_param, _))| { + if *other_idx == idx { + return false; + } + let Some(other_generic_param) = other_generic_param else { + return false; + }; + if matched_inputs[idx.into()].is_none() + && matched_inputs[(*other_idx).into()].is_none() + { + return false; } + if matched_inputs[idx.into()].is_some() + && matched_inputs[(*other_idx).into()].is_some() + { + return false; + } + other_generic_param.name.ident() == generic_param.name.ident() }) + .map(|(other_idx, (_, other_param))| (other_idx, *other_param)) .collect(); - let matched_ty = self - .resolve_vars_if_possible(formal_and_expected_inputs[idx.into()].1) - .sort_string(self.tcx); + if !other_params_matched.is_empty() { + let other_param_matched_names: Vec = other_params_matched + .iter() + .map(|(_, other_param)| { + if let hir::PatKind::Binding(_, _, ident, _) = other_param.pat.kind + { + format!("`{ident}`") + } else { + "{unknown}".to_string() + } + }) + .collect(); - if matched_inputs[idx.into()].is_some() { - spans.push_span_label( - param.span, - format!( - "{} {} to match the {} type of this parameter", - display_list_with_comma_and(&other_param_matched_names), + let matched_ty = self + .resolve_vars_if_possible(formal_and_expected_inputs[idx.into()].1) + .sort_string(self.tcx); + + if matched_inputs[idx.into()].is_some() { + spans.push_span_label( + param.span, format!( - "need{}", - pluralize!(if other_param_matched_names.len() == 1 { - 0 - } else { - 1 - }) + "{} {} to match the {} type of this parameter", + display_list_with_comma_and(&other_param_matched_names), + format!( + "need{}", + pluralize!(if other_param_matched_names.len() == 1 { + 0 + } else { + 1 + }) + ), + matched_ty, ), - matched_ty, - ), - ); + ); + } else { + spans.push_span_label( + param.span, + format!( + "this parameter needs to match the {} type of {}", + matched_ty, + display_list_with_comma_and(&other_param_matched_names), + ), + ); + } + generics_with_unmatched_params.push(generic_param); } else { + spans.push_span_label(param.span, ""); + } + } + + for generic_param in self + .tcx + .hir() + .get_if_local(def_id) + .and_then(|node| node.generics()) + .into_iter() + .flat_map(|x| x.params) + .filter(|x| { + generics_with_unmatched_params + .iter() + .any(|y| x.name.ident() == y.name.ident()) + }) + { + let param_idents_matching: Vec = params_with_generics + .iter() + .filter(|(generic, _)| { + if let Some(generic) = generic { + generic.name.ident() == generic_param.name.ident() + } else { + false + } + }) + .map(|(_, param)| { + if let hir::PatKind::Binding(_, _, ident, _) = param.pat.kind { + format!("`{ident}`") + } else { + "{unknown}".to_string() + } + }) + .collect(); + + if !param_idents_matching.is_empty() { spans.push_span_label( - param.span, + generic_param.span, format!( - "this parameter needs to match the {} type of {}", - matched_ty, - display_list_with_comma_and(&other_param_matched_names), + "{} all reference this parameter {}", + display_list_with_comma_and(¶m_idents_matching), + generic_param.name.ident().name, ), ); } - generics_with_unmatched_params.push(generic_param); - } else { - spans.push_span_label(param.span, ""); } } - - for generic_param in self - .tcx - .hir() - .get_if_local(def_id) - .and_then(|node| node.generics()) - .into_iter() - .flat_map(|x| x.params) - .filter(|x| { - generics_with_unmatched_params.iter().any(|y| x.name.ident() == y.name.ident()) - }) - { - let param_idents_matching: Vec = params_with_generics - .iter() - .filter(|(generic, _)| { - if let Some(generic) = generic { - generic.name.ident() == generic_param.name.ident() - } else { - false - } - }) - .map(|(_, param)| { - if let hir::PatKind::Binding(_, _, ident, _) = param.pat.kind { - format!("`{ident}`") - } else { - "{unknown}".to_string() - } - }) - .collect(); - - if !param_idents_matching.is_empty() { - spans.push_span_label( - generic_param.span, - format!( - "{} all reference this parameter {}", - display_list_with_comma_and(¶m_idents_matching), - generic_param.name.ident().name, - ), - ); - } - } - err.span_note(spans, format!("{} defined here", self.tcx.def_descr(def_id))); } else if let Some(hir::Node::Expr(e)) = self.tcx.hir().get_if_local(def_id) && let hir::ExprKind::Closure(hir::Closure { body, .. }) = &e.kind @@ -2535,74 +2553,77 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { return; }; - let params_with_generics = self.get_hir_params_with_generics(def_id, is_method); - - for (idx, (generic_param, _)) in params_with_generics.iter().enumerate() { - if matched_inputs[idx.into()].is_none() { - continue; - } + if let Some(params_with_generics) = self.get_hir_params_with_generics(def_id, is_method) { + debug_assert_eq!(params_with_generics.len(), matched_inputs.len()); + for (idx, (generic_param, _)) in params_with_generics.iter().enumerate() { + if matched_inputs[idx.into()].is_none() { + continue; + } - let Some((_, matched_arg_span)) = provided_arg_tys.get(idx.into()) else { - continue; - }; + let Some((_, matched_arg_span)) = provided_arg_tys.get(idx.into()) else { + continue; + }; - let Some(generic_param) = generic_param else { - continue; - }; + let Some(generic_param) = generic_param else { + continue; + }; - let mut idxs_matched: Vec = vec![]; - for (other_idx, (_, _)) in params_with_generics.iter().enumerate().filter( - |(other_idx, (other_generic_param, _))| { - if *other_idx == idx { - return false; - } - let Some(other_generic_param) = other_generic_param else { - return false; - }; - if matched_inputs[(*other_idx).into()].is_some() { - return false; - } - other_generic_param.name.ident() == generic_param.name.ident() - }, - ) { - idxs_matched.push(other_idx); - } + let mut idxs_matched: Vec = vec![]; + for (other_idx, (_, _)) in params_with_generics.iter().enumerate().filter( + |(other_idx, (other_generic_param, _))| { + if *other_idx == idx { + return false; + } + let Some(other_generic_param) = other_generic_param else { + return false; + }; + if matched_inputs[(*other_idx).into()].is_some() { + return false; + } + other_generic_param.name.ident() == generic_param.name.ident() + }, + ) { + idxs_matched.push(other_idx); + } - if idxs_matched.is_empty() { - continue; - } + if idxs_matched.is_empty() { + continue; + } - let expected_display_type = self - .resolve_vars_if_possible(formal_and_expected_inputs[idx.into()].1) - .sort_string(self.tcx); - let label = if idxs_matched.len() == params_with_generics.len() - 1 { - format!( - "expected all arguments to be this {} type because they need to match the type of this parameter", - expected_display_type - ) - } else { - format!( - "expected some other arguments to be {} {} type to match the type of this parameter", - a_or_an(&expected_display_type), - expected_display_type, - ) - }; + let expected_display_type = self + .resolve_vars_if_possible(formal_and_expected_inputs[idx.into()].1) + .sort_string(self.tcx); + let label = if idxs_matched.len() == params_with_generics.len() - 1 { + format!( + "expected all arguments to be this {} type because they need to match the type of this parameter", + expected_display_type + ) + } else { + format!( + "expected some other arguments to be {} {} type to match the type of this parameter", + a_or_an(&expected_display_type), + expected_display_type, + ) + }; - err.span_label(*matched_arg_span, label); + err.span_label(*matched_arg_span, label); + } } } + /// Returns the parameters of a function, with their generic parameters if those are the full + /// type of that parameter. Returns `None` if the function body is unavailable (eg is an instrinsic). fn get_hir_params_with_generics( &self, def_id: DefId, is_method: bool, - ) -> Vec<(Option<&hir::GenericParam<'_>>, &hir::Param<'_>)> { - let fn_node = self.tcx.hir().get_if_local(def_id); + ) -> Option>, &hir::Param<'_>)>> { + let fn_node = self.tcx.hir().get_if_local(def_id)?; let generic_params: Vec>> = fn_node - .and_then(|node| node.fn_decl()) + .fn_decl()? + .inputs .into_iter() - .flat_map(|decl| decl.inputs) .skip(if is_method { 1 } else { 0 }) .map(|param| { if let hir::TyKind::Path(QPath::Resolved( @@ -2611,7 +2632,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { )) = param.kind { fn_node - .and_then(|node| node.generics()) + .generics() .into_iter() .flat_map(|generics| generics.params) .find(|param| ¶m.def_id.to_def_id() == res_def_id) @@ -2621,14 +2642,16 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> { }) .collect(); - let params: Vec<&hir::Param<'_>> = fn_node - .and_then(|node| node.body_id()) + let params: Vec<&hir::Param<'_>> = self + .tcx + .hir() + .body(fn_node.body_id()?) + .params .into_iter() - .flat_map(|id| self.tcx.hir().body(id).params) .skip(if is_method { 1 } else { 0 }) .collect(); - generic_params.into_iter().zip(params).collect() + Some(generic_params.into_iter().zip_eq(params).collect()) } } diff --git a/tests/crashes/128848.rs b/tests/crashes/128848.rs deleted file mode 100644 index 636811fc6b504..0000000000000 --- a/tests/crashes/128848.rs +++ /dev/null @@ -1,5 +0,0 @@ -//@ known-bug: rust-lang/rust#128848 - -fn f(a: T, b: T, c: T) { - f.call_once() -} diff --git a/tests/ui/mismatched_types/mismatch-args-crash-issue-128848.rs b/tests/ui/mismatched_types/mismatch-args-crash-issue-128848.rs new file mode 100644 index 0000000000000..bfd6d4aeae645 --- /dev/null +++ b/tests/ui/mismatched_types/mismatch-args-crash-issue-128848.rs @@ -0,0 +1,10 @@ +#![feature(fn_traits)] + +// Regression test for https://github.com/rust-lang/rust/issues/128848 + +fn f(a: T, b: T, c: T) { + f.call_once() + //~^ ERROR this method takes 1 argument but 0 arguments were supplied +} + +fn main() {} diff --git a/tests/ui/mismatched_types/mismatch-args-crash-issue-128848.stderr b/tests/ui/mismatched_types/mismatch-args-crash-issue-128848.stderr new file mode 100644 index 0000000000000..899cf435ddf5d --- /dev/null +++ b/tests/ui/mismatched_types/mismatch-args-crash-issue-128848.stderr @@ -0,0 +1,16 @@ +error[E0061]: this method takes 1 argument but 0 arguments were supplied + --> $DIR/mismatch-args-crash-issue-128848.rs:6:7 + | +LL | f.call_once() + | ^^^^^^^^^-- argument #1 of type `(_, _, _)` is missing + | +note: method defined here + --> $SRC_DIR/core/src/ops/function.rs:LL:COL +help: provide the argument + | +LL | f.call_once(/* args */) + | ~~~~~~~~~~~~ + +error: aborting due to 1 previous error + +For more information about this error, try `rustc --explain E0061`.