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 #88672

Merged
merged 16 commits into from
Apr 3, 2022
Merged

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

merged 16 commits into from
Apr 3, 2022

Conversation

camelid
Copy link
Member

@camelid camelid commented Sep 5, 2021 •
edited
Loading

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

This is a continuation of #83536.

r? @estebank

@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 Sep 5, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 5, 2021
@camelid camelid added the A-parser Area: The parsing of Rust source code to an AST label Sep 5, 2021
@camelid 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 6, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
camelid
camelid commented Sep 13, 2021
View reviewed changes
compiler/rustc_parse/src/parser/diagnostics.rs Outdated Show resolved Hide resolved
compiler/rustc_parse/src/parser/expr.rs Outdated Show resolved Hide resolved
src/test/ui/parser/increment.stderr Outdated Show resolved Hide resolved
compiler/rustc_parse/src/parser/diagnostics.rs Outdated Show resolved Hide resolved
compiler/rustc_parse/src/parser/diagnostics.rs Outdated Show resolved Hide resolved
@camelid
Copy link
Member Author

camelid commented Sep 13, 2021

Is there any way this could accidentally break macros that have i++–style syntax? I tested it a bit, and I didn't find any issues, but I thought it'd be worth bringing up.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid
Copy link
Member Author

camelid commented Sep 13, 2021

(CI is just failing because I used // TODO, which was intentional so they would be resolved before this is merged.)

@apiraino apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 13, 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 Oct 3, 2021
@joelpalmer joelpalmer 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 Oct 19, 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 Nov 8, 2021
@pnkfelix
Copy link
Member

Hi @camelid ; we visited this for triage.

It looks like you had some questions you wanted answered, but also, @estebank had a suggestion for a change they wanted to see.

What is the status on those fronts? Do you think you are stuck waiting for more feedback from @estebank, or can you make forward progress on the suggestion they provided in the meantime?

@camelid
Copy link
Member Author

camelid commented Nov 19, 2021 •
edited
Loading

It's been a while, but my recollection is that I implemented most of the user-facing side of what estebank suggested, but in a different way in the parser because I got stuck with the way estebank suggested. The way the change is implemented currently works pretty well, though sometimes the suggestions aren't as good as they could be. Perhaps it would be good to accept the current state of output as good enough for an "MVP PR" since it can be improved further in the future. That's of course assuming there aren't any major issues in the output.

So, in summary: I think I need estebank to see if the impl looks okay and the error output looks good enough for a first implementation.

@camelid
Copy link
Member Author

camelid commented Dec 10, 2021

@estebank could you take another look at this when you get a chance?

@bors
Copy link
Contributor

bors commented Dec 15, 2021

☔ The latest upstream changes (presumably #91933) made this pull request unmergeable. Please resolve the merge conflicts.

@camelid
Copy link
Member Author

camelid commented Dec 15, 2021

I'll wait to rebase until this is reviewed in case it needs more rebases.

@camelid
Copy link
Member Author

camelid commented Jan 25, 2022

r? @davidtwco

camelid added 10 commits March 23, 2022 22:31
@camelid
Emit both subexp and standalone sugg for postfix
62b8ea6 
This solves the TODO.
@camelid
Improve handling of tmp variable name conflicts
073010d
@camelid
Bless tests
713a11b
@camelid
Improve function names
29a5c36
@camelid
Add heuristic to avoid treating x + +2 as increment
4212835
@camelid
Make standalone an enum
95960b7
@camelid
Change temporary variable name if it would conflict
ef74796
@camelid
Use multipart_suggestions
725cde4 
This records that the suggestions are mutually-exclusive (i.e., only one
should be applied).
@camelid
Fix rustfix panic on test
86220d6 
`run-rustfix` applies all suggestions regardless of their Applicability.
There's a flag, `rustfix-only-machine-applicable`, that does what it
says, but then the produced `.fixed` file would have invalid code from
the suggestions that weren't applied. So, I moved the cases of postfix
increment, in which case multiple suggestions are given, to the
`-notfixed` test, which does not run rustfix.

I also changed the Applicability to Unspecified since MaybeIncorrect
requires that the code be valid, even if it's incorrect.
@camelid
Fix from rebase
4943688 
I changed the test functions to be `pub` rather than called from a
`main` function too, for easier future modification of tests.
@camelid
Copy link
Member Author

camelid commented Mar 27, 2022

Ok, hopefully CI should pass now.

@davidtwco
Copy link
Member

This is great, thanks for seeing this through. Could you squash some of the commits? r=me after that.

@davidtwco davidtwco 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 1, 2022
@camelid
Copy link
Member Author

camelid commented Apr 2, 2022

@davidtwco It's hard to squash without squashing all of them into one commit, and I'd rather keep the history to make it easier to understand the design choices for the code (e.g., when git blameing). Is it okay to leave it unsquashed?

@davidtwco
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Apr 3, 2022

📌 Commit 4943688 has been approved by davidtwco

@bors 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 3, 2022
@bors
Copy link
Contributor

bors commented Apr 3, 2022

⌛ Testing commit 4943688 with merge 133859d...

@bors
Copy link
Contributor

bors commented Apr 3, 2022

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 133859d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 3, 2022
@bors bors merged commit 133859d into rust-lang:master Apr 3, 2022
@rustbot rustbot added this to the 1.61.0 milestone Apr 3, 2022
@bors bors mentioned this pull request Apr 3, 2022
Merged
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (133859d): comparison url.

Summary:

  • Primary benchmarks: no relevant changes found
  • Secondary benchmarks: no relevant changes found. 2 results were found to be statistically significant but too small to be relevant.
Regressions 😿
(primary)		 Regressions 😿
(secondary)		 Improvements 🎉
(primary)		 Improvements 🎉
(secondary)		 All 😿 🎉
(primary)
count1 0 2 0 0 0
mean2 N/A 1.4% N/A N/A N/A
max N/A 2.6% N/A N/A N/A

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

@rustbot label: -perf-regression

Footnotes

  1. number of relevant changes ↩

  2. the arithmetic mean of the percent change ↩

@camelid camelid deleted the inc-parser-sugg branch April 3, 2022 22:16
@camelid
Copy link
Member Author

camelid commented Apr 3, 2022

Thank you so much for helping me with this PR and being patient even though it took me forever to finish it! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@b-naber b-naber b-naber left review comments

@estebank estebank estebank left review comments

@davidtwco davidtwco davidtwco approved these changes

Assignees

@davidtwco davidtwco

Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The parsing of Rust source code to an AST A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.61.0
Development

Successfully merging this pull request may close these issues.

Add diagnostic for ++ and -- usages