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

x/tools/cmd/goimports: lost line between package and import statements in output #26921

Open
sergeleger opened this issue Aug 10, 2018 · 4 comments

Comments

@sergeleger
Copy link

commented Aug 10, 2018

Please answer these questions before submitting your issue. Thanks!

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

go version go1.10.3 linux/amd64

Does this issue reproduce with the latest release?

Yes with go1.10.3 and the following commits:

golang.org/x/tools/imports: 8cb83b71b42ccf5fe279fa8a24a6a8f65507dc9c
golang.org/x/tools/cmd/goimports: 059bec968c61383b574810040ba9410712de36c5

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/legers/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/legers/go"
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GCCGO="/usr/bin/gccgo"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build017712154=/tmp/go-build -gno-record-gcc-switches"

What did you do?

Very similar to issue #26290, but no initial import statement.

$ cat x.go
package p // comment

// T does something useful.
func T() {
    var _ = fmt.Printf
}

$ goimports x.go
package p // comment

import "fmt" // T does something useful.
func T() {
	var _ = fmt.Printf
}

A blank line should have been inserted after the new import statement.

@gopherbot gopherbot added this to the Unreleased milestone Aug 10, 2018

@bcmills

This comment has been minimized.

Copy link
Member

commented Aug 10, 2018

@fraenkel

This comment has been minimized.

Copy link
Contributor

commented Aug 10, 2018

well, with head, you get the opposite

package p // comment
import "fmt"

// T does something useful.
func T() {
	var _ = fmt.Printf
}
@wingyplus

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2018

I found that it's incorrect because of impDecl.TokPos = c.End() + 2. So I try changed to impDecl.TokPos = c.End() + 1, it returns correct order but not format the source:

package p // comment
import "fmt"

// T does something useful.
func T() {
        var _ = fmt.Printf
}

I think it's a bug in go/format package that it doesn't format import statement. Reproduce source is:

package main

import (
	"fmt"
	"go/format"
	"log"
)

const src = `package main // comment
import "fmt"

// T does something useful.
func T() {
  var _ = fmt.Printf
}
`

func main() {
	out, err := format.Source([]byte(src))
	if err != nil {
		log.Fatal(err)
	}
	fmt.Println(string(out))
}
@josharian

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2018

The root cause is #22631. However, we'll need to double-check once that has been fixed that goimports does something acceptable when built with and without that fix. In particular, I want to make sure that once the go/printer bug is fixed, it fixes the bug in the context of goimports, and if goimports does need further changes, that those further changes don't break goimports as built without the go/printer fix. Leaving this bug open for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.