Indentation of asssignment comments between lhs and rhs #5501

davidBar-On · 2022-08-14T07:52:27Z

Backport of suggested R2 changes for handling indentation of comments between assignment rhs and lhs, taken from different R2 PRs/commits. Not all changes were merged into R2, so this is why I didn't add "Backport" to the title.

Test cases are backport of PR #4734 test cases (which are enhancement of PR #4661 test cases).

The PR is based on:

Backport of R2 PR Comments between assignment (without let) lhs and rhs handling #4661 (merged into R2).
Backport of R2 PR Indentation of multiline comments between lhs and rhs #4734 (not merged into R2).
Changes based of backport of R2 changes to Rewrite for ast::Local done by this commit.
Implementation of the R1.x PR Backport PR #4658 and Fix issue #4551 - multilines comment indentation #5480 (was not merged yet) changes to light_rewrite_comment() which were backported from R2, as the test cases require these changes for proper formatting.

ytmimi · 2022-10-15T17:50:41Z

src/comment.rs

+/// that end on that line.
+fn is_first_comment_ends_on_first_line(s: &str) -> bool {
+    if s.starts_with("/*") {
+        s.lines().next().map_or(false, |l| l.contains("*/"))


should we use ends_with instead of contains?

Suggested change

s.lines().next().map_or(false, |l| l.contains("*/"))

s.lines().next().map_or(false, |l| l.ends_with("*/"))

Changed (although I am not sure whether the comment was deleted).
The main benefit of the change is that line that ends with /* comment 1*/ /* comment 2 is now properly handled. Therefore I also changed the function name and its description.

While evaluating the suggested change I found two issues that are worth noting:

The following code is not handled properly and therefore is not formatted:

var= /* Block comment 1*/ // Line comment 2 value;

Fixing this is quite complex. Since it is not expected to be used much in practice, I assume that for this PR it is o.k. not to handle this case, but I did add FIXME comment for it.

the following code (now added to the test cases):

var = /* Block comment */ value;

Is formatted into one line:

var = /* Block comment */ value;

However:

var = /* Block comment single line 1 */ /* Block comment single line 2*/ value;

is formatted as:

var = /* Block comment single line 1 */ /* Block comment single line 2*/ value;

I believe this is o.k., but I note that in case the formatted value in the first example should be in the next line.

ytmimi · 2022-10-15T17:51:32Z

src/comment.rs

@@ -225,7 +237,13 @@ pub(crate) fn combine_strs_with_missing_comments(
        Cow::from("")
    } else {
        let one_line_width = last_line_width(prev_str) + first_line_width(&missing_comment) + 1;
-        if prefer_same_line && one_line_width <= shape.width {
+        // First comment of a comments block can be in same line if ends in the first line
+        // (and meets other conditions)


can we enumerate what the other conditions are in the comment

Changed - removed the "(and meets other conditions)" as it was redundant. Adding the additional details could just be confusing as the "other conditions" are clear from the code.

src/comment.rs

src/expr.rs

src/items.rs

src/comment.rs

ytmimi · 2022-10-20T15:43:41Z

src/items.rs

+            let base_span = if let Some(ref ty) = self.ty {
+                mk_sp(ty.span.hi(), self.span.hi())
+            } else {
+                mk_sp(self.pat.span.hi(), self.span.hi())
+            };
+
+            let offset = context.snippet(base_span).find_uncommented("=")?;
+            let base_span_lo = base_span.lo();
+
+            let assign_lo = base_span_lo + BytePos(offset as u32);
+            let comment_start_pos = if let Some(ref ty) = self.ty {
+                ty.span.hi()
+            } else {
+                self.pat.span.hi()
+            };
+            let comment_before_assign = context.snippet(mk_sp(comment_start_pos, assign_lo)).trim();
+
+            let assign_hi = base_span_lo + BytePos((offset + 1) as u32);
+            let rhs_span_lo = init.span.lo();
+            let comment_end_pos = if init.attrs.is_empty() {
+                rhs_span_lo
+            } else {
+                let attr_span_lo = init.attrs.first().unwrap().span.lo();
+                // for the case using block
+                // ex. let x = { #![my_attr]do_something(); }
+                if rhs_span_lo < attr_span_lo {
+                    rhs_span_lo
+                } else {
+                    attr_span_lo
+                }
+            };
+
+            if !comment_before_assign.is_empty() {
+                let new_indent_str = &pat_shape
+                    .block_indent(0)
+                    .to_string_with_newline(context.config);
+                result = format!("{}{}{}", comment_before_assign, new_indent_str, result);
+            }
+


Took some time to look into this code. It seems like a lot of what's going on here is to recover comments before the =, e.g. let pat: ty /*comment*/ = or let pat /*comment*/ =. However, this PR and all the test cases you've outlined below only test for comments that come after the = e.g. let pat: ty = /*comment*/ expr.

For that reason I think we should only worry about the post = comments in this PR.

I also propose renaming init to expr since I think it's a little clearer that it's an ast::Expr node, and using init() instead of init_else_opt() since we don't use the _else and currently don't support ast::LocalKind::InitElse(..) variants.

if let Some(expr) = self.kind.init() { // Adjust the span after the `pat` and `ty` before searching for the `=` let base_span = if let Some(ref ty) = self.ty { mk_sp(ty.span.hi(), self.span.hi()) } else { mk_sp(self.pat.span.hi(), self.span.hi()) }; let comment_lo = context.snippet_provider.span_after(base_span, "="); let comment_span = mk_sp(comment_lo, expr.span.lo()); let nested_shape = shape.sub_width(1)?; result = rewrite_assign_rhs_with_comments( context, result, expr, nested_shape, &RhsAssignKind::Expr(&expr.kind, expr.span), RhsTactics::Default, comment_span, true, )?; }

... a lot of what's going on here is to recover comments before the =

I somehow missed that when I copied the R2 commit changes ☹️. I also checked now R2 and didn't find any test to the assign pre = comments. I therefore assume that maybe that commit was tested properly.

Changed the code per suggestion.

ytmimi

Thanks again for applying all the feedback, and for helping to backport those PRs! I think we're good to go here!

calebcartwright · 2023-02-04T17:17:21Z

I haven't had a chance to review this in detail, but from a cursory glance through the discussion and test cases my inclination is that this probably is the direction we want to go in long term.

However, it's got too big a risk of having a high blast radius for me to want to release it in the immediate future. I'd like to see this tested against some larger codebases, both to test/confirm that hypothesis/concern and see how it shakes out against a broader set of code.

Ultimately I think we'd need to version gate this no matter what, especially given the increased rigor around formatting stability given the pivot to style guide editions as the evolution mechanism

davidBar-On · 2023-02-07T15:00:36Z

Ultimately I think we'd need to version gate this no matter what ...

Added version gating.
Also rebased and consolidated all commits into one commit.

src/items.rs

tests/source/lhs-to-rhs-between-comments/assignment-multi-line-comments.rs

ytmimi · 2023-02-23T16:37:02Z

@davidBar-On thanks for making the requested changes. when you have a chance can you rebase your feature branch on master

davidBar-On · 2023-02-23T19:43:07Z

... can you rebase your feature branch on master

Rebase done.

ytmimi · 2023-02-23T19:58:45Z

@calebcartwright I just ran the Diff Check Job, and with the version gate these changes don't produce any changes 👍🏼

ytmimi added the pr-not-reviewed label Aug 15, 2022

ytmimi reviewed Oct 15, 2022

View reviewed changes

src/comment.rs Outdated Show resolved Hide resolved

ytmimi reviewed Oct 15, 2022

View reviewed changes

src/expr.rs Outdated Show resolved Hide resolved

ytmimi reviewed Oct 18, 2022

View reviewed changes

src/items.rs Show resolved Hide resolved

ytmimi added pr-waiting-on-author and removed pr-not-reviewed labels Oct 18, 2022

ytmimi reviewed Oct 18, 2022

View reviewed changes

src/comment.rs Outdated Show resolved Hide resolved

ytmimi reviewed Oct 20, 2022

View reviewed changes

ytmimi approved these changes Oct 27, 2022

View reviewed changes

ytmimi added pr-ready-to-merge and removed pr-waiting-on-author labels Oct 27, 2022

davidBar-On force-pushed the issues-4661-4734-part5480-fromf897e25-Indentation-of-assignment-comments-between-lhs-and-rhs branch from c0c3b3e to d1da537 Compare February 7, 2023 14:45

ytmimi reviewed Feb 13, 2023

View reviewed changes

src/items.rs Outdated Show resolved Hide resolved

ytmimi reviewed Feb 20, 2023

View reviewed changes

src/items.rs Outdated Show resolved Hide resolved

ytmimi reviewed Feb 20, 2023

View reviewed changes

tests/source/lhs-to-rhs-between-comments/assignment-multi-line-comments.rs Show resolved Hide resolved

davidBar-On added 3 commits February 23, 2023 21:31

Version gating along with rebase and commits consolidation

74a29cf

Changes per revierwr comments

0dcaf0f

Changes per reviewer comments

81849b9

davidBar-On force-pushed the issues-4661-4734-part5480-fromf897e25-Indentation-of-assignment-comments-between-lhs-and-rhs branch from 1aa6ae0 to 81849b9 Compare February 23, 2023 19:37

ytmimi mentioned this pull request Aug 16, 2024

Issue6246-Let statement should rewrite rhs with comments after equal sign #6282

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Indentation of asssignment comments between lhs and rhs #5501

Indentation of asssignment comments between lhs and rhs #5501

davidBar-On commented Aug 14, 2022

ytmimi Oct 15, 2022

davidBar-On Oct 18, 2022

ytmimi Oct 15, 2022

davidBar-On Oct 18, 2022

ytmimi Oct 20, 2022 •

edited

Loading

davidBar-On Oct 21, 2022

ytmimi left a comment

calebcartwright commented Feb 4, 2023

davidBar-On commented Feb 7, 2023

ytmimi commented Feb 23, 2023

davidBar-On commented Feb 23, 2023

ytmimi commented Feb 23, 2023

	s.lines().next().map_or(false, \|l\| l.contains("*/"))
	s.lines().next().map_or(false, \|l\| l.ends_with("*/"))

Indentation of asssignment comments between lhs and rhs #5501

Are you sure you want to change the base?

Indentation of asssignment comments between lhs and rhs #5501

Conversation

davidBar-On commented Aug 14, 2022

ytmimi Oct 15, 2022

Choose a reason for hiding this comment

davidBar-On Oct 18, 2022

Choose a reason for hiding this comment

ytmimi Oct 15, 2022

Choose a reason for hiding this comment

davidBar-On Oct 18, 2022

Choose a reason for hiding this comment

ytmimi Oct 20, 2022 • edited Loading

Choose a reason for hiding this comment

davidBar-On Oct 21, 2022

Choose a reason for hiding this comment

ytmimi left a comment

Choose a reason for hiding this comment

calebcartwright commented Feb 4, 2023

davidBar-On commented Feb 7, 2023

ytmimi commented Feb 23, 2023

davidBar-On commented Feb 23, 2023

ytmimi commented Feb 23, 2023

ytmimi Oct 20, 2022 •

edited

Loading