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

Incorrect comment indent inside if/else #4120

Closed
dhardy opened this issue Apr 13, 2020 · 6 comments · Fixed by #4459
Closed

Incorrect comment indent inside if/else #4120

dhardy opened this issue Apr 13, 2020 · 6 comments · Fixed by #4459
Milestone

Comments

@dhardy
Copy link

dhardy commented Apr 13, 2020

Example:

fn main() {
    let x = if true {
        1
    // comment with wrong indent
    } else {
        0
    };
}

(And no, this definitely should not be a comment on the else — actually, it should not be rustfmt's decision whether this applies to the else or is a free comment after the 1.)

@topecongiro topecongiro added this to the 3.0.0 milestone Jun 29, 2020
@dtolnay
Copy link
Member

dtolnay commented Sep 22, 2020

It seems this isn't specific to if/else -- the same might apply to any indented comment before a }. I just got a diff like this from rustfmt as of current master (4a88598):

  extern "C" {
      fn first();

-     // TODO: add rest
  // TODO: add rest
  }

@whizsid
Copy link
Contributor

whizsid commented Oct 7, 2020

I like to work on this issue. Probably this issue was caused in here.

fn close_block(&mut self, span: Span, unindent_comment: bool) {

@calebcartwright
Copy link
Member

calebcartwright commented Oct 9, 2020

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 else

@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 (else...)

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)

@dhardy
Copy link
Author

dhardy commented Oct 9, 2020

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.

@calebcartwright
Copy link
Member

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

@calebcartwright
Copy link
Member

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 // comment with wrong indent comment from the issue description), then rustfmt will honor that indentation level going forward for trailing comments in if/else.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants