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

cmd/gofmt: puts `else if` comments in wrong place #20562

Open
purpleidea opened this issue Jun 3, 2017 · 13 comments
Open

cmd/gofmt: puts `else if` comments in wrong place #20562

purpleidea opened this issue Jun 3, 2017 · 13 comments
Assignees
Labels
Milestone

Comments

@purpleidea
Copy link

@purpleidea purpleidea commented Jun 3, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

play.golang.org

What operating system and processor architecture are you using (go env)?

play.golang.org

What did you do?

Ran this: https://play.golang.org/p/xJ0-Cq8ZYi and clicked format

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.
here it is: https://play.golang.org/p/xJ0-Cq8ZYi

What did you expect to see?

I expected the B and C branch comments to be aligned the same way the A branch comment is..

What did you see instead?

Instead they were indented as if they were part of the previous block.

I get how this is a tricky issue, but if there is a blank line and the comment is right before the next block, I think it should be indented for that block.

Thanks!

@dsnet
Copy link
Member

@dsnet dsnet commented Jun 3, 2017

It's not clear to me that this is wrong. The B and C comments are within the "{" and "}" of the containing if block, so it makes sense to associate the comment at that indentation level.

\cc @griesemer

@dsnet dsnet changed the title gofmt puts `elseif` comments in wrong place cmd/gofmt: puts `else if` comments in wrong place Jun 3, 2017
@purpleidea
Copy link
Author

@purpleidea purpleidea commented Jun 3, 2017

@dsnet If the comments didn't have a space after the previous if statements contents, and if they weren't right against the following line, I'd agree. Where else would you put the comment that should precede the else if?

I've amazingly found an edge case which just wasn't considered previously.

Thanks for looking into it.

@dsnet dsnet added this to the Unplanned milestone Jun 3, 2017
@ALTree
Copy link
Member

@ALTree ALTree commented Jun 3, 2017

I agree with dsnet.

Where else would you put the comment that should precede the else if?

If a comment is about branch B, I'd definitely put it inside the branch it refers to. Like this:

if A {
    // comment about A goes here, at the top of the A branch scope
 
    do stuff...

} else if B {
    // comment about B goes here, at the top of the B branch scope

    do stuff...

} else {
   // comment about else goes here, at the top of the 'else' branch scope

    do stuff...

}

IMHO putting a comment about B before its if (so in the scope of the previous, different branch) would be confusing even with the right indentation and I don't think goftm should indent those comments like you suggest.

@josharian
Copy link
Contributor

@josharian josharian commented Jun 3, 2017

I think this is effectively a dup of #9910. As Russ says there:

Sometimes you want one or the other. Gofmt preserves what you've done.

@purpleidea
Copy link
Author

@purpleidea purpleidea commented Jun 3, 2017

@josharian Nice find! I'm okay with the fix in #9910 about leaving them untouched. It would be better than them always getting moved incorrectly in my case! Thanks for the comment!

@josharian
Copy link
Contributor

@josharian josharian commented Jun 3, 2017

Cool. Closing this as a dup, then.

@josharian josharian closed this Jun 3, 2017
@purpleidea
Copy link
Author

@purpleidea purpleidea commented Jun 3, 2017

@josharian Sorry if I wasn't clear, but this issue isn't fixed, the other one took the right approach which I think we should too!

@josharian
Copy link
Contributor

@josharian josharian commented Jun 3, 2017

OK, reopening, but I'm confused. I clicked on your original playground link, hit format, and nothing moved, which seems like exactly the proposed resolution--when there's ambiguity, don't move things. What am I missing?

@josharian josharian reopened this Jun 3, 2017
@purpleidea
Copy link
Author

@purpleidea purpleidea commented Jun 3, 2017

@josharian Click format on this link:

https://play.golang.org/p/Odchgkj1Tg

I would expect either nothing to move, but unfortunately it does.

On the earlier link, I would expect things to move to look like this link, or if we agree with the solution in #9910 for nothing to move.

@griesemer griesemer self-assigned this Jun 4, 2017
@purpleidea
Copy link
Author

@purpleidea purpleidea commented Jun 24, 2017

Found a workaround until this is fixed. You can do comments like this:

https://play.golang.org/p/mcx57OdNvy

It's a bit awkward, but at least it's gofmt legal, and the comments are at the right indentation level.

@purpleidea
Copy link
Author

@purpleidea purpleidea commented Jun 24, 2017

Full code for those who hate link following:

package main

import (
	"fmt"
)

func main() {
	fmt.Println("Hello, sane comment indenting")

	if false {
		fmt.Println("if")
		// bad indentation
	} else
	// do something else...
	if true {
		fmt.Println("elseif")
	} else

	// otherwise...
	{
		fmt.Println("else")
	}
}

@as
Copy link
Contributor

@as as commented Jun 24, 2017

It's a bit awkward, but at least it's gofmt legal

I disagree completely. The bug in gofmt is that it this awkward workaround is legal. I would hate to this type of style in production, especially since gofmt doesn't fix it.

Removing the strategically-placed comments fixes it, but it requires the comments to be removed/replaced.
https://play.golang.org/p/OotrW-1T-K

It would be nice if gofmt was less lenient with this.

@purpleidea
Copy link
Author

@purpleidea purpleidea commented Jun 24, 2017

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

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.