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: non-idempotent formatting of line comments #13702

Closed
cezarsa opened this issue Dec 21, 2015 · 4 comments
Closed

cmd/gofmt: non-idempotent formatting of line comments #13702

cezarsa opened this issue Dec 21, 2015 · 4 comments
Assignees
Milestone

Comments

@cezarsa
Copy link
Contributor

@cezarsa cezarsa commented Dec 21, 2015

tl;dr: Access http://play.golang.org/p/Ry5OUCK43C; Click Format; Click Format.

After running the following snippet through gofmt -w:

package main

//line x:1


func main() {
}

The output is:

package main

//line x:1

func main() {
}

This output looks correct with a single newline between //line and func main. However if we run gofmt -w again on the same file the output is different:

package main

//line x:1
func main() {
}

Now the newline between //line and func main was removed. This behavior is different from what would happen if instead of //line we had a regular commented line.

This was tested both on master and 1.5.1:

$ go version
go version devel +97f854c Sat Dec 19 10:00:04 2015 +0000 darwin/amd64
@griesemer griesemer self-assigned this Dec 21, 2015
@griesemer griesemer added this to the Go1.6Maybe milestone Dec 21, 2015
@rsc
Copy link
Contributor

@rsc rsc commented Jan 6, 2016

Unfortunate but probably shouldn't be running gofmt on files with //line comments anyway. Can wait for Go 1.7.

@rsc rsc modified the milestones: Go1.7, Go1.6Maybe Jan 6, 2016
@rsc rsc modified the milestones: Go1.8, Go1.7 May 17, 2016
@quentinmit quentinmit added the NeedsFix label Oct 6, 2016
@rsc rsc modified the milestones: Go1.9, Go1.8 Oct 21, 2016
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented May 23, 2017

@griesemer, do you still want to do this for Go 1.9? It missed the past 4 releases and still repros.

@griesemer
Copy link
Contributor

@griesemer griesemer commented May 24, 2017

@bradfitz I will need to see who complicated the fix is, first. I won't get to this before June 1.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Jun 5, 2017

I don't think we should do anything here: Fixing this would mean that once we see a //line directive we're not allowed to move anything that follows later to a different line. At the very least, that is the assumption we have to make until we see the next //line comment. But that interferes directly with gofmt's main purpose which is to organize white space for uniform formatting. One "solution" would be to not move any lines that follow a //line comment. But it's pretty tricky to get this correct under all circumstances. I'm not convinced the amount of complexity is worth the few cases where it might matter. I don't want to go down that route.

The situation is additionally complicated by the fact that the go/printer has a mode that emits //line comments.

I've recently fixed #5945 which makes sure that the above-mentioned mode only prints //line directives immediately before non-whitespace. Code that is printed that way will pass through gofmt undisturbed.

In other words, don't use //line directives in front of empty lines - use them where it matters. Or don't gofmt such files.

Closing as unfortunate.

@griesemer griesemer closed this Jun 5, 2017
@griesemer griesemer added Unfortunate and removed NeedsFix labels Jun 5, 2017
@golang golang locked and limited conversation to collaborators Jun 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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.