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: a comment at the end of a line clings onto/prevents insertion of a newline before the next line #22631

Open
nishanths opened this Issue Nov 8, 2017 · 14 comments

Comments

Projects
None yet
7 participants
@nishanths

nishanths commented Nov 8, 2017

Please answer these questions before submitting your issue. Thanks!

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

go version go1.9.2 darwin/amd64

Does this issue reproduce with the latest release?

yes

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

GOARCH="amd64"
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"

What did you do?

  1. Created a file named a.go with the following contents:
package foo // import "example.org/foo"
import "fmt"
var ErrWhatever = fmt.Errorf("whatever")
  1. Ran gofmt a.go. The output was:
package foo // import "example.org/foo"
import "fmt"

var ErrWhatever = fmt.Errorf("whatever")

What did you expect to see?

Expected the gofmt output to have a newline between the package and the import lines; i.e. like so:

package foo // import "example.org/foo"

import "fmt"

var ErrWhatever = fmt.Errorf("whatever")

What did you see instead?

The gofmt output did not have a newline between the package and the import lines.

If the import comment // import "example.org/foo" isn't present in the file, then the newline is produced. It looks like gofmt's behavior is different when an import comment exists, but that shouldn't be the case.

@griesemer griesemer added this to the Go1.10 milestone Nov 8, 2017

@griesemer

This comment has been minimized.

Contributor

griesemer commented Nov 8, 2017

Thanks for this report. That is indeed a bug.

@griesemer griesemer added the NeedsFix label Nov 8, 2017

@odeke-em

This comment has been minimized.

Member

odeke-em commented Nov 9, 2017

Thank you for the report @nishanths. I think the bug manifests if any comment, not just the "import" comment is present, for example https://play.golang.org/p/gAp3FSD3H4

package main //abcd
import "fmt"

func main() { fmt.Printf("Hello world!") }

or

package main // abcd
import "fmt"

func main() { fmt.Printf("Hello world!") }
@odeke-em

This comment has been minimized.

Member

odeke-em commented Nov 9, 2017

Okay, the same problem manifests on any line that ends with a // or /* prefixed comment, not just for packages and imports, for example:

package main
func main() {}
var _ = 10 // this
func foo() {}

gets formatted to

package main

func main() {}

var _ = 10 // this
func foo() {}

yet it should have been formatted to

package main

func main() {}

var _ = 10 // this

func foo() {}

It seems like the presence of a comment at the end makes the line stick to its next single proceeding line
e.g

package main
func main() {}
var _ = 10 // this
func foo() {}
var _ = 11

gets formatted to

package main

func main() {}

var _ = 10 // this
func foo() {}

var _ = 11

notice that var _ = 10 // this stuck with its next newlined neighbor func foo() {}.

Hope this can help with the prognosis of the bug and perhaps help us look elsewhere e.g go/parser?

@odeke-em odeke-em changed the title from cmd/gofmt: missing newline between package and import lines, if an import comment is present to cmd/gofmt: a comment at the end of a line prevents newline insertion when formatting Nov 9, 2017

@odeke-em

This comment has been minimized.

Member

odeke-em commented Nov 9, 2017

@griesemer I've retitled the bug, please feel free to revert/revise accordingly.

@odeke-em odeke-em changed the title from cmd/gofmt: a comment at the end of a line prevents newline insertion when formatting to cmd/gofmt: a comment at the end of a line clings onto/prevents insertion of a newline before the next line Nov 9, 2017

@griesemer

This comment has been minimized.

Contributor

griesemer commented Nov 13, 2017

This seems like a more general problem

package p     // foo
import "math" // foo
var x int     // foo
const c = 0

also gets formatted without empty lines between the different sections.

Edited: Added this comment w/o reading @odeke-em's findings above.

@griesemer

This comment has been minimized.

Contributor

griesemer commented Nov 14, 2017

Analysis: The problem is clearly in go/printer. After interspersing a comment (called from p.flush), any "left-over" newlines and formfeeds in the white space buffer (p.wsbuf) are consumed but for one (p.writeCommentSuffix). Afterwards, p.print will introduce extra newlines as necessary, based on actual positions.

In this specific case, we want to force extra newlines between declarations that are not as far apart, and we force those extra newlines in p.declList, but unfortunately they get consumed per the above description.

It does appear that this is the only place where we force extra newlines, and thus this hasn't shown up elsewhere. We may need a different mechanism here.

@odeke-em

This comment has been minimized.

Member

odeke-em commented Nov 14, 2017

@griesemer in deed, I took at stab at it in the ungodly hours of Saturday morning, going line by line and noticed that the formatting tripped out in go/printer in the code related to

needsLinebreak = false
, and when I commented that line it almost formatted alright but with an extra newline. I figured that perhaps I had a wrong analysis since I had been up for too long and was exhausted, but it is awesome to see an independent confirmation from you #insertMillyRockingGIF

@rsc

This comment has been minimized.

Contributor

rsc commented Nov 22, 2017

At this point we should probably punt to Go 1.11. I haven't done that only because the issue is so recently filed.

@josharian

This comment has been minimized.

Contributor

josharian commented Aug 13, 2018

This keeps getting rediscovered, and dups filed. Moving from Unplanned back to 1.12.

@jamdagni86

This comment has been minimized.

Contributor

jamdagni86 commented Sep 17, 2018

@griesemer - I took a look at the issue and as you pointed out in an earlier comment, p.writeCommentSuffix writes only one newline (or a formfeed) and ignores any newlines present in the whitespace buffer.

I managed to fix this by making printer.writeCommentSuffix write all the newlines/formfeeds present in printer.wsbuf. However, this causes extra newlines to be added when blocks or functions end with a commented line.

In printer.linebreak we need to figure out if we already wrote a comment. And if we did, only one linebreak is required.

if p.comment != nil {
	pos := p.posFor(p.comment.Pos())
	if p.last.Line <= pos.Line && pos.Line < line && line-p.last.Line > 1 {
		n = 1
	}
}

Also, instances like

type _ struct { /* this comment should be visible */
}

where there are no fields in the struct would break. We need to change printer.fieldList to write required number of formfeeds depending on the length of fields.List

Please let me know if the approach looks OK. I will send a PR for the same.

@griesemer

This comment has been minimized.

Contributor

griesemer commented Sep 17, 2018

@jamdagni86 Thanks for looking into this. I don't know if your approach is ok w/o actually seeing it. I'd be a bit concerned if you have to change to many pieces in the formatter as the more you change the more likely it is that you'll unexpectedly change other behavior. Ideally the fix addresses exactly the specific problem.

@jamdagni86

This comment has been minimized.

Contributor

jamdagni86 commented Sep 17, 2018

@griesemer I found another way to fix this, however I'm not sure if it's the best way though. Since we know the problem exists only for the declarations at the top level, and we'd have the required number of newlines/formfeeds in printer.wsbuf, we can use the printer.indent to selectively determine if we need to ignore the formfeed/newlines by wrapping line no. 709-712 by an if condition.

} else {
if ch == formfeed {
droppedFF = true
}
p.wsbuf[i] = ignore
}

to

if p.indent > 0 {
	if ch == formfeed {
		droppedFF = true
	}
	p.wsbuf[i] = ignore
}

I can still send a PR for the first approach if you want to have a look.

@jamdagni86

This comment has been minimized.

Contributor

jamdagni86 commented Sep 24, 2018

@griesemer did you get a chance to look at the above solution?

@griesemer

This comment has been minimized.

Contributor

griesemer commented Sep 24, 2018

@jamdagni86 Sorry, not yet. It's on my list but it may take a while as this is not the highest priority item.

@ianlancetaylor ianlancetaylor added this to the Go1.13 milestone Dec 7, 2018

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