-
Notifications
You must be signed in to change notification settings - Fork 901
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
Add tests for nested tuple access #4470
Comments
Hey @calebcartwright! Can I pick this issue up? |
Absolutely @V1shvesh! Let us know if you have any questions |
I've setup the project, going through the issues now 👍 |
@V1shvesh - apologies as there's some additional context here that'll impact the tests. Until recently nested tuple access that didn't have some kind of separator or token before latter items would result in a parser error as they would be parsed as a float. As such rustfmt has historically put a space before the ident to prevent said parsing error. Since that's no longer the case, we no longer need to conditionally insert that space rustfmt/src/formatting/chains.rs Lines 203 to 207 in 2d6a968
I think the rustc parser improvements will obviate the need for us to track whether a the tuple fields are nested now as well, if you are interested in working on that and doing a bit more work than the issue description originally covered (though simply removing the conditional space insertion above should suffice for tests) rustfmt/src/formatting/chains.rs Line 122 in 2d6a968
|
@calebcartwright so far I just tried to read up and get more context on the test cases. Can you clarify if the test cases are still required? |
@V1shvesh - Yes, the test cases are absolutely needed and they are the main point of this issue. All you really need to do for test cases is to create files with snippets of code (you can think of these as being like test fixtures), and the linked issues in the description contain the actual snippets you would stick in these files. For example, one such snippet comes from #4355 fn main() {
let _ = ((1,),).0.0;
} As detailed in, https://github.com/rust-lang/rustfmt/blob/master/Contributing.md#creating-test-cases you can just create a file under What I was referring to in my follow up is that rustfmt will currently add an extra space in there for historical reasons. So without making any changes to rustfmt, it would convert the above snippet this (notice the space before the trailing fn main() {
let _ = ((1,),).0 .0;
} This used to be necessary for rustfmt to do so that the emitted formatting didn't cause any parser/compilation, however, it is no longer necessary to do so. As such, in addition to adding the test case files, you'll also want to update the formatting of the |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
I'll work on this! |
This issue was fixed by #4570, isn't it? |
There have been various reports of the same underlying parsing issue with nested tuple access over the last few weeks. rust-lang/rust#77774 in rustc addressed the underlying issue which was then incorporated within rustfmt via #4469
However, would still be good to add some regression test cases for all the reports by adding corresponding files under the
tests/target
directory (no source files needed for this)https://github.com/rust-lang/rustfmt/blob/master/Contributing.md#creating-test-cases
For anyone interested in working on this, please add tests for all the reported cases (including those added in issue comments)
#4355
#4410
#4363
rust-lang/rust#76449
The text was updated successfully, but these errors were encountered: