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

Rollup merge of #129320 - jder:issue-128848, r=compiler-errors

Browse Source 
Fix crash when labeling arguments for call_once and friends

When calling a method on Fn* traits explicitly, argument diagnostics should point at the called method (eg Fn::call_once), not the underlying callee.

This PR makes 3 main changes:

* It uses TupleArguments to detect if the user called a Fn* method directly (`my_fn.call_once(…)`) or implicitly (`my_fn(…)`). If it was explicit, argument diagnostics should point at the call_once method, not the underlying callable.
* The previous state was causing confusion between the two arguments lists (which could be different lengths), causing an out-of-bounds slice indexing in #128848. I added a length assert to capture the requirement in case this regresses or happens in another case.
* Unfortunately, this assert tripped when the required arguments information was not available (`self.get_hir_params_with_generics` was returning an empty Vec), so I've updated that to return None when that information is not available. (cc `@strottos` if you have any comments, since you added this function in #121595) Sorry this causes a bunch of indentation changes, recommend reviewing [ignoring whitespace](https://github.com/rust-lang/rust/pull/129320/files?w=1).)

This is my first rustc PR, so please call out if you'd like this split into more commits (or PRs), style nits, etc. I will add a few comments/questions inline. Thank you!

Fixes #128848
This commit is contained in:
Matthias Krüger 2024-09-13 18:25:44 +02:00 committed by GitHub
commit 24da940631
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
5 changed files with 251 additions and 207 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
compiler/rustc_hir_typeck/src/callee.rs
View File

@ -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>),
}

425
compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs
View File

@ -506,6 +506,7 @@ fn variadic_error<'tcx>(
fn_def_id,
call_span,
call_expr,
tuple_arguments,
);
}
}
@ -520,6 +521,7 @@ fn report_arg_errors(
fn_def_id: Option<DefId>,
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 @@ fn has_error_or_infer<'tcx>(tys: impl IntoIterator<Item = Ty<'tcx>>) -> bool {
&matched_inputs,
&formal_and_expected_inputs,
is_method,
tuple_arguments,
);
suggest_confusable(&mut err);
return err.emit();
@ -1001,6 +1004,7 @@ fn has_error_or_infer<'tcx>(tys: impl IntoIterator<Item = Ty<'tcx>>) -> bool {
&matched_inputs,
&formal_and_expected_inputs,
is_method,
tuple_arguments,
);
suggest_confusable(&mut err);
return err.emit();
@ -1448,6 +1452,7 @@ enum SuggestionText {
&matched_inputs,
&formal_and_expected_inputs,
is_method,
tuple_arguments,
);
// And add a suggestion block for all of the parameters
@ -2219,21 +2224,27 @@ fn label_fn_like(
matched_inputs: &IndexVec<ExpectedIdx, Option<ProvidedIdx>>,
formal_and_expected_inputs: &IndexVec<ExpectedIdx, (Ty<'tcx>, 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,81 +2314,154 @@ fn label_fn_like(
{
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()
})
.map(|(other_idx, (_, other_param))| (other_idx, *other_param))
.collect();
let check_for_matched_generics = check_for_matched_generics();
if !other_params_matched.is_empty() {
let other_param_matched_names: Vec<String> = other_params_matched
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;
};
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 {
.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();
if !other_params_matched.is_empty() {
let other_param_matched_names: Vec<String> = other_params_matched
.iter()
.map(|(_, other_param)| {
if let hir::PatKind::Binding(_, _, ident, _) = other_param.pat.kind
{
format!("`{ident}`")
} else {
"{unknown}".to_string()
}
})
.collect();
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!(
"{} {} 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,
),
);
} 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<String> = 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()
@ -2385,84 +2469,18 @@ fn label_fn_like(
})
.collect();
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() {
if !param_idents_matching.is_empty() {
spans.push_span_label(
param.span,
generic_param.span,
format!(
"{} {} 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,
),
);
} 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),
"{} all reference this parameter {}",
display_list_with_comma_and(&param_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<String> = 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(&param_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 @@ fn label_generic_mismatches(
return;
};
let params_with_generics = self.get_hir_params_with_generics(def_id, is_method);
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;
}
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(generic_param) = generic_param else {
continue;
};
let mut idxs_matched: Vec<usize> = 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;
}
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);
}
let Some((_, matched_arg_span)) = provided_arg_tys.get(idx.into()) else {
continue;
};
let Some(generic_param) = generic_param else {
continue;
};
let mut idxs_matched: Vec<usize> = 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;
}
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);
}
}
/// 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<Vec<(Option<&hir::GenericParam<'_>>, &hir::Param<'_>)>> {
let fn_node = self.tcx.hir().get_if_local(def_id)?;
let generic_params: Vec<Option<&hir::GenericParam<'_>>> = 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 @@ fn get_hir_params_with_generics(
)) = param.kind
{
fn_node
.and_then(|node| node.generics())
.generics()
.into_iter()
.flat_map(|generics| generics.params)
.find(|param| &param.def_id.to_def_id() == res_def_id)
@ -2621,14 +2642,16 @@ fn get_hir_params_with_generics(
})
.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())
}
}

5
tests/crashes/128848.rs
View File

@ -1,5 +0,0 @@
//@ known-bug: rust-lang/rust#128848
fn f<T>(a: T, b: T, c: T) {
f.call_once()
}

10
tests/ui/mismatched_types/mismatch-args-crash-issue-128848.rs Normal file
View File

@ -0,0 +1,10 @@
#![feature(fn_traits)]
// Regression test for https://github.com/rust-lang/rust/issues/128848
fn f<T>(a: T, b: T, c: T) {
f.call_once()
//~^ ERROR this method takes 1 argument but 0 arguments were supplied
}
fn main() {}

16
tests/ui/mismatched_types/mismatch-args-crash-issue-128848.stderr Normal file
View File

@ -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`.