Suggest tuple-parentheses for enum variants #90677

bobrippling · 2021-11-07T22:57:57Z

This follows on from #86493 / #86481, making the parentheses suggestion. To summarise, given the following code:

fn f() -> Option<(i32, i8)> {
    Some(1, 2)
}

The current output is:

error[E0061]: this enum variant takes 1 argument but 2 arguments were supplied
 --> b.rs:2:5
  |
2 |     Some(1, 2)
  |     ^^^^ -  - supplied 2 arguments
  |     |
  |     expected 1 argument

error: aborting due to previous error

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

With this change, rustc will now suggest parentheses when:

The callee is expecting a single tuple argument
The number of arguments passed matches the element count in the above tuple
The arguments' types match the tuple's fields

error[E0061]: this enum variant takes 1 argument but 2 arguments were supplied
 --> b.rs:2:5
  |
2 |     Some(1, 2)
  |     ^^^^ -  - supplied 2 arguments
  |
help: use parentheses to construct a tuple
  |
2 |     Some((1, 2))
  |          +    +

rust-highfive · 2021-11-07T22:58:00Z

r? @michaelwoerister

(rust-highfive has picked a reviewer for you, use r? to override)

bobrippling · 2021-11-07T23:04:13Z

I would quite like to also handle normal functions with this change, currently the below doesn't get the tuple suggestion:

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

pub fn main() {
    f(1, 2);
}

This appears to be because expected_inputs_for_expected_output short-circuits because there's no expectation on the type. I suspect this isn't the right rabbit-hole to be going down though.

camelid · 2021-11-08T03:23:47Z

Can you add a UI test for this? It would go in src/test/ui/suggestions (or a similar location) and look roughly like this:

fn f() -> Option<(i32, i8)> {
    Some(1, 2)
    //~^ ERROR this enum variant takes 1 argument but 2 arguments were supplied
}

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

michaelwoerister · 2021-11-08T09:24:15Z

r? rust-lang/diagnostics

src/test/ui/suggestions/args-instead-of-tuple.rs

oli-obk · 2021-11-19T16:24:30Z

I would quite like to also handle normal functions with this change, currently the below doesn't get the tuple suggestion:
fn f(_: (i32, i32)) {
}

pub fn main() {
    f(1, 2);
}
This appears to be because expected_inputs_for_expected_output short-circuits because there's no expectation on the type. I suspect this isn't the right rabbit-hole to be going down though.

I think we could merge without this change, as it's not that common to have tuple arguments to regular functions. Though I may just be blind here due to my codebases not having this ^^

bobrippling · 2021-11-20T12:05:53Z

I would quite like to also handle normal functions with this change, currently the below doesn't get the tuple suggestion:
fn f(_: (i32, i32)) {
}

pub fn main() {
    f(1, 2);
}
[...]
I think we could merge without this change, as it's not that common to have tuple arguments to regular functions. Though I may just be blind here due to my codebases not having this ^^

Luckily there weren't many adjustments to make to handle this too, although I'd agree with you that it's far less common, at least in codebases I've been through too.

Let me know what you think - rebased + tidied the commits.

oli-obk · 2021-11-24T16:38:42Z

r? @camelid

compiler/rustc_infer/src/infer/error_reporting/mod.rs

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

camelid · 2021-11-28T00:29:28Z

Left some comments.

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

compiler/rustc_infer/src/infer/error_reporting/mod.rs

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

camelid · 2022-01-25T19:46:18Z

If you rebase, I can approve this :)

…ment

bobrippling · 2022-01-25T22:55:48Z

No worries, thanks for sticking with me and for the review! I appreciate the guidance :)

camelid

Thanks again!

camelid · 2022-01-27T21:58:56Z

@bors r+

bors · 2022-01-27T21:58:58Z

📌 Commit a8bac98 has been approved by camelid

…=camelid Suggest tuple-parentheses for enum variants This follows on from rust-lang#86493 / rust-lang#86481, making the parentheses suggestion. To summarise, given the following code: ```rust fn f() -> Option<(i32, i8)> { Some(1, 2) } ``` The current output is: ``` error[E0061]: this enum variant takes 1 argument but 2 arguments were supplied --> b.rs:2:5 | 2 | Some(1, 2) | ^^^^ - - supplied 2 arguments | | | expected 1 argument error: aborting due to previous error For more information about this error, try `rustc --explain E0061`. ``` With this change, `rustc` will now suggest parentheses when: - The callee is expecting a single tuple argument - The number of arguments passed matches the element count in the above tuple - The arguments' types match the tuple's fields ``` error[E0061]: this enum variant takes 1 argument but 2 arguments were supplied --> b.rs:2:5 | 2 | Some(1, 2) | ^^^^ - - supplied 2 arguments | help: use parentheses to construct a tuple | 2 | Some((1, 2)) | + + ```

bors · 2022-01-28T09:46:25Z

⌛ Testing commit a8bac98 with merge e0e70c0...

bors · 2022-01-28T12:46:06Z

☀️ Test successful - checks-actions
Approved by: camelid
Pushing e0e70c0 to master...

jackh726 · 2022-01-28T13:58:30Z

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

-            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() {


This is essentially duplicated on line 217.

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

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?

I'm thinking of trying to refactor it myself.

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.

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).

rust-timer · 2022-01-28T14:17:39Z

Finished benchmarking commit (e0e70c0): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

… r=camelid Suggest 1-tuple parentheses on exprs without existing parens A follow-on from rust-lang#86116, split out from rust-lang#90677. This alters the suggestion to add a trailing comma to create a 1-tuple - previously we would only apply this if the relevant expression was parenthesised. We now make the suggestion regardless of parentheses, which reduces the fragility of the check (w.r.t formatting). e.g. ```rust let a: Option<(i32,)> = Some(3); ``` gets the below suggestion: ```rust let a: Option<(i32,)> = Some((3,)); // ^ ^^ ``` This change also improves the suggestion in other ways, such as by only making the suggestion if the types would match after the suggestion is applied and making the suggestion a multipart suggestion.

rust-highfive assigned michaelwoerister Nov 7, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 7, 2021

bobrippling force-pushed the suggest-tuple-parens branch from d4db5a9 to 0b42630 Compare November 7, 2021 23:12

camelid added the A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` label Nov 8, 2021

camelid reviewed Nov 8, 2021

View reviewed changes

compiler/rustc_typeck/src/check/fn_ctxt/checks.rs Outdated Show resolved Hide resolved

compiler/rustc_typeck/src/check/fn_ctxt/checks.rs Outdated Show resolved Hide resolved

rust-highfive assigned oli-obk and unassigned michaelwoerister Nov 8, 2021

This comment has been minimized.

Sign in to view

bobrippling force-pushed the suggest-tuple-parens branch from 0013b90 to f58ef0f Compare November 9, 2021 23:34

bobrippling commented Nov 9, 2021

View reviewed changes

src/test/ui/suggestions/args-instead-of-tuple.rs Show resolved Hide resolved

This comment has been minimized.

Sign in to view

bobrippling force-pushed the suggest-tuple-parens branch from 23bb0af to ec8a46f Compare November 20, 2021 11:01

bobrippling marked this pull request as ready for review November 20, 2021 12:05

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Nov 22, 2021

bobrippling requested a review from camelid November 23, 2021 11:15

rust-highfive assigned camelid and unassigned oli-obk Nov 24, 2021

camelid reviewed Nov 24, 2021

View reviewed changes

compiler/rustc_infer/src/infer/error_reporting/mod.rs Outdated Show resolved Hide resolved

compiler/rustc_typeck/src/check/fn_ctxt/checks.rs Outdated Show resolved Hide resolved

camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 24, 2021

camelid reviewed Nov 28, 2021

View reviewed changes

camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 25, 2022

bobrippling added 5 commits January 25, 2022 22:51

Suggest tuple-parentheses when passing N arguments to an N-tuple argu…

94c300a

…ment

Test tuple suggestions, including tuple-as-function-argument

80059f9

Compare tuple element & arg types before suggesting a tuple

54d2d30

Handle generics with ParamEnv

a129a85

Remove 1-tuple unreachable case

a8bac98

bobrippling force-pushed the suggest-tuple-parens branch from 0afa9b2 to a8bac98 Compare January 25, 2022 22:56

camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 26, 2022

camelid approved these changes Jan 27, 2022

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 27, 2022

matthiaskrgr mentioned this pull request Jan 28, 2022

Rollup of 8 pull requests #93409

Closed

bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 28, 2022

bors merged commit e0e70c0 into rust-lang:master Jan 28, 2022

rustbot added this to the 1.60.0 milestone Jan 28, 2022

bors mentioned this pull request Jan 28, 2022

Better method call error messages #92364

Merged

jackh726 reviewed Jan 28, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Suggest tuple-parentheses for enum variants #90677

Suggest tuple-parentheses for enum variants #90677

bobrippling commented Nov 7, 2021 •

edited by camelid

Loading

rust-highfive commented Nov 7, 2021

bobrippling commented Nov 7, 2021

camelid commented Nov 8, 2021

michaelwoerister commented Nov 8, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

oli-obk commented Nov 19, 2021

bobrippling commented Nov 20, 2021

oli-obk commented Nov 24, 2021

camelid commented Nov 28, 2021

camelid commented Jan 25, 2022

bobrippling commented Jan 25, 2022

camelid left a comment

camelid commented Jan 27, 2022

bors commented Jan 27, 2022

bors commented Jan 28, 2022

bors commented Jan 28, 2022

jackh726 Jan 28, 2022

jackh726 Jan 28, 2022

camelid Jan 28, 2022

camelid Jan 28, 2022

jackh726 Jan 28, 2022

bobrippling Jan 28, 2022

rust-timer commented Jan 28, 2022

Suggest tuple-parentheses for enum variants #90677

Suggest tuple-parentheses for enum variants #90677

Conversation

bobrippling commented Nov 7, 2021 • edited by camelid Loading

rust-highfive commented Nov 7, 2021

bobrippling commented Nov 7, 2021

camelid commented Nov 8, 2021

michaelwoerister commented Nov 8, 2021

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

oli-obk commented Nov 19, 2021

bobrippling commented Nov 20, 2021

oli-obk commented Nov 24, 2021

camelid commented Nov 28, 2021

camelid commented Jan 25, 2022

bobrippling commented Jan 25, 2022

camelid left a comment

Choose a reason for hiding this comment

camelid commented Jan 27, 2022

bors commented Jan 27, 2022

bors commented Jan 28, 2022

bors commented Jan 28, 2022

jackh726 Jan 28, 2022

Choose a reason for hiding this comment

jackh726 Jan 28, 2022

Choose a reason for hiding this comment

camelid Jan 28, 2022

Choose a reason for hiding this comment

camelid Jan 28, 2022

Choose a reason for hiding this comment

jackh726 Jan 28, 2022

Choose a reason for hiding this comment

bobrippling Jan 28, 2022

Choose a reason for hiding this comment

rust-timer commented Jan 28, 2022

bobrippling commented Nov 7, 2021 •

edited by camelid

Loading