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 i += 1 when we see i++ or ++i #83536

Closed
wants to merge 2 commits into from

Conversation

camelid
Copy link
Member

@camelid camelid commented Mar 26, 2021

Closes #83502 (for i++ and ++i; --i should be covered by #82987 and i--
is tricky to handle).

@camelid camelid added A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. labels Mar 26, 2021
@rust-highfive
Copy link
Collaborator

r? @matthewjasper

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 26, 2021
@camelid
Copy link
Member Author

camelid commented Mar 26, 2021

r? @estebank

Comment on lines +8 to +11
help: try using `+= 1` instead
|
LL | i += 1;
| ^^^^
Copy link
Member Author

Choose a reason for hiding this comment

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

One issue with using a structured suggestion is that we will suggest invalid code in the case of

fn main() {
    let mut i = 0;
    while i++ < 5 { /* ... */ }

We suggest

fn main() {
    let mut i = 0;
    while i += 1 < 5 { /* ... */ }
}

Errors:

   Compiling playground v0.0.1 (/playground)
error[E0308]: mismatched types
 --> src/main.rs:3:11
  |
3 |     while i += 1 < 5 { /* ... */ }
  |           ^^^^^^^^^^ expected `bool`, found `()`

error[E0277]: cannot add-assign `bool` to `{integer}`
 --> src/main.rs:3:13
  |
3 |     while i += 1 < 5 { /* ... */ }
  |             ^^ no implementation for `{integer} += bool`
  |
  = help: the trait `AddAssign<bool>` is not implemented for `{integer}`

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0277, E0308.
For more information about an error, try `rustc --explain E0277`.
error: could not compile `playground`

To learn more, run the command again with --verbose.

And adding parentheses doesn't fix it:

fn main() {
    let mut i = 0;
    while (i += 1) < 5 { /* ... */ }
}

Errors:

   Compiling playground v0.0.1 (/playground)
error[E0308]: mismatched types
 --> src/main.rs:3:22
  |
3 |     while (i += 1) < 5 { /* ... */ }
  |                      ^ expected `()`, found integer

error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.
error: could not compile `playground`

To learn more, run the command again with --verbose.


So maybe it would be better to just skip the structured suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking whether it would make sense to try to suggest while { i += 1; i } < 5?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's interesting! The advantage of that is that it will preserve the behavior of i++ in other languages. Would we always suggest { i += 1; i } or would we sometimes suggest just i += 1? I think it would probably be pretty hard to have conditional logic in this case, so I vote for unconditionally using { i += 1; i }, unless there's a simple way to encode the logic.

Also, what should we suggest in the ++i case? In that case, { i += 1; i } is not the correct behavior; rather, the equivalent in Rust is { let temp = i; i += 1; temp }. The problem with suggesting that is that the user may have code like:

++temp;

and then we would suggest

{ let temp = temp; temp += 1; temp }

which is incorrect.

Copy link
Member Author

Choose a reason for hiding this comment

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

So, thinking about it more, maybe simply suggesting i += 1 in all cases is good enough? Or, alternatively, would it be better to just include a note and skip the structured suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

You wrote that backwards: preincrement ++i is { i += 1; i }, and postincrement i++ is { let temp = i; i += 1; temp }.

Comment on lines +8 to +11
++i; //~ ERROR
}

fn main() {}
Copy link
Member Author

Choose a reason for hiding this comment

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

Probably should add tests for while i++ < 5 etc.

@rust-log-analyzer

This comment has been minimized.

&& self.look_ahead(2, |t| !t.is_lit())
{
err.note("Rust has no dedicated increment and decrement operators");
err.help("try using `+= 1` instead");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
err.help("try using `+= 1` instead");
let lo = self.token.span; // `+`
self.bump(); // `+`
let pre_sp = lo.to(self.token.span);
err.multipart_suggestion(
"try using `+= 1` instead"
vec![
(pre_sp, String::new()),
(self.token.span.shrink_to_hi(), " += 1".to_string()),
],
Applicability::MachineApplicable,
);
self.bump(); // ident

Copy link
Member Author

Choose a reason for hiding this comment

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

Won't this turn ++i into += 1i?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, good catch. You'll need to account for ++i and i++ specifically.

@JohnCSimon
Copy link
Member

Ping from triage:
@camelid can you please address the review suggestion from estebank? Thanks

@rustbot label: -S-waiting-on-review +S-waiting-on-author

@rustbot rustbot 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 Apr 18, 2021
@camelid 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 Apr 27, 2021
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 15, 2021
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 11, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 27, 2021
@Dylan-DPC-zz
Copy link

closing this due to inactivity

@Dylan-DPC-zz Dylan-DPC-zz added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 3, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 3, 2022
Suggest `i += 1` when we see `i++` or `++i`

Closes rust-lang#83502 (for `i++` and `++i`; `--i` should be covered by rust-lang#82987, and `i--`
is tricky to handle).

This is a continuation of rust-lang#83536.

r? `@estebank`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add diagnostic for ++ and -- usages
10 participants