-
Notifications
You must be signed in to change notification settings - Fork 908
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
BinOp Comments Handling #4417
BinOp Comments Handling #4417
Conversation
Thank you for the PR @davidBar-On! There's a lot going on here so will need some time to digest and review |
@calebcartwright I hope that whatever is going on is because of good reasons. |
I just meant there's a pretty large diff in this PR (admittedly driven by tests) and wanted to let you know that it'll probably be a little while before someone has time to go through it all 😄 |
I forget the triggering circumstances, but let's make sure we cover comment recovery in the fallback case too. Still digesting and processing the rest of the changes rustfmt/src/formatting/expr.rs Lines 98 to 104 in aa8e601
|
@calebcartwright Sorry, am not familiar enough with rust and don't understand what you mean by fallback case or what are the required changes ... Can you please clarify? Also, I found that my changes don't handle well one-line comments. Should I submit the fix now or wait until you finish the review? |
No worries! If you take a look at the binary expression handling code (larger snippet linked below), you'll see that there's I'm not sure if this will ever actually get hit in practice (hence my reference to it as a "fallback case"), but we should make sure we update this rustfmt/src/formatting/expr.rs Lines 95 to 107 in aa8e601
Please go ahead and make your changes, will be easier to review once you think it's in a good place |
O.k. thanks. I submitted the fix for the one-line comment. Note that the formatting of Regarding |
That may be the case in practice at the moment, but that's based on an invariant around the internals of that function. These types of assumptions always have the potential to be broken down the line when future changes are made (very plausible that the internal implementation Given that we know definitively the snippet in question would produce buggy behavior when comments are present within the bin expression, it makes sense to correct that as part of the effort to address buggy comment behavior in bin expressions. |
fn main() { | ||
let x = 2 /* v */ | ||
/* w */ * /* x */ | ||
/* y */ | ||
4; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one looks pretty off to me. I think we should be expecting something more like:
fn main() {
let x = 2 /* v */ /* w */ * /* x */ /* y */ 4;
}
or if it has to be line breaking, maybe
fn main() {
let x = 2 /* v */ /* w */ *
/* x */ /* y */ 4;
}
(edit: updated since this is back
not front
)
fn main() { | ||
let x = 2 /* v */ | ||
/* w */ | ||
* /* x */ | ||
/* y */ 4; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one also feels off to me (requirements from the style guide for binops for reference: https://github.com/rust-dev-tools/fmt-rfcs/blob/master/guide/expressions.md#binary-operations)
I'm not sure if it should be something like
fn main() {
let x = 2 /* v */
/* w */
* /* x */
/* y */ 4;
}
or even
fn main() {
let x = 2 /* v */ /* w */
* /* x */ /* y */ 4;
}
but I don't think we should force the operand/rhs to a separate newline with increased indent to support comment alignment
i feel like the correct/expected formatting for some of these snippets is getting a little fuzzy, so cc @topecongiro for some clarity and guidance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(cc: @topecongiro)
The above formatting issues are actually not because the pre/post binop comments which this PR is about, but because of outer comments in the expression. However, they may need to be fixed in this PR, as before the original source of the expression was copied and now the output is the formatted expression, which may be worse if these issues are not fixed.
Before I submit further enhancements, I tried to come up with better examples, so we can decide about the desired output. Initially the examples are for the Front case only. In the following examples, I assumed that concatenated closed-comments like /* v */ /* w */
(maybe with new lines between the two comments) will no happen in practice, so they can be ignored in the tests cases. Is that acceptable?
Following is what I assume is a desired output for pre/post binop comments only (except for a following comment/expression just to make sure its formatting continues to be o.k.):
// Desired output (Front)
fn main() {
let y = 2 /* 1 This is the first bin op argument */
/* 2 which will be multiplied by the second argument */
* /* 3 This is the multiplying operation */
/* 4 which multiplies both arguments */ 4;
/* 7 And this is a comment for the next operation */
let i = 7;
}
If it is agreed that this formatting is o.k., then the current PR is o.k.
The following is what I assume is the desired output when outer comments are added:
// Desired output (Front)
fn main () {
let y = 2 /* 1 This is the first bin op argument */
/* 2 which will be multiplied by the second argument */
* /* 3 This is the multiplying operation */
/* 4 which multiplies both arguments */
4; /* 5 This is the second argument */
/* 6 which is multiplied by the first argument */
/* 7 And this is a comment for the next operation */
let i = 7;
}
Currently, there are at least three issues with this example. After we agree about the desired output, should I try to fix them i this PR?
The first issue is that "4
" doesn't start in a new line, which is according the current formatting rules, so it may be that the desired output should be different. The second issues is that comment 6 is not properly indented and is formatted as comment 7:
* /* 3 This is the multiplying operation */
/* 4 which multiplies both arguments */ 4; /* 5 This is the second argument */
/* 6 which is multiplied by the first argument */
/* 7 And this is a comment for the next operation */
let i = 7;
I am not sure if the second issue can be fixed easily, as the original formatting should be taken into account which may be quite complex to do.
The second issue is that when comments 5 and 6 are put before the ";
", there is no distinction in the formatting between comments 5 and 6 which are for the binop and between comment 7.
* /* 3 This is the multiplying operation */
/* 4 which multiplies both arguments */ 4
/* 5 This is the second argument */
/* 6 which is multiplied by the first argument */;
/* 7 And this is a comment for the next operation */
let i = 7;
I assume this issue can be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely something that would need to be addressed in order for this PR to be merged. Currently rustfmt will leave binary expressions with comments between the operator and operand(s) alone, and allow the programmer to adjust the formatting as needed. However, this patch would remove that, and forcibly rewrite code, so the code that's emitted needs to be correct.
The biggest thing that jumps out to me with the proposed changes are the forced vertical/visual alignment of the block comments, which is not compatible with the Rust Style Guide, and accordingly, nor rustfmt's defaults.
For example, the latest version in this PR currently changes:
fn main() {
let y = 2 /* 1 This is the first bin op argument */
/* 2 which will be multiplied by the second argument */
* /* 3 This is the multiplying operation */
/* 4 which multiplies both arguments */ 4;
}
to
fn main() {
let y = 2 /* 1 This is the first bin op argument */
/* 2 which will be multiplied by the second argument */
* /* 3 This is the multiplying operation */
/* 4 which multiplies both arguments */ 4;
}
This is not correct
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely something that would need to be addressed in order for this PR to be merged. Currently rustfmt will leave binary expressions with comments between the operator and operand(s) alone, and allow the programmer to adjust the formatting as needed. However, this patch would remove that, and forcibly rewrite code, so the code that's emitted needs to be correct.
Seems that I completely misunderstood the required formatting of multi-line comments. I understand from the above that multi-line comments should keep their relative indentation. This will actually solve the indentation issues in the previous message. I will try to change the PR so it will work according to the above guidelines. Just to make sure, I understand that for example:
let y = 2 /* comment 1 */
/* comment 2 */
/* comment 3 */
* /* comment 4 */
/* comment 5 */
4;
Should be formatted to:
let y = 2 /* comment 1 */
/* comment 2 */
/* comment 3 */
* /* comment 4 */
/* comment 5 */
4;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that there's a difference between having multiple, single-line block comments (as is the case in the snippets discussed here) vs. a single, multi-line block comment.
/* c1 */ /* c2 */
/* c3 */
vs.
/*
* c1
* c2
* c3
*/
Yes the alignment of the multiline comment is important, but we don't need to visually/vertically align separate comments with each other.
Should be formatted to:
let y = 2 /* comment 1 */ /* comment 2 */ /* comment 3 */ * /* comment 4 */ /* comment 5 */ 4;
No, not that either, as rustfmt shouldn't be maintaining incorrect indentation either; the comments should be properly indented too.
I'd think we'd expect something like this for your example which maintains the author's original comment line positioning, but corrects the indentation for comment 2 and 3
let y = 2 /* comment 1 */
/* comment 2 */
/* comment 3 */
* /* comment 4 */
/* comment 5 */ 4;
It's a situation like this one that I feel like there may be some ambiguity (all with default behavior/config):
fn main() {
let y = 2 /* comment 1 */ /* loooooooooooooooooooooooooooooooooooooooooonnnnnnnnnnnnnnnnnnnnnnnnnnnn */ * /* another lonnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnngggggggggggggggg 4 */ /* comment 5 */ 4;
}
Arguably rustfmt should just fail to format and differ to the programmer to update/correct, because the comments between the lhs operand and operator would exceed max_width, and the comments between the operator and rhs operand will cause that line to also exceed max width. However, I suppose there's a question of whether rustfmt should attempt to split those lines to fit within the limits, and if so, that also begs the question of what to do with the rhs operand given that it's no longer on the same line as the operator (should it go on its own newline as well, should it get another block indent, etc.):
i.e.
fn main() {
let y = 2 /* comment 1 */
/* loooooooooooooooooooooooooooooooooooooooooonnnnnnnnnnnnnnnnnnnnnnnnnnnn */
* /* another lonnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnngggggggggggggggg 4 */
/* comment 5 */ 4;
}
This isn't called out explicitly in the style guide which is unsurprising as these seem like rather edge-y cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't distinguish between multiline comments and block of separate comments ... I make changes according the the above guidelines, which hopefully will produce output that is closer to the desired result.
|
@calebcartwright, I am still trying to find a good enough solution to the BinOp comments, and I understand much better now why it was decided to not handle comments formatting in certain cases.... A main issue is that for proper handling of the BinOp comments, it seems some general changes are required,mainly for handling multiline comments. Such change affect comments formatting in other cases. In any case, I tried to come up with some guidelines for comments handling, based on the previous discussion, on code reverse engineering and on some solutions I found while making the changes. Following is what I cam up with by now: RUSFMT Comment Formatting Suggested Guidelines EnhancementsFollowing are some suggested enhancements for the rusfmt comments writing guidelines for Rust Style Guide. These guidelines are mainly related to indentation of one line comments ( Large part of these guidelines are from reverse engineering of the rustfmt code, so they are not new, but may include clarifications about un-handled cases.. The guidelines start with some issues were it is either not clear what should be the right formatting approach or whether implementing the right approach may be problematic with the current design of rustfmt. IssuesIssue 1. Limitted Use of Multiline CommentsA main issue is proper indentation of multiline comments in different cases, especially such comments in the middle of expression. A possible approach is to limit the use of multiline comments. That is, rustfmt will handle multiline comments only in certain cases. In other cases, the original format will be used. This should probably allow to properly format simple comments in all cases. Issue 2. Comment after the "(" of a Function CallIn rustfmt, a comment after the " let x = function(
// One-line comment
param1,
param2,
); and not: let x = function( // One-line comment
param1,
param2,
); Issue 3. Comments preceding the ";"There are several issues with handling comments that precede the " Issue 4. Multiline Comment Internal Lines Indentation OffsetWhen a multiline comment is appended to the same line of the code it follows, its internal lines are aligned to the first comment line start, and the code indentation. It is not clear whether a new offset parameter should be added for this, or whether the Formatting GuidelinesA. Guidelines for Same line Comments
x = 6; // comment is formatted as: x = 6; // comment
struct s {
a: Bool, // comment 1
bbb: Binary, // comment 2
ccccccc: bool, // comment 5
dddddddd: Binary, // comment 3
e: bool, // comment 4
} is formatted as: struct s {
a: Bool, // comment 1
bbb: Binary, // comment 2
ccccccc: bool, // comment 5
dddddddd: Binary, // comment 3
e: bool, // comment 4
}
struct s {
x2: bool, /* multiline comment 2 line 1
* multiline comment 2 line 2 */
xxxxxx3: Binary, /* multiline comment 3 line 1
* multiline comment 3 line 2 */
xxxxxxxxxxx4: bool, /* comment 4 */
} is formatted as: struct s {
x2: bool, /* multiline comment 2 line 1
* multiline comment 2 line 2 */
xxxxxx3: Binary, /* multiline comment 3 line 1
* multiline comment 3 line 2 */
xxxxxxxxxxx4: bool, /* comment 4 */
} and: let x = flonggggggggggggggggggggggggggggggggggggggggggg(
first_item, /* first */
second, /* second */
); is formatted as: let x = flonggggggggggggggggggggggggggggggggggggggggggg(
first_item, /* first */
second, /* second */ but: struct s {
a: Bool, // Longgggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg comment 1
b: Binary, // comment 2
dddddd: Binary, // comment 3
} is formatted as: struct s {
a: Bool, // Longgggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg comment 1
b: Binary, // comment 2
dddddd: Binary, // comment 3
} B. Guidelines for Multi-comments and Multiline CommentsMulti-comments are comments that follow each other. Multiline comments are comments that include more than one internal line
fn main() {
x = 6; /* Assignment comment 1st line
* Assignment 2nd line */
}
fn main() {
x = 6; /* Assignment comment 1st line
* Assignment comment 2nd line */
/* Code commented out:
let y = 8;
let z = 9;
let u = 20; */
}
y = 7;
/* y=7 comment 1st line
* y=7 comment 2nd line Longggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg */
let x = 5; /* Explanations to assigning x - Same line in origin */
/* Comments to assigning y */
y = 7;
let x = 5;
/* Explanations to assigning x - Starts new line in origin */
/* Comments to assigning y */
let y = 7;
if /* If 1st comment line
* If 2nd comment line */
condition1 /* Condition1 1st comment line
* condition1 2nd comment line */
&& condition2
if /* If 1st comment line
* If 2nd comment line */
condition1 /* Condition1 1st comment line
* condition1 2nd comment line */
&& condition2
let x = /* If 1st comment line
* If 2nd comment line */
22 /* Operand1 comment */ + 33;
let x = /* If 1st comment line
* If 2nd comment line */ 22 /* Operand1 comment */ + 33; C. Misc
/* comment */
x = 6;
if a
| // comment3
b
{} and not: if a | // comment3
b
{} |
@davidBar-On thank you again for the PR, and for writing up your thoughts about comment handling in general. My sense is that we should probably just go ahead and close this PR, as it doesn't seem likely that binop comment handling will be addressed here at this time. More generally, if you'd like to suggest changes to the style guide then that's best done within the Style Guide repo by opening a new issue as an RFC. However, I don't anticipate incorporating anything so prescriptive for comments to the Style Guide itself, which is intentionally silent about things like where comments should/shouldn't be used. If you'd like to suggest some fundamental changes to comment handling then I think the best forum would be a separate, dedicated new issue in this repo. I haven't had a chance to read through the entire write up, but some initial thoughts:
|
@calebcartwright, I am still trying to find a good enough solution to the BinOp comments, and I understand much better now why it was decided to not handle comments formatting in certain cases.... A main issue is that for proper handling of the BinOp comments, it seems some general changes are required,mainly for handling multiline comments. Such change affect comments formatting in other cases. In any case, I tried to come up with some guidelines for comments handling, based on the previous discussion, on code reverse engineering and on some solutions I found while making the changes. Following is what I cam up with by now: RUSFMT Comment Formatting Suggested Guidelines EnhancementsFollowing are some suggested enhancements for the rusfmt comments writing guidelines for Rust Style Guide. These guidelines are mainly related to indentation of one line comments ( Large part of these guidelines are from reverse engineering of the rustfmt code, so they are not new, but may include clarifications about un-handled cases.. The guidelines start with some issues were it is either not clear what should be the right formatting approach or whether implementing the right approach may be problematic with the current design of rustfmt. IssuesIssue 1. Limitted Use of Multiline CommentsA main issue is proper indentation of multiline comments in different cases, especially such comments in the middle of expression. A possible approach is to limit the use of multiline comments. That is, rustfmt will handle multiline comments only in certain cases. In other cases, the original format will be used. This should probably allow to properly format simple comments in all cases. Issue 2. Comment after the "(" of a Function CallIn rustfmt, a comment after the " let x = function(
// One-line comment
param1,
param2,
); and not: let x = function( // One-line comment
param1,
param2,
); Issue 3. Comments preceding the ";"There are several issues with handling comments that precede the " Issue 4. Multiline Comment Internal Lines Indentation OffsetWhen a multiline comment is appended to the same line of the code it follows, its internal lines are aligned to the first comment line start, and the code indentation. It is not clear whether a new offset parameter should be added for this, or whether the Formatting GuidelinesA. Guidelines for Same line Comments
x = 6; // comment is formatted as: x = 6; // comment
struct s {
a: Bool, // comment 1
bbb: Binary, // comment 2
ccccccc: bool, // comment 5
dddddddd: Binary, // comment 3
e: bool, // comment 4
} is formatted as: struct s {
a: Bool, // comment 1
bbb: Binary, // comment 2
ccccccc: bool, // comment 5
dddddddd: Binary, // comment 3
e: bool, // comment 4
}
struct s {
x2: bool, /* multiline comment 2 line 1
* multiline comment 2 line 2 */
xxxxxx3: Binary, /* multiline comment 3 line 1
* multiline comment 3 line 2 */
xxxxxxxxxxx4: bool, /* comment 4 */
} is formatted as: struct s {
x2: bool, /* multiline comment 2 line 1
* multiline comment 2 line 2 */
xxxxxx3: Binary, /* multiline comment 3 line 1
* multiline comment 3 line 2 */
xxxxxxxxxxx4: bool, /* comment 4 */
} and: let x = flonggggggggggggggggggggggggggggggggggggggggggg(
first_item, /* first */
second, /* second */
); is formatted as: let x = flonggggggggggggggggggggggggggggggggggggggggggg(
first_item, /* first */
second, /* second */ but: struct s {
a: Bool, // Longgggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg comment 1
b: Binary, // comment 2
dddddd: Binary, // comment 3
} is formatted as: struct s {
a: Bool, // Longgggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg comment 1
b: Binary, // comment 2
dddddd: Binary, // comment 3
} B. Guidelines for Multi-comments and Multiline CommentsMulti-comments are comments that follow each other. Multiline comments are comments that include more than one internal line
fn main() {
x = 6; /* Assignment comment 1st line
* Assignment 2nd line */
}
fn main() {
x = 6; /* Assignment comment 1st line
* Assignment comment 2nd line */
/* Code commented out:
let y = 8;
let z = 9;
let u = 20; */
}
y = 7;
/* y=7 comment 1st line
* y=7 comment 2nd line Longggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg */
let x = 5; /* Explanations to assigning x - Same line in origin */
/* Comments to assigning y */
y = 7;
let x = 5;
/* Explanations to assigning x - Starts new line in origin */
/* Comments to assigning y */
let y = 7;
if /* If 1st comment line
* If 2nd comment line */
condition1 /* Condition1 1st comment line
* condition1 2nd comment line */
&& condition2
if /* If 1st comment line
* If 2nd comment line */
condition1 /* Condition1 1st comment line
* condition1 2nd comment line */
&& condition2
let x = /* If 1st comment line
* If 2nd comment line */
22 /* Operand1 comment */ + 33;
let x = /* If 1st comment line
* If 2nd comment line */ 22 /* Operand1 comment */ + 33; C. Misc
/* comment */
x = 6;
if a
| // comment3
b
{} and not: if a | // comment3
b
{} |
Ok. Closing the PR. Will open a new one if I will have good enough solution for improving comments handling. |
This PR is for handling pre/post comments to binop, based on initial examples given in #2896 discussion.
Two test files are added (with the same cases): one for "Front" binop_separator and one for "Back".
Comments about approach taken:
Comments are trimmed and are put in the same line of other code when possible. Original prefix/suffix white spaces are trimmed.
Multi-lines comments are appended to a line or starts new line based on the width if longest line in the comment and not the first line width. This is because all the lines in a multi-line comment are indented to the same place, so even if the first line may be appended to the previous line, a longer line in the comment may exceed the maximum allowed width.
When binop_separator=="Front", something will be added to the separator line, so that the separator will not be left alone in the line. This is even if line will become too long. Reasoning is that separators are usually short, so only in rare cases they will be the cause for making the line too long, and readability is better when the separator is not alone in a line.
For example, the code:
a + /* longgggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg comment*/ b
will be formatted as:
Possible Issues:
The prefix/suffix items where added in
PairList struct
usingString
and not&str
, as I could not find an easy way to use&str
. Also, maybe the prefix/suffix items should have been collected into one vector with the separator instead of having 3 separate vectors.There is no distinction between comment that follows
if
condition about the if statement and a comment that precede the execution block after the if statement. This is not directly related to the binop comments, but is raised here since the last comment to the if statement would become a binop prefix comment if another condition will be added to the if statement. For example, in test 22.1 (Front):