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.

Sign up for GitHub

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

Jump to bottom

Suggest i += 1 when we see i++ or ++i #83536

Closed
wants to merge 2 commits into from
Closed

Suggest i += 1 when we see i++ or ++i #83536

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5b6a96d
Suggest `i += 1` when we see `i++` or `++i`
camelid Mar 26, 2021
d686f13
Fix false positive
camelid Mar 26, 2021
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
17 changes: 17 additions & 0 deletions compiler/rustc_parse/src/parser/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1775,6 +1775,23 @@ impl<'a> Parser<'a> {
self.sess.expr_parentheses_needed(&mut err, *sp, None);
}
err.span_label(span, "expected expression");
if self.prev_token.kind == TokenKind::BinOp(token::Plus)
&& self.token.kind == TokenKind::BinOp(token::Plus)
{
let span = self.prev_token.span.to(self.token.span);
err.note("Rust has no dedicated increment and decrement operators");
err.span_suggestion_verbose(
span,
"try using `+= 1` instead",
" += 1".into(),
Applicability::Unspecified,
);
} else if self.token.kind == TokenKind::BinOp(token::Plus)
&& self.look_ahead(1, |t| t.kind == TokenKind::BinOp(token::Plus))
{
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.

}
err
}

Expand Down
11 changes: 11 additions & 0 deletions src/test/ui/parser/increment.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
fn test1() {
let i = 0;
i++; //~ ERROR
}

fn test2() {
let i = 0;
++i; //~ ERROR
}

fn main() {}
Comment on lines +8 to +11
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.

23 changes: 23 additions & 0 deletions src/test/ui/parser/increment.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
error: expected expression, found `+`
--> $DIR/increment.rs:3:7
|
LL | i++;
| ^ expected expression
|
= note: Rust has no dedicated increment and decrement operators
help: try using `+= 1` instead
|
LL | i += 1;
| ^^^^
Comment on lines +8 to +11
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 }.


error: expected expression, found `+`
--> $DIR/increment.rs:8:5
|
LL | ++i;
| ^ expected expression
|
= note: Rust has no dedicated increment and decrement operators
= help: try using `+= 1` instead

error: aborting due to 2 previous errors