-
Notifications
You must be signed in to change notification settings - Fork 898
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
Incorrect comment indent inside if/else #4120
Comments
It seems this isn't specific to if/else -- the same might apply to any indented comment before a extern "C" {
fn first();
- // TODO: add rest
// TODO: add rest
} |
I like to work on this issue. Probably this issue was caused in here. rustfmt/src/formatting/visitor.rs Line 298 in 19e3caa
|
There's been a bit of back and forth on this over the years, and unfortunately that complicates things a bit (refs #1575, #1737). My initial reaction was that the comment resides within the block, and thus it should receive the same indent as other items (including comments) within the block, and that folks that really wanted a comment on part of a control flow could do something like fn main() {
let x = if true {
1
}
// comment
else {
0
};
} but that causes problems due to a separate issue with the handling of if expressions. The indentation in the extern block is just plain wrong and needs to be fixed. However, in the if/else case, I feel there's valid scenarios for both indentations, especially since rustfmt doesn't really know if the comment is just the last member of the block or intended to be associated to the @whizsid - if possible (and possible to do so idempotently) my preference and recommendation for the if/else case would be to indent based on the original placement to attempt to honor the author's intent. If the comment as at or to the right of the block indented level, then align it with the other block items. If it's to the left then the author most likely deliberately wants the comment associated with the below content ( So fn main() {
let x = if true {
1
// comment with wrong indent
} else {
0
};
let x = if true {
1
// comment with wrong indent
} else {
0
};
if cond {
"if"
// else comment
} else {
"else"
}
if cond {
"if"
// else comment
} else {
"else"
}
} would be formatted as fn main() {
let x = if true {
1
// comment with wrong indent
} else {
0
};
let x = if true {
1
// comment with wrong indent
} else {
0
};
if cond {
"if"
// else comment
} else {
"else"
}
if cond {
"if"
// else comment
} else {
"else"
}
} However, that should only be utilized on if/else, so extern "C" {
fn first();
// TODO: add rest
}
extern "C" {
fn first();
// TODO: add rest
}
extern "C" {
fn first();
// TODO: add rest
} would be formatted as extern "C" {
fn first();
// TODO: add rest
}
extern "C" {
fn first();
// TODO: add rest
}
extern "C" {
fn first();
// TODO: add rest
} (edit should really only apply to if/else, not the other types of control flows) |
If I wanted to comment an else block, I'd use one of the following (unfortunately rustfmt does not like the first two): if cond() {
...
} else /* !cond() */ {
...
}
if cond() {
...
} else { // !cond()
...
}
if cond() {
...
} else {
// cond() is false
...
} So IMO it is completely wrong to assume a comment on the last line of the if block refers to the else condition. |
I agree, we should not categorically make that assumption. As I stated in my previous comment, we should only do that alignment conditionally, and this is what was subsequently implemented in #4459 This issue was closed as is our practice when changes are merged in source, and the fix will be immediately available for folks using v2.0 from source. We'll also look at backporting this to an upcoming 1.x release |
Change will be available in v1.4.26 which should hit nightly within the next couple days. Note that rustfmt won't modify the existing indentation, but if you manually re-indent a comment within the if block (e.g. the In the case of trailing comments in an extern block, that's also been fixed in v1.4.26 and those will always be properly indented. |
Example:
(And no, this definitely should not be a comment on the
else
— actually, it should not be rustfmt's decision whether this applies to theelse
or is a free comment after the1
.)The text was updated successfully, but these errors were encountered: