Rustfmt: Add tests for nested tuple access

Created on 14 Oct 2020  路  11Comments  路  Source: rust-lang/rustfmt

There have been various reports of the same underlying parsing issue with nested tuple access over the last few weeks. https://github.com/rust-lang/rust/pull/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

https://github.com/rust-lang/rust/issues/76449

good first issue hacktoberfest help wanted

All 11 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

https://github.com/rust-lang/rustfmt/blob/2d6a968d52b13b75fc3799f50020a34efd480a28/src/formatting/chains.rs#L203-L207

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)

https://github.com/rust-lang/rustfmt/blob/2d6a968d52b13b75fc3799f50020a34efd480a28/src/formatting/chains.rs#L122

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

Hi, rustfmt panicked here.

On Windows it just panicked without providing much context, on my linux box it provided more details, but in the GitHub CI it panicked the same way Windows did.

Thank you for reaching out @jRimbault, but you're reporting a duplicate of several previous issues that have already been fixed (#4355)

Ah sorry, I didn't find this one (I guess because github default to searching only open issues).

I'll work on this!

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

Was this page helpful?
0 / 5 - 0 ratings