Skip to content

Commit

Permalink
Rollup merge of #106347 - estebank:removal-suggestion, r=TaKO8Ki
Browse files Browse the repository at this point in the history 
More accurate spans for arg removal suggestion

Partially address #106304.
  • Loading branch information
@matthiaskrgr
matthiaskrgr committed Feb 16, 2023
2 parents af3c8b2 + dff10d0 commit f65c6e4
Show file tree
Hide file tree
Showing 24 changed files with 286 additions and 232 deletions.
3 changes: 2 additions & 1 deletion compiler/rustc_errors/src/emitter.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1768,7 +1768,7 @@ impl EmitterWriter {

// Render the replacements for each suggestion
let suggestions = suggestion.splice_lines(sm);
debug!("emit_suggestion_default: suggestions={:?}", suggestions);
debug!(?suggestions);

if suggestions.is_empty() {
// Suggestions coming from macros can have malformed spans. This is a heavy handed
Expand Down Expand Up @@ -1797,6 +1797,7 @@ impl EmitterWriter {
for (complete, parts, highlights, only_capitalization) in
suggestions.iter().take(MAX_SUGGESTIONS)
{
debug!(?complete, ?parts, ?highlights);
notice_capitalization |= only_capitalization;

let has_deletion = parts.iter().any(|p| p.is_deletion(sm));
Expand Down
121 changes: 96 additions & 25 deletions compiler/rustc_hir_typeck/src/fn_ctxt/checks.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -755,15 +755,20 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}

errors.drain_filter(|error| {
let Error::Invalid(provided_idx, expected_idx, Compatibility::Incompatible(Some(e))) = error else { return false };
let (provided_ty, provided_span) = provided_arg_tys[*provided_idx];
let trace = mk_trace(provided_span, formal_and_expected_inputs[*expected_idx], provided_ty);
if !matches!(trace.cause.as_failure_code(*e), FailureCode::Error0308(_)) {
self.err_ctxt().report_and_explain_type_error(trace, *e).emit();
return true;
}
false
});
let Error::Invalid(
provided_idx,
expected_idx,
Compatibility::Incompatible(Some(e)),
) = error else { return false };
let (provided_ty, provided_span) = provided_arg_tys[*provided_idx];
let trace =
mk_trace(provided_span, formal_and_expected_inputs[*expected_idx], provided_ty);
if !matches!(trace.cause.as_failure_code(*e), FailureCode::Error0308(_)) {
self.err_ctxt().report_and_explain_type_error(trace, *e).emit();
return true;
}
false
});

// We're done if we found errors, but we already emitted them.
if errors.is_empty() {
Expand Down Expand Up @@ -864,7 +869,27 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
let mut suggestion_text = SuggestionText::None;

let ty_to_snippet = |ty: Ty<'tcx>, expected_idx: ExpectedIdx| {
if ty.is_unit() {
"()".to_string()
} else if ty.is_suggestable(tcx, false) {
format!("/* {} */", ty)
} else if let Some(fn_def_id) = fn_def_id
&& self.tcx.def_kind(fn_def_id).is_fn_like()
&& let self_implicit =
matches!(call_expr.kind, hir::ExprKind::MethodCall(..)) as usize
&& let Some(arg) = self.tcx.fn_arg_names(fn_def_id)
.get(expected_idx.as_usize() + self_implicit)
&& arg.name != kw::SelfLower
{
format!("/* {} */", arg.name)
} else {
"/* value */".to_string()
}
};

let mut errors = errors.into_iter().peekable();
let mut suggestions = vec![];
while let Some(error) = errors.next() {
match error {
Error::Invalid(provided_idx, expected_idx, compatibility) => {
Expand Down Expand Up @@ -905,7 +930,22 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
"".to_string()
};
labels
.push((provided_span, format!("argument{} unexpected", provided_ty_name)));
.push((provided_span, format!("unexpected argument{}", provided_ty_name)));
let mut span = provided_span;
if arg_idx.index() > 0
&& let Some((_, prev)) = provided_arg_tys
.get(ProvidedIdx::from_usize(arg_idx.index() - 1)
) {
// Include previous comma
span = span.with_lo(prev.hi());
} else if let Some((_, next)) = provided_arg_tys.get(
ProvidedIdx::from_usize(arg_idx.index() + 1),
) {
// Include next comma
span = span.until(*next);
}
suggestions.push((span, String::new()));

suggestion_text = match suggestion_text {
SuggestionText::None => SuggestionText::Remove(false),
SuggestionText::Remove(_) => SuggestionText::Remove(true),
Expand Down Expand Up @@ -1095,6 +1135,45 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
}

// Incorporate the argument changes in the removal suggestion.
// When a type is *missing*, and the rest are additional, we want to suggest these with a
// multipart suggestion, but in order to do so we need to figure out *where* the arg that
// was provided but had the wrong type should go, because when looking at `expected_idx`
// that is the position in the argument list in the definition, while `provided_idx` will
// not be present. So we have to look at what the *last* provided position was, and point
// one after to suggest the replacement. FIXME(estebank): This is hacky, and there's
// probably a better more involved change we can make to make this work.
// For example, if we have
// ```
// fn foo(i32, &'static str) {}
// foo((), (), ());
// ```
// what should be suggested is
// ```
// foo(/* i32 */, /* &str */);
// ```
// which includes the replacement of the first two `()` for the correct type, and the
// removal of the last `()`.
let mut prev = -1;
for (expected_idx, provided_idx) in matched_inputs.iter_enumerated() {
// We want to point not at the *current* argument expression index, but rather at the
// index position where it *should have been*, which is *after* the previous one.
if let Some(provided_idx) = provided_idx {
prev = provided_idx.index() as i64;
}
let idx = ProvidedIdx::from_usize((prev + 1) as usize);
if let None = provided_idx
&& let Some((_, arg_span)) = provided_arg_tys.get(idx)
{
// There is a type that was *not* found anywhere, so it isn't a move, but a
// replacement and we look at what type it should have been. This will allow us
// To suggest a multipart suggestion when encountering `foo(1, "")` where the def
// was `fn foo(())`.
let (_, expected_ty) = formal_and_expected_inputs[expected_idx];
suggestions.push((*arg_span, ty_to_snippet(expected_ty, expected_idx)));
}
}

// If we have less than 5 things to say, it would be useful to call out exactly what's wrong
if labels.len() <= 5 {
for (span, label) in labels {
Expand All @@ -1112,7 +1191,12 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
Some(format!("provide the argument{}", if plural { "s" } else { "" }))
}
SuggestionText::Remove(plural) => {
Some(format!("remove the extra argument{}", if plural { "s" } else { "" }))
err.multipart_suggestion(
&format!("remove the extra argument{}", if plural { "s" } else { "" }),
suggestions,
Applicability::HasPlaceholders,
);
None
}
SuggestionText::Swap => Some("swap these arguments".to_string()),
SuggestionText::Reorder => Some("reorder these arguments".to_string()),
Expand Down Expand Up @@ -1151,20 +1235,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
} else {
// Propose a placeholder of the correct type
let (_, expected_ty) = formal_and_expected_inputs[expected_idx];
if expected_ty.is_unit() {
"()".to_string()
} else if expected_ty.is_suggestable(tcx, false) {
format!("/* {} */", expected_ty)
} else if let Some(fn_def_id) = fn_def_id
&& self.tcx.def_kind(fn_def_id).is_fn_like()
&& let self_implicit = matches!(call_expr.kind, hir::ExprKind::MethodCall(..)) as usize
&& let Some(arg) = self.tcx.fn_arg_names(fn_def_id).get(expected_idx.as_usize() + self_implicit)
&& arg.name != kw::SelfLower
{
format!("/* {} */", arg.name)
} else {
"/* value */".to_string()
}
ty_to_snippet(expected_ty, expected_idx)
};
suggestion += &suggestion_text;
}
Expand Down
9 changes: 4 additions & 5 deletions tests/ui/alloc-error/alloc-error-handler-bad-signature-3.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,17 @@ LL | fn oom() -> ! {
| _-^^^^^^^^^^^^
LL | | loop {}
LL | | }
| |_- argument of type `core::alloc::Layout` unexpected
| | -
| | |
| |_unexpected argument of type `core::alloc::Layout`
| help: remove the extra argument
|
note: function defined here
--> $DIR/alloc-error-handler-bad-signature-3.rs:10:4
|
LL | fn oom() -> ! {
| ^^^
= note: this error originates in the attribute macro `alloc_error_handler` (in Nightly builds, run with -Z macro-backtrace for more info)
help: remove the extra argument
|
LL | fn oom() -> !() {
| ++

error: aborting due to previous error

Expand Down
9 changes: 4 additions & 5 deletions tests/ui/argument-suggestions/basic.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,16 @@ error[E0061]: this function takes 0 arguments but 1 argument was supplied
--> $DIR/basic.rs:21:5
|
LL | extra("");
| ^^^^^ -- argument of type `&'static str` unexpected
| ^^^^^ --
| |
| unexpected argument of type `&'static str`
| help: remove the extra argument
|
note: function defined here
--> $DIR/basic.rs:14:4
|
LL | fn extra() {}
| ^^^^^
help: remove the extra argument
|
LL | extra();
| ~~

error[E0061]: this function takes 1 argument but 0 arguments were supplied
--> $DIR/basic.rs:22:5
Expand Down
36 changes: 16 additions & 20 deletions tests/ui/argument-suggestions/exotic-calls.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,65 +2,61 @@ error[E0057]: this function takes 0 arguments but 1 argument was supplied
--> $DIR/exotic-calls.rs:2:5
|
LL | t(1i32);
| ^ ---- argument of type `i32` unexpected
| ^ ----
| |
| unexpected argument of type `i32`
| help: remove the extra argument
|
note: callable defined here
--> $DIR/exotic-calls.rs:1:11
|
LL | fn foo<T: Fn()>(t: T) {
| ^^^^
help: remove the extra argument
|
LL | t();
| ~~

error[E0057]: this function takes 0 arguments but 1 argument was supplied
--> $DIR/exotic-calls.rs:7:5
|
LL | t(1i32);
| ^ ---- argument of type `i32` unexpected
| ^ ----
| |
| unexpected argument of type `i32`
| help: remove the extra argument
|
note: type parameter defined here
--> $DIR/exotic-calls.rs:6:11
|
LL | fn bar(t: impl Fn()) {
| ^^^^^^^^^
help: remove the extra argument
|
LL | t();
| ~~

error[E0057]: this function takes 0 arguments but 1 argument was supplied
--> $DIR/exotic-calls.rs:16:5
|
LL | baz()(1i32)
| ^^^^^ ---- argument of type `i32` unexpected
| ^^^^^ ----
| |
| unexpected argument of type `i32`
| help: remove the extra argument
|
note: opaque type defined here
--> $DIR/exotic-calls.rs:11:13
|
LL | fn baz() -> impl Fn() {
| ^^^^^^^^^
help: remove the extra argument
|
LL | baz()()
| ~~

error[E0057]: this function takes 0 arguments but 1 argument was supplied
--> $DIR/exotic-calls.rs:22:5
|
LL | x(1i32);
| ^ ---- argument of type `i32` unexpected
| ^ ----
| |
| unexpected argument of type `i32`
| help: remove the extra argument
|
note: closure defined here
--> $DIR/exotic-calls.rs:21:13
|
LL | let x = || {};
| ^^
help: remove the extra argument
|
LL | x();
| ~~

error: aborting due to 4 previous errors

Expand Down
Loading

0 comments on commit f65c6e4

Please sign in to comment.