Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggest tuple-parentheses for enum variants #90677

Merged
merged 5 commits into from
Jan 28, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 68 additions & 6 deletions compiler/rustc_typeck/src/check/fn_ctxt/checks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,11 @@ use crate::structured_errors::StructuredDiagnostic;
use std::iter;
use std::slice;

struct FnArgsAsTuple<'hir> {
first: &'hir hir::Expr<'hir>,
last: &'hir hir::Expr<'hir>,
}

impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
pub(in super::super) fn check_casts(&self) {
let mut deferred_cast_checks = self.deferred_cast_checks.borrow_mut();
Expand Down Expand Up @@ -127,8 +132,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {

let expected_arg_count = formal_input_tys.len();

// expected_count, arg_count, error_code, sugg_unit
let mut error: Option<(usize, usize, &str, bool)> = None;
// expected_count, arg_count, error_code, sugg_unit, sugg_tuple_wrap_args
let mut error: Option<(usize, usize, &str, bool, Option<FnArgsAsTuple<'_>>)> = None;

// If the arguments should be wrapped in a tuple (ex: closures), unwrap them here
let (formal_input_tys, expected_input_tys) = if tuple_arguments == TupleArguments {
Expand All @@ -138,7 +143,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
ty::Tuple(arg_types) => {
// Argument length differs
if arg_types.len() != provided_args.len() {
error = Some((arg_types.len(), provided_args.len(), "E0057", false));
error = Some((arg_types.len(), provided_args.len(), "E0057", false, None));
}
let expected_input_tys = match expected_input_tys.get(0) {
Some(&ty) => match ty.kind() {
Expand Down Expand Up @@ -169,7 +174,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
if supplied_arg_count >= expected_arg_count {
(formal_input_tys.to_vec(), expected_input_tys)
} else {
error = Some((expected_arg_count, supplied_arg_count, "E0060", false));
error = Some((expected_arg_count, supplied_arg_count, "E0060", false, None));
(self.err_args(supplied_arg_count), vec![])
}
} else {
Expand All @@ -181,7 +186,25 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
} else {
false
};
error = Some((expected_arg_count, supplied_arg_count, "E0061", sugg_unit));

// are we passing elements of a tuple without the tuple parentheses?
let expected_input_tys = if expected_input_tys.is_empty() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is essentially duplicated on line 217.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is because we need it for the call to suggested_tuple_wrap?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this whole (pre-existing) function could be cleaned up a lot in general. Like, why does this if (and the one on L217) even need to exist? And why is an empty list used as a sentinel value?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking of trying to refactor it myself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this gets changed quite a bit in #92364

Changing empty list as a sentinel is something orthogonal that I've thought about and worth doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the empty list led to a lot of confusion during the development of this too - like you say, it being used as a sentinel could be made more explicit (or even removed).

// In most cases we can use expected_input_tys, but some callers won't have the type
// information, in which case we fall back to the types from the input expressions.
formal_input_tys
} else {
&*expected_input_tys
};

let sugg_tuple_wrap_args = self.suggested_tuple_wrap(expected_input_tys, provided_args);
camelid marked this conversation as resolved.
Show resolved Hide resolved

error = Some((
expected_arg_count,
supplied_arg_count,
"E0061",
sugg_unit,
sugg_tuple_wrap_args,
));
(self.err_args(supplied_arg_count), vec![])
};

Expand Down Expand Up @@ -305,7 +328,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}

// If there was an error in parameter count, emit that here
if let Some((expected_count, arg_count, err_code, sugg_unit)) = error {
if let Some((expected_count, arg_count, err_code, sugg_unit, sugg_tuple_wrap_args)) = error
{
let (span, start_span, args, ctor_of) = match &call_expr.kind {
hir::ExprKind::Call(
hir::Expr {
Expand Down Expand Up @@ -408,6 +432,15 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
String::from("()"),
Applicability::MachineApplicable,
);
} else if let Some(FnArgsAsTuple { first, last }) = sugg_tuple_wrap_args {
err.multipart_suggestion(
"use parentheses to construct a tuple",
vec![
(first.span.shrink_to_lo(), '('.to_string()),
(last.span.shrink_to_hi(), ')'.to_string()),
],
Applicability::MachineApplicable,
);
} else {
err.span_label(
span,
Expand Down Expand Up @@ -457,6 +490,35 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
}
}

fn suggested_tuple_wrap(
&self,
expected_input_tys: &[Ty<'tcx>],
provided_args: &'tcx [hir::Expr<'tcx>],
) -> Option<FnArgsAsTuple<'_>> {
let [expected_arg_type] = &expected_input_tys[..] else { return None };

let ty::Tuple(expected_elems) = self.resolve_vars_if_possible(*expected_arg_type).kind()
else { return None };

let expected_types: Vec<_> = expected_elems.iter().map(|k| k.expect_ty()).collect();
let supplied_types: Vec<_> = provided_args.iter().map(|arg| self.check_expr(arg)).collect();

let all_match = iter::zip(expected_types, supplied_types)
.all(|(expected, supplied)| self.can_eq(self.param_env, expected, supplied).is_ok());

if all_match {
match provided_args {
[] => None,
camelid marked this conversation as resolved.
Show resolved Hide resolved
[_] => unreachable!(
"shouldn't reach here - need count mismatch between 1-tuple and 1-argument"
),
[first, .., last] => Some(FnArgsAsTuple { first, last }),
}
} else {
None
}
}

// AST fragment checking
pub(in super::super) fn check_lit(
&self,
Expand Down
16 changes: 16 additions & 0 deletions src/test/ui/suggestions/args-instead-of-tuple-errors.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// Ensure we don't suggest tuple-wrapping when we'd end up with a type error

fn main() {
// we shouldn't suggest to fix these - `2` isn't a `bool`

let _: Option<(i32, bool)> = Some(1, 2);
//~^ ERROR this enum variant takes 1 argument but 2 arguments were supplied
int_bool(1, 2);
//~^ ERROR this function takes 1 argument but 2 arguments were supplied

let _: Option<(i8,)> = Some();
//~^ ERROR this enum variant takes 1 argument but 0 arguments were supplied
}

fn int_bool(_: (i32, bool)) {
}
33 changes: 33 additions & 0 deletions src/test/ui/suggestions/args-instead-of-tuple-errors.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
error[E0061]: this enum variant takes 1 argument but 2 arguments were supplied
--> $DIR/args-instead-of-tuple-errors.rs:6:34
|
LL | let _: Option<(i32, bool)> = Some(1, 2);
| ^^^^ - - supplied 2 arguments
| |
| expected 1 argument

error[E0061]: this function takes 1 argument but 2 arguments were supplied
--> $DIR/args-instead-of-tuple-errors.rs:8:5
|
LL | int_bool(1, 2);
| ^^^^^^^^ - - supplied 2 arguments
| |
| expected 1 argument
|
note: function defined here
--> $DIR/args-instead-of-tuple-errors.rs:15:4
|
LL | fn int_bool(_: (i32, bool)) {
| ^^^^^^^^ --------------

error[E0061]: this enum variant takes 1 argument but 0 arguments were supplied
--> $DIR/args-instead-of-tuple-errors.rs:11:28
|
LL | let _: Option<(i8,)> = Some();
| ^^^^-- supplied 0 arguments
| |
| expected 1 argument

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0061`.
27 changes: 27 additions & 0 deletions src/test/ui/suggestions/args-instead-of-tuple.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// Test suggesting tuples where bare arguments may have been passed
// See issue #86481 for details.

// run-rustfix

fn main() {
let _: Result<(i32, i8), ()> = Ok((1, 2));
//~^ ERROR this enum variant takes 1 argument but 2 arguments were supplied
let _: Option<(i32, i8, &'static str)> = Some((1, 2, "hi"));
//~^ ERROR this enum variant takes 1 argument but 3 arguments were supplied
let _: Option<()> = Some(());
//~^ ERROR this enum variant takes 1 argument but 0 arguments were supplied

two_ints((1, 2)); //~ ERROR this function takes 1 argument

with_generic((3, 4)); //~ ERROR this function takes 1 argument
}

fn two_ints(_: (i32, i32)) {
}

fn with_generic<T: Copy + Send>((a, b): (i32, T)) {
if false {
// test generics/bound handling
with_generic((a, b)); //~ ERROR this function takes 1 argument
}
}
27 changes: 27 additions & 0 deletions src/test/ui/suggestions/args-instead-of-tuple.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
// Test suggesting tuples where bare arguments may have been passed
bobrippling marked this conversation as resolved.
Show resolved Hide resolved
// See issue #86481 for details.

// run-rustfix

fn main() {
let _: Result<(i32, i8), ()> = Ok(1, 2);
//~^ ERROR this enum variant takes 1 argument but 2 arguments were supplied
let _: Option<(i32, i8, &'static str)> = Some(1, 2, "hi");
//~^ ERROR this enum variant takes 1 argument but 3 arguments were supplied
let _: Option<()> = Some();
bobrippling marked this conversation as resolved.
Show resolved Hide resolved
//~^ ERROR this enum variant takes 1 argument but 0 arguments were supplied

two_ints(1, 2); //~ ERROR this function takes 1 argument

with_generic(3, 4); //~ ERROR this function takes 1 argument
}

fn two_ints(_: (i32, i32)) {
}

fn with_generic<T: Copy + Send>((a, b): (i32, T)) {
if false {
// test generics/bound handling
with_generic(a, b); //~ ERROR this function takes 1 argument
}
}
84 changes: 84 additions & 0 deletions src/test/ui/suggestions/args-instead-of-tuple.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
error[E0061]: this enum variant takes 1 argument but 2 arguments were supplied
--> $DIR/args-instead-of-tuple.rs:7:36
|
LL | let _: Result<(i32, i8), ()> = Ok(1, 2);
| ^^ - - supplied 2 arguments
|
help: use parentheses to construct a tuple
|
LL | let _: Result<(i32, i8), ()> = Ok((1, 2));
| + +

error[E0061]: this enum variant takes 1 argument but 3 arguments were supplied
--> $DIR/args-instead-of-tuple.rs:9:46
|
LL | let _: Option<(i32, i8, &'static str)> = Some(1, 2, "hi");
| ^^^^ - - ---- supplied 3 arguments
|
help: use parentheses to construct a tuple
|
LL | let _: Option<(i32, i8, &'static str)> = Some((1, 2, "hi"));
| + +

error[E0061]: this enum variant takes 1 argument but 0 arguments were supplied
--> $DIR/args-instead-of-tuple.rs:11:25
|
LL | let _: Option<()> = Some();
| ^^^^-- supplied 0 arguments
|
help: expected the unit value `()`; create it with empty parentheses
|
LL | let _: Option<()> = Some(());
| ++

error[E0061]: this function takes 1 argument but 2 arguments were supplied
--> $DIR/args-instead-of-tuple.rs:14:5
|
LL | two_ints(1, 2);
| ^^^^^^^^ - - supplied 2 arguments
|
note: function defined here
--> $DIR/args-instead-of-tuple.rs:19:4
|
LL | fn two_ints(_: (i32, i32)) {
| ^^^^^^^^ -------------
help: use parentheses to construct a tuple
|
LL | two_ints((1, 2));
| + +

error[E0061]: this function takes 1 argument but 2 arguments were supplied
--> $DIR/args-instead-of-tuple.rs:16:5
|
LL | with_generic(3, 4);
| ^^^^^^^^^^^^ - - supplied 2 arguments
|
note: function defined here
--> $DIR/args-instead-of-tuple.rs:22:4
|
LL | fn with_generic<T: Copy + Send>((a, b): (i32, T)) {
| ^^^^^^^^^^^^ ----------------
help: use parentheses to construct a tuple
|
LL | with_generic((3, 4));
| + +

error[E0061]: this function takes 1 argument but 2 arguments were supplied
--> $DIR/args-instead-of-tuple.rs:25:9
|
LL | with_generic(a, b);
| ^^^^^^^^^^^^ - - supplied 2 arguments
|
note: function defined here
--> $DIR/args-instead-of-tuple.rs:22:4
|
LL | fn with_generic<T: Copy + Send>((a, b): (i32, T)) {
| ^^^^^^^^^^^^ ----------------
help: use parentheses to construct a tuple
|
LL | with_generic((a, b));
| + +

error: aborting due to 6 previous errors

For more information about this error, try `rustc --explain E0061`.