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 Comment Preservation in PatKind::Paren Patterns #6111

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

yuvraj-wale
Copy link

@yuvraj-wale yuvraj-wale commented Mar 11, 2024

Fixes #6110

Description:

This pull request resolves an issue where rustfmt was removing comments inside parentheses in PatKind::Paren patterns. The fix ensures that comments are preserved during formatting, maintaining the original code structure and readability.

Changes:

  • Modified the formatting logic for PatKind::Paren to correctly handle comments within parentheses.
  • Added tests to verify that comments are preserved in PatKind::Paren patterns.

@yuvraj-wale yuvraj-wale marked this pull request as draft March 11, 2024 17:54
@yuvraj-wale yuvraj-wale changed the title [Draft] Fix Comment Preservation in PatKind::Paren Patterns Fix Comment Preservation in PatKind::Paren Patterns Mar 11, 2024
@yuvraj-wale yuvraj-wale changed the title Fix Comment Preservation in PatKind::Paren Patterns [Draft] Fix Comment Preservation in PatKind::Paren Patterns Mar 11, 2024
@ytmimi
Copy link
Contributor

ytmimi commented Mar 12, 2024

@yuvraj-wale unfortunately we can't accept these changes as is since they modify the formatting of already formatted code. rustfmt has strong guarantees about not introducing stable formatting changes so you'll likely need to come up with a different approach to solve this one.

One idea you could try is to manually write the comment recovery code.

We might be able to move forward with these changes if they were version gated, though I'd want some clarify from the style team on whether or not the new formatting is what's expected based on the Rust Style Guide

@yuvraj-wale
Copy link
Author

Thanks for the feedback! I will look into recovering comments manually and try to update the PR.

@ytmimi
Copy link
Contributor

ytmimi commented Mar 12, 2024

Great. Let me know if you need any pointers on how to get started

@yuvraj-wale
Copy link
Author

Hey @ytmimi,
I've been digging into this issue for a while now, and I've successfully preserved comments in PatKind::Paren patterns. However, I'm encountering difficulties with their formatting, particularly regarding newlines and whitespaces. Could you provide some guidance on how to apply proper formatting? I suspect it involves selecting the appropriate tactics and utilizing ListFormatting for effective formatting if I am not wrong.

@ytmimi
Copy link
Contributor

ytmimi commented Mar 14, 2024

I'm wondering if overflow::rewrite_with_parens is the right thing to call here since it seems to be the source of a lot of the issues you're running into. You might only want to call it after conditionally determining that there are comments before or after the inner pattern.

Another approach to preserve comments within the PatKind::Paren would be to return the contents of the Span unformatted with Some(context.snippet(span).to_owned()). This wouldn't apply any formatting, and would therefore keep the comments.

@yuvraj-wale
Copy link
Author

@ytmimi
Ah, I see what you mean about modifying the formatting of already formatted code. I was heading in a different direction initially. What I've done now is to extract the pre and post snippets around the inner pattern, which is formatted as usual according to the original implementation. By combining these snippets with the inner pattern, the comments are preserved while the formatting is still applied. Do you think this is the right approach and implementation? I apologize if I've misunderstood anything again. Thanks!

@ytmimi
Copy link
Contributor

ytmimi commented Mar 15, 2024

@yuvraj-wale possibly. I think you'll need to add test cases to this PR to know for sure.

Comment on lines +276 to +277
let pre_snippet = context.snippet(mk_sp(self.span.lo(), pat.span.lo()));
let post_snippet = context.snippet(mk_sp(pat.span.hi(), self.span.hi()));
Copy link
Contributor

Choose a reason for hiding this comment

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

When I was suggesting we return context.snippet(span) I was thinking more along the lines of the following approach where we don't apply any formatting, not even to the inner pattern.

let pre_snippet = context.snippet(self.span.lo().between(pat.span.lo()));
let post_snippet = context.snippet(pat.span.lo().between(self.span.hi()));

if contains_comment(pre_snippet) || contains_comment(post_snippet) {
    return Some(context.snippet(self.span).to_owned())
}

Your approach may also work though! As I mentioned I'd like you to add some test cases to better prove this out.

Copy link
Author

Choose a reason for hiding this comment

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

Hey I've added some test cases, let me know if there's anything specific you'd like to see.

@yuvraj-wale yuvraj-wale changed the title [Draft] Fix Comment Preservation in PatKind::Paren Patterns Fix Comment Preservation in PatKind::Paren Patterns Mar 16, 2024
@yuvraj-wale yuvraj-wale marked this pull request as ready for review March 16, 2024 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustfmt removes comments inside parentheses in patterns (PatKind::Paren).
3 participants