golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
122.62k stars 17.49k forks source link

cmd/gofmt: moves comment in for #9460

Open sethwklein opened 9 years ago

sethwklein commented 9 years ago

On playground (and elsewhere), this code

package main

func main() {
        for i := 0; i < 1 /* 9000 */ ; i++ {
                _ = i
        }
}

is transformed by gofmt into this code:

package main

func main() {
    for i := 0; i < 1; /* 9000 */ i++ {
        _ = i
    }
}

It has swapped the comment and the nearby semi-colon.

This code arises when temporarily changing the limit of a loop.

I think this swap changes the context of the comment, possibly changing its meaning.

It also means that I'll have to swap 9000 (in this example) and the semi-colon back when restoring the original loop limit.

griesemer commented 9 years ago

The issue here is that the AST (go/ast) doesn't record the position of semicolons for the 'for' statement and thus comments get placed afterwards. The true fix would be to record all semicolon positions, but we don't do this elsewhere because it just costs extra space and such comments are exceedingly rare. This was a pre-2009 design decision; we're not going to change it now.

The other option is to heuristically assume that in a for-loop such semicolons are assumed to be immediate following (and adjacent) to the pre-, cond-, or post-expression in a for loop. This would fix this case but then misplace a comment in the situation

for i := 0; i < n; /* foo */ i++ {...

where the /* foo */ comment is supposed to be before the post-statement. This might be an option if the assumption is that comments are likely to be after an expression rather than before (which is not unreasonable).

This is not urgent, and there's a trivial work-around: simply put the comment before the 1 in your example and the meaning doesn't get distorted. This program

package main

func main() { for i := 0; i < /* 9000 */ 1; i++ { _ = i } }

gets formatted as is.

Leaving open for now as a reminder for future AST changes.

griesemer commented 9 years ago

This issue is very specific to 'for'-statements and not related to other comment formatting issues. Removing adg's comment above.

speter commented 9 years ago

For reference, a similar change occurs in composite literals: http://play.golang.org/p/1MPnS7RUuF

var ints = []int{ /* one */ 1, /* two */ 2}

becomes

var ints = []int{ /* one */ 1 /* two */, 2}