Handle comments between Trait Generics and Bounds #4666

davidBar-On · 2021-01-22T11:55:07Z

Add handling of comments between Trait Generics and Bounds by using rewrite_assign_rhs_with_comments() (one of the cases identified in #4626 discussion).

Also added workaround for an issue that a comment before the Ident caused code duplication. The workaround is to use the original code in this case.

The PR also partly enhance the resolution of #2055.

calebcartwright · 2021-03-02T04:11:38Z

src/formatting/items.rs

        let pre_block_span = if !generics.where_clause.predicates.is_empty() {
            mk_sp(generics.where_clause.span.hi(), item.span.hi())
+        } else if !generic_bounds.is_empty() {
+            mk_sp(generic_bounds.last().unwrap().span().hi(), item.span.hi())


This would conflict with #4711. I'm not sure there's a real case where the generic_bounds would be empty while the collection on generics would not (or vice versa), but either way, I'd think we could just use the end of where_clause span because I believe the parser advances this to desired position in the presence of any generics regardless.

Probably worth checking to confirm though.

Side note, if the span differences necessitate keeping these checks and separate calcs, that instead of checking is_empty and then having to call last and unwrap, an else if let Some(last) = generic_bounds.last() { could be used to express it a bit more succinctly

... I believe the parser advances this to desired position in the presence of any generics regardless

The item.span is not advanced, but found that a WhereClause is always available, even when a where clause is not available in the code. Therefore, removed all if conditions and changed to use generics.where_clause.span.hi() unconditionally. (Also rebased.)

The item.span is not advanced, but found that a WhereClause is always available

Yes I am aware.

The spans on the AST nodes we're working with are immutable for practical purposes, and the span of the item will always cover the portions it's supposed to cover.

I was referring to the process within the parser that sets those spans, and what it ends up setting as the span for the where_clause in the various cases of source input (no generics at all, generic params with no where clause, where clause with no predicates, and where clause with predicates).

I was just saying that I wasn't positive off hand where the span position of the where_clause is relative to the broader item span in those various scenarios, and that if we're going to use it unconditionally then we should validate to make sure that span will always have the hi end be placed sufficiently far enough so that we can be sure we'd avoid speciously double counting comments which had actually been handled already.

calebcartwright

Left some additional comments inline

calebcartwright · 2021-03-12T02:13:54Z

src/formatting/items.rs

+        if contains_comment(context.snippet(mk_sp(item.span.lo(), generics.span.lo()))) {
+            return None;
+        }
+


Nit: if we're going to have an early bail (which is a good idea btw!) then i think we should adjust this a bit.

The inline comment hi end is the beginning of generics, not the ident so if we can handle any comments between the ident and wherever the generics begin then let's reduce the end of this span we're using for the early bail.

Additionally, we can shift this check up front, as there's really no need for the allocation and formatting of the visibility and friends if we're going to have to bail out down here anyway

calebcartwright · 2021-03-12T02:27:27Z

src/formatting/items.rs

+        // FIXME(#2055): rustfmt fails to format when there are comments within trait bounds.
        if !generic_bounds.is_empty() {
-            let ident_hi = context
-                .snippet_provider
-                .span_after(item.span, &item.ident.as_str());
+            let bound_lo = generic_bounds.first().unwrap().span().lo();
            let bound_hi = generic_bounds.last().unwrap().span().hi();
-            let snippet = context.snippet(mk_sp(ident_hi, bound_hi));
+            let snippet = context.snippet(mk_sp(bound_lo, bound_hi));
            if contains_comment(snippet) {
                return None;
            }


I'm not sure if this is true/needed anymore given #4474 (and the fix/adjustment in #4653). I think that currently (with the inclusion of the correction to the pre-block span derivation) we can already handle any and all comments to the right of the colon following the ident.

I think the remaining gaps are really any such comments to the left

It seems that you are correct and comments whiting that generics bounds are handled properly. Before I make the change, there is one change to 2015 edition test case output that I want to make sure is acceptable. The following code:

rustfmt/tests/source/trait_2015_edition.rs

Lines 65 to 70 in 0e90855

pub trait Foo:

// A and C

A + C

// and B

+ B

{}

Will now be formatted as (same as for the other editions):

pub trait Foo: // A and C A + C // and B + B { }

Is this o.k. or the test for comments within the generics bound should still be done when edition is set to 2015?

The edition really only drives parsing, and shouldn't have any impact on formatting. I wonder if there's a separate bug in there 🤔

What happens if you remove that leading // A and C comment?

What happens if you remove that leading // A and C comment?

You are right that removing the // A and C comment changes the indentation to:

pub trait Foo: A + C // and B + B { }

I wonder if there's a separate bug in there.

There seem to be a general a problem in rewrite_assign_rhs_with_comments(). The same issue is seen in the following assignments, where an extra indentation is added because of a comment the the 2nd and following lines of the expression (the long names are because adding the // and B comment into the expressions cause using the original code):

fn main () { x = Aaaaaaaaaaaaaaaa + Cccccccccccccccccccccccccccc + Bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ; y = /* A and C */ Aaaaaaaaaaaaaaaa + Cccccccccccccccccccccccccccc + Bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ; z = /* A and C */ Aaaaaaaaaaaaaaaa + Cccccccccccccccccccccccccccc + Bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb ; }

is formatted as:

fn main() { x = Aaaaaaaaaaaaaaaa + Cccccccccccccccccccccccccccc + Bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb; y = /* A and C */ Aaaaaaaaaaaaaaaa + Cccccccccccccccccccccccccccc + Bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb; z = /* A and C */ Aaaaaaaaaaaaaaaa + Cccccccccccccccccccccccccccc + Bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb; }

The problem is caused by the following shape adjustment that should probably be done only if the formatted comment is not immediately followed by newline:

rustfmt/src/formatting/expr.rs

Lines 1998 to 2002 in e10d2d7

let shape = if contains_comment {

shape.block_left(context.config.tab_spaces())?

} else {

shape

};

Should I try to fix this issue in this PR, or for now the test case expected target should just be changed? (Note that the test case output will be changed in any case, since now it is using the original code and with this PR the code is formatted.)

The same issue is seen in the following assignments, where an extra indentation is added because of a comment the the 2nd and following lines of the expression

Well, not quite. The former are bounds on a trait whereas your example are binop expressions, and these are governed by different formatting rules.

https://github.com/rust-dev-tools/fmt-rfcs/blob/master/guide/items.md#traits vs https://github.com/rust-dev-tools/fmt-rfcs/blob/master/guide/expressions.md#binary-operations

As such the only issue/non-compliance with the Style Guide is on the traits, so I think you'll want to try to address that in the same PR since both that fix and the originally discussed changes here would be needed in combination (neither would be mergeable in isolation)

The former are bounds on a trait whereas your example are binop expressions, and these are governed by different formatting rules.

I don't see a reference in the guide to the formatting output I was referring to, so I will try to clarify what I meant. I expected that the y assignment in the example above will be similar to the x assignment and to the formatting example in the guide:

y = /* A and C */ Aaaaaaaaaaaaaaaa + Cccccccccccccccccccccccccccc + Bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb;

However, because of the comment the 3rd and 4th lines are formatted with additional indentation:

y = /* A and C */ Aaaaaaaaaaaaaaaa + Cccccccccccccccccccccccccccc + Bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb;

Is this the expected output?

I expected that the y assignment in the example above will be similar to the x assignment and to the formatting example in the guide

What I'm saying is that you've changed to the (off)topic discussion of binary expressions which is not relevant for the formatting of trait bounds, and additionally that your expectation about binary expression formatting is incorrect.

From the binary expression section of the guide (emphasis mine):

If line-breaking, put the operator on a new line and block indent. Put each sub-expression on its own line

The block indents in the binary expressions are happening because they are supposed to be there, the only impact the comment between the assignment operator and the rhs is the placement of the first operand. If another condition (such as a comment) results in the opening of the binary expression not being able to be placed on the same line as the lhs/assignment operator, then that first operand of the bin expr has to be block indented itself, but the other subexpr(s) still have to be block indented relative to that first operand.

It is merely a coincidence in your first snippet that the first operand happens to be in the same column position as the block indentation, due to the single character variable name (x + = + two spaces). If you were to use a longer var name, have a let, etc. you will see that the alignment is different:

e.g. try formatting these:

fn main() { let x = Aaaaaaaaaaaaaaaa + Cccccccccccccccccccccccccccc + Bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb; foo_bar = Aaaaaaaaaaaaaaaa + Cccccccccccccccccccccccccccc + Bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb; }

Hopefully that helps, but if you have additional thoughts/questions/doubts about the binary expression formatting, then I'd encourage opening a separate issue to avoid taking this thread any farther away from the issue at hand, namely the extra, incorrect additional block indent that's added to the trait bounds when there's a comment preceding the first bound.

Submitted changes which I hope satisfy the comments requirements.

Hopefully that helps, but if you have additional thoughts/questions/doubts about the binary expression formatting, then I'd encourage opening a separate issue

Yes, I understand now. Thanks for the detailed explanation. I do have some minor comments, but its not worth further discussion, especially considering backward compatibility.

What I'm saying is that you've changed to the (off)topic discussion of binary expressions which is not relevant for the formatting of trait bounds

I know it was (of)topic but I thought that a common change for both traits and binop is required. I now added a separate function to handle trait rhs with comments.

calebcartwright · 2021-03-12T02:31:59Z

tests/source/lhs-to-rhs-between-comments/trait.rs

@@ -0,0 +1,70 @@
+// Based on issue #2055:


Given the updates to the pre block span, we'd want to have tests here that included a mix of some other trait cases, including some with where clauses

we'd want to have tests here that included a mix of some other trait cases, including some with where clauses

Added some test cases mainly with where clause.

calebcartwright · 2021-03-25T22:31:15Z

src/formatting/expr.rs

+pub(crate) fn rewrite_trait_rhs_with_comments<S: Into<String>, R: Rewrite>(
+    context: &RewriteContext<'_>,
+    lhs: S,
+    ex: &R,
+    shape: Shape,
+    rhs_tactics: RhsTactics,
+    between_span: Span,
+    allow_extend: bool,
+) -> Option<String> {
+    let lhs = lhs.into();
+    let contains_comment = contains_comment(context.snippet(between_span));
+
+    let rhs = rewrite_rhs_expr(context, &lhs, ex, shape, rhs_tactics, ":")?;
+
+    if contains_comment {
+        let rhs = rhs.trim_start();
+        combine_strs_with_missing_comments(
+            context,
+            &lhs,
+            &rhs,
+            between_span,
+            shape.block_left(context.config.tab_spaces())?,
+            allow_extend,
+        )
+    } else {
+        Some(lhs + &rhs)
+    }
+}
+


I think the overall idea here is good, but think we're doing this in the wrong place in the codebase. Traits are by definition Items, not Expressions, and by design (encapsulation, etc.) we'd only ever need to call this from one place: the corresponding formatting flow for traits in items.rs.

As such, this function feels quite out of place here in expr.rs and the logic would be best placed back in items.

Additionally (regarding changes above this one), and a bit of a nit, it's quite reasonable to have one of these rewrite functions allow the caller to specify the separator, but that doesn't mean we can't also have a helper function that avoids the duplicative calls that each have to specify the assignment operator as the separator (which would be introduced here)

As such, this function feels quite out of place here in expr.rs and the logic would be best placed back in items.

Moved rewrite_trait_rhs_with_comments to items.rs.

... have a helper function that avoids the duplicative calls that each have to specify the assignment operator as the separator

Added a helper function rewrite_assign_rhs_expr that specifies the '=' separator.

calebcartwright · 2021-04-02T02:40:18Z

Thanks for the updates! I may have a very minor nit or two as a final review pass, but I think we're otherwise in good shape.

I want to think on this a bit longer to make sure we're not overlooking any scenarios. In cases like these it's possible that folks may feel like after upgrading rustfmt that it is "changed" its formatting, but really rustfmt is just finally able to format these trait/comment scenarios. It's definitely a fix we want/need, but it's one we need to be sure we've got right/fully covered up front (because we'll be limited in our ability to change it afterwards)

davidBar-On mentioned this pull request Jan 22, 2021

trait bounds with comments are not formatted #2055

Open

calebcartwright added the pr-not-reviewed label Jan 23, 2021

davidBar-On force-pushed the lhs-to-rhs-bewteen-comments-trait branch from ea54ac7 to 8fa9c8c Compare January 28, 2021 15:57

calebcartwright reviewed Mar 2, 2021

View reviewed changes

calebcartwright added pr-waiting-on-author and removed pr-not-reviewed labels Mar 2, 2021

davidBar-On force-pushed the lhs-to-rhs-bewteen-comments-trait branch from 8fa9c8c to af74fbb Compare March 2, 2021 11:04

calebcartwright added pr-follow-up-review-pending and removed pr-waiting-on-author labels Mar 6, 2021

calebcartwright reviewed Mar 12, 2021

View reviewed changes

calebcartwright added pr-waiting-on-author and removed pr-follow-up-review-pending labels Mar 12, 2021

calebcartwright mentioned this pull request Mar 12, 2021

Fix corrupted comments in trait generics #4711

Open

davidBar-On added 3 commits March 19, 2021 10:06

Handle comments between trait generics and bounds

b63aeea

Changes per comments and moved test cases files(1)

bceaf18

Changes per comments and with added test cases

cc0daad

davidBar-On force-pushed the lhs-to-rhs-bewteen-comments-trait branch from af74fbb to cc0daad Compare March 22, 2021 07:37

calebcartwright reviewed Mar 25, 2021

View reviewed changes

davidBar-On force-pushed the lhs-to-rhs-bewteen-comments-trait branch from cf551a3 to 2dda3da Compare March 30, 2021 09:19

Changes per review comments (v3)

e4ab01e

davidBar-On force-pushed the lhs-to-rhs-bewteen-comments-trait branch from 2dda3da to e4ab01e Compare March 30, 2021 09:35

calebcartwright removed the pr-waiting-on-author label Apr 2, 2021

calebcartwright changed the base branch from master to rustfmt-2.0.0-rc.2 April 30, 2021 00:23

calebcartwright mentioned this pull request Sep 27, 2021

Add "type_separator" option to control placement of "+" in types #5004

Open

ulyssa mentioned this pull request Nov 2, 2021

Support comments in trait bounds #5059

Open

ytmimi added pr-targeting-2.0 This PR is targeting the 2.0 branch p-low labels Jul 27, 2022

calebcartwright mentioned this pull request Jun 18, 2023

Rustfmt ate a single line comment in a long list of trait bounds #3669

Open

sffc mentioned this pull request Jan 31, 2024

Rustfmt deletes comments in first position of trait bound #6051

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle comments between Trait Generics and Bounds #4666

Handle comments between Trait Generics and Bounds #4666

davidBar-On commented Jan 22, 2021 •

edited

Loading

calebcartwright Mar 2, 2021

davidBar-On Mar 2, 2021

calebcartwright Mar 12, 2021

calebcartwright left a comment

calebcartwright Mar 12, 2021

calebcartwright Mar 12, 2021

davidBar-On Mar 15, 2021

calebcartwright Mar 19, 2021

davidBar-On Mar 19, 2021

calebcartwright Mar 20, 2021

davidBar-On Mar 20, 2021

calebcartwright Mar 21, 2021

davidBar-On Mar 22, 2021

calebcartwright Mar 12, 2021

davidBar-On Mar 22, 2021

calebcartwright Mar 25, 2021

davidBar-On Mar 27, 2021

calebcartwright commented Apr 2, 2021

	let shape = if contains_comment {
	shape.block_left(context.config.tab_spaces())?
	} else {
	shape
	};

Handle comments between Trait Generics and Bounds #4666

Are you sure you want to change the base?

Handle comments between Trait Generics and Bounds #4666

Conversation

davidBar-On commented Jan 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebcartwright left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calebcartwright commented Apr 2, 2021

davidBar-On commented Jan 22, 2021 •

edited

Loading