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

Fix missing struct field separators under certain conditions #5159

Merged
merged 3 commits into from
Mar 6, 2022

Conversation

cassaundra
Copy link
Contributor

@cassaundra cassaundra commented Dec 29, 2021

Fixes #4791 and #4928.

When struct_field_align_threshold is non-zero and trailing_comma is set to Never, struct field separators are omitted between field groups, resulting in invalid code. This issue is resolved by forcing separators between groups.

A test is included with a minimal reproducible example adapted from the original issue.

When struct_field_align_threshold is non-zero and trailing_comma is set to
"Never," struct field separators are omitted between field groups. This issue is
resolved by forcing separators between groups.

Fixes rust-lang#4791.

A test is included with a minimal reproducible example.
@calebcartwright
Copy link
Member

@ytmimi if/when you get a chance could you take a look at this? At some point we had a different fix for this that can be found over on the 2.0 branch, so would like to compare this approach vs. that one. I also feel like you may have possibly done some somewhat similar work recently and thought you may be interested in reviewing

@ytmimi
Copy link
Contributor

ytmimi commented Dec 30, 2021

Sure thing! I'll try to find the fix on the 2.0 branch to compare it to and will hopefully be able to get the review done in the next few days. I'll let you know if I run into any issues.

@ytmimi
Copy link
Contributor

ytmimi commented Jan 2, 2022

I managed to track down #4201, which I believe is the PR that fixes this issue on the 2.0 branch. Both fixes are very similar and involve passing additional information to rewrite_aligned_items_inner. The previous PR passes a slice of the remaining fields to rewrite_aligned_items_inner, while the new proposal just passes a boolean. I happen to prefer the newer solution since it's a little more flexible in case there are other scenarios that we need to force trailing separators.

I'm happy with the changes as they are! @cassaundra If you have time would it be possible to add test cases with trailing_comma=Always and test cases where struct_field_align_threshold=0. Since these are the two configuration options that you need to reproduce the issue it would be great if we had test cases that fully covered their interaction in this scenario.

@cassaundra
Copy link
Contributor Author

Thank you for the review! I will add those test cases soon.

@cassaundra
Copy link
Contributor Author

cassaundra commented Jan 15, 2022

Apologies for the late response.

I've added the tests as requested, hopefully providing better coverage between struct_field_align_threshold and trailing_comma. The tests are located in tests/{source,target}/issue-4791/ and are named as follows:

zero non-zero
always (default) trailing_comma.rs
never no_trailing_comma.rs buggy.rs

@ytmimi
Copy link
Contributor

ytmimi commented Jan 18, 2022

Thanks for adding those additional test cases! I think we now have the test coverage we need. @calebcartwright, is there anything else you'd like to see added? If not, I'm happy to move forward with these changes.

@calebcartwright
Copy link
Member

Changes LGTM! @cassaundra would you mind adding test files for #4928 too?

@cassaundra
Copy link
Contributor Author

Sorry again for the late reply! It's been a crazy month. Those tests have now been added.

@ytmimi
Copy link
Contributor

ytmimi commented Mar 6, 2022

@cassaundra Thanks for including that additional test!

Copy link
Member

@calebcartwright calebcartwright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comma deleted if followed by empty line
3 participants