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

Jump to bottom

cmd/gofmt: not idempotent on line with multiple comments #24472

Open
ALTree opened this issue Mar 21, 2018 · 4 comments
Open

cmd/gofmt: not idempotent on line with multiple comments #24472

ALTree opened this issue Mar 21, 2018 · 4 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unplanned

Comments

@ALTree
Copy link
Member

ALTree commented Mar 21, 2018 

$ gotip version
go version devel +5f0a9ba134 Tue Mar 20 22:46:00 2018 +0000 linux/amd64

Note as the second gofmt invocation changes the code again:

$ cat test.go
package p

func f() {
	foo(); /* one */ fooooo(); /* two */
}

$ cat test.go | gofmt
package p

func f() {
	foo() /* one */
	fooooo() /* two */
}

$ cat test.go | gofmt | gofmt
package p

func f() {
	foo()    /* one */
	fooooo() /* two */
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Mar 21, 2018
@ALTree ALTree added this to the Go1.11 milestone Mar 21, 2018
@ALTree ALTree changed the title go/fmt: not idempotent on line with multiple comments gofmt: not idempotent on line with multiple comments Mar 21, 2018
@ALTree ALTree mentioned this issue Mar 21, 2018
Closed
@griesemer griesemer removed this from the Go1.11 milestone Mar 21, 2018
@griesemer griesemer self-assigned this Mar 21, 2018
@griesemer griesemer added this to the Unplanned milestone Mar 21, 2018
@ysmolski
Copy link
Member

FYI, this break following test:

% go test cmd/gofmt
--- FAIL: TestAll (13.14s)
	long_test.go:88: gofmt /Users/thorn/golang/test/fixedbugs/issue22662.go not idempotent
FAIL
FAIL	cmd/gofmt	13.182s

@gopherbot
Copy link

Change https://golang.org/cl/130377 mentions this issue: cmd/gofmt: skip gofmt idempotency check on known issue

gopherbot pushed a commit that referenced this issue Aug 21, 2018
@ALTree @griesemer
cmd/gofmt: skip gofmt idempotency check on known issue
3649fe2 
gofmt's TestAll runs gofmt on all the go files in the tree and checks,
among other things, that gofmt is idempotent (i.e. that a second
invocation does not change the input again).

There's a known bug of gofmt not being idempotent (Issue #24472), and
unfortunately the fixedbugs/issue22662.go file triggers it. We can't
just gofmt the file, because it tests the effect of various line
directives inside weirdly-placed comments, and gofmt moves those
comments, making the test useless.

Instead, just skip the idempotency check when gofmt-ing the
problematic file.

This fixes go test on the cmd/gofmt package, and a failure seen on the
longtest builder.

Updates #24472

Change-Id: Ib06300977cd8fce6c609e688b222e9b2186f5aa7
Reviewed-on: https://go-review.googlesource.com/130377
Reviewed-by: Daniel Martí <mvdan@mvdan.cc>
Reviewed-by: Robert Griesemer <gri@golang.org>
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@ALTree
Copy link
Member Author

ALTree commented Aug 21, 2018

The bad file tested fixedbugs/issue22662.go (which was causing failures in the std library test suite in TestAll when run without -short) is now skipped (CL 130377). The check should be re-enabled on that file when this issue is fixed.

@homuler
Copy link

homuler commented Dec 4, 2022

It seems that this issue is caused by the following block.

go/src/go/printer/printer.go

Lines 383 to 388 in 3a7a528

if pos.Line == next.Line {
// next item is on the same line as the comment
// (which must be a /*-style comment): separate
// with a blank instead of a tab
sep = ' '
}

When the next token is on the same line as the comment, this appends a space character instead of \t.
However, the next token is not necessarily on the same line after formatting.

@dmitshur dmitshur changed the title gofmt: not idempotent on line with multiple comments cmd/gofmt: not idempotent on line with multiple comments Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
5 participants
@ysmolski @ALTree @homuler @griesemer @gopherbot