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

Don't flatten blocks that have labels #5677

Merged
merged 2 commits into from Aug 13, 2023

Conversation

ytmimi
Copy link
Contributor

@ytmimi ytmimi commented Jan 26, 2023

Fixes #5676

This PR prevents rustfmt from flattening a match arm's body block if that block was labeled.

I wonder if it would be worth making this change a little smarter to allow flattening in cases where the label isn't used inside the block?

r? @calebcartwright

Previously we alwasy assumed the match arm pattern would have
`shape.width` - 5 characters of space to work with.

Now if we're formatting a block expression with a label we'll take the
label into account.
Comment on lines +252 to +253
// 7 = ` => : {`
let label_len = label.ident.as_str().len();
Copy link
Member

Choose a reason for hiding this comment

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

Does the identifier string include the ' prefix? If not we should account for that token in the width ourselves

Copy link
Contributor Author

@ytmimi ytmimi Jan 30, 2023

Choose a reason for hiding this comment

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

I double checked when I was writing the code and yes the label contains the '.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should future proof the code by adding a check for the leading ' in case that changes in the future

@calebcartwright
Copy link
Member

This is a problem that needs fixing, but also not the most urgent in the world give the ability to utilize the skip attribute on the match expression. I'd like to cross reference this against the style guide (if you'd be up for that it would be great), to both ensure that we're fully in adherence, and also see if there's any augmenting text needed to the guide

@calebcartwright
Copy link
Member

@ytmimi I'm perusing through some of the open PRs to find ones that seem like good (see: easy 😆) candidates for review and merge and this seems like one. Do you have any thoughts on my prior comment about checking the style guide? If unspecified in the guide then we can probably move ahead, and perhaps it's worth a PR to update the guide?

@ytmimi
Copy link
Contributor Author

ytmimi commented Aug 1, 2023

I tried checking the guide and I wasn't able to find anything about block labels. Does that mean we're good to move ahead with this?

@calebcartwright
Copy link
Member

I tried checking the guide and I wasn't able to find anything about block labels. Does that mean we're good to move ahead with this?

Thanks for checking. I suspect that's because the presence of this syntax was driven by the fairly recent RFC 2046, so yes, we should be good here

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.

lgtm!

@calebcartwright calebcartwright added release-notes Needs an associated changelog entry and removed pr-not-reviewed labels Aug 13, 2023
@calebcartwright calebcartwright merged commit e86c2ba into rust-lang:master Aug 13, 2023
@ytmimi ytmimi deleted the issue_5676 branch August 13, 2023 23:32
@ytmimi ytmimi removed the release-notes Needs an associated changelog entry label Oct 23, 2023
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.

Formatting labelled block expression in match arm causes invalid Rust code
2 participants