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

Add tests for nested tuple access #4470

Closed
calebcartwright opened this issue Oct 14, 2020 · 11 comments
Closed

Add tests for nested tuple access #4470

calebcartwright opened this issue Oct 14, 2020 · 11 comments
Labels
good first issue Issues up for grabs, also good candidates for new rustfmt contributors hacktoberfest help wanted

Comments

@calebcartwright
Copy link
Member

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

@calebcartwright calebcartwright added good first issue Issues up for grabs, also good candidates for new rustfmt contributors help wanted hacktoberfest labels Oct 14, 2020
@V1shvesh
Copy link

Hey @calebcartwright! Can I pick this issue up?

@calebcartwright
Copy link
Member Author

Absolutely @V1shvesh! Let us know if you have any questions

@V1shvesh
Copy link

I've setup the project, going through the issues now 👍

@calebcartwright
Copy link
Member Author

@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

ChainItemKind::TupleField(ident, nested) => format!(
"{}.{}",
if nested { " " } else { "" },
rewrite_ident(context, ident)
),

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)

TupleField(symbol::Ident, bool),

@V1shvesh
Copy link

@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?

@calebcartwright
Copy link
Member Author

@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 tests/target/... with that content. That's really all there is to these tests, just a collection of snippets from a few different issues.

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

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 TupleField I linked to above to no longer insert that extra space

@jRimbault

This comment has been minimized.

@calebcartwright

This comment has been minimized.

@jRimbault

This comment has been minimized.

@sasurau4
Copy link
Contributor

sasurau4 commented Dec 1, 2020

I'll work on this!

@sasurau4
Copy link
Contributor

sasurau4 commented Dec 11, 2020

This issue was fixed by #4570, isn't it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues up for grabs, also good candidates for new rustfmt contributors hacktoberfest help wanted
Projects
None yet
Development

No branches or pull requests

4 participants