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

Bugfix/comment duplication #5913

Merged

Conversation

GambitingMan
Copy link
Contributor

Fixes #5871

The issue was in the rewrite_paren function.

Since the span on expressions doesn't include attributes and attributes comes before the expression, this caused the attribute to be included in pre_comment since the end of the comment span was subexpr.span.lo().
I changed the logic to use the span of the first attribute, if there are any attributes present. If not, default to the expression span.

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.

Thanks for the PR! You've definitely identified the root cause, as well as the proper underlying technique to address the issue.

However, this is such a common type of occurrence that we've got existing utilities that can and should be reused here instead. Details linked below, but basically using our span() instead of the AST struct's span field will cover the inline logic you originally proposed

rustfmt/src/spanned.rs

Lines 43 to 55 in da7f678

macro_rules! implement_spanned {
($this:ty) => {
impl Spanned for $this {
fn span(&self) -> Span {
span_with_attrs!(self)
}
}
};
}
// Implement `Spanned` for structs with `attrs` field.
implement_spanned!(ast::AssocItem);
implement_spanned!(ast::Expr);

src/expr.rs Outdated Show resolved Hide resolved
Co-authored-by: Caleb Cartwright <calebcartwright@users.noreply.github.com>
@GambitingMan
Copy link
Contributor Author

That did the trick 😄

Copy link
Contributor

@ytmimi ytmimi left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes @calebcartwright asked for. I think we're good to go here!

@calebcartwright
Copy link
Member

Thanks for fixing this and for your first contribution!

@calebcartwright calebcartwright merged commit a1fabbf into rust-lang:master Sep 20, 2023
27 checks passed
@GambitingMan GambitingMan deleted the bugfix/comment_duplication branch September 22, 2023 19:36
@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.

buggy formatting on expression attributes with comments
4 participants