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: inserts unexpected blank space after an inline comment #26246

Closed
ncw opened this issue Jul 6, 2018 · 7 comments
Closed

x/tools/cmd/goimports: inserts unexpected blank space after an inline comment #26246

ncw opened this issue Jul 6, 2018 · 7 comments

Comments

@ncw
Copy link
Contributor

@ncw ncw commented Jul 6, 2018

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

go version go1.10.1 linux/amd64

with

goimports latest 16f8f9b

Does this issue reproduce with the latest release?

Yes with the latest version of goimports

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/home/ncw/.cache/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/ncw/go"
GORACE=""
GOROOT="/opt/go/go1.10"
GOTMPDIR=""
GOTOOLDIR="/opt/go/go1.10/pkg/tool/linux_amd64"
GCCGO="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-build952400509=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I saved this program into a file https://play.golang.org/p/soaLshUrvTf

package main

import (
	"fmt"
	"os"

	"github.com/ncw/rclone/vfs/vfsflags"
	"github.com/spf13/cobra"
	"golang.org/x/net/context" // switch to "context" when we stop supporting go1.8
	"golang.org/x/net/webdav"
)

func main() {
	var (
		_ context.Context
		_ *os.File
		_ webdav.File
		_ cobra.Command
		_ = vfsflags.Opt
	)
	fmt.Println("Hello")
}

I then ran goimports -d on it

What did you expect to see?

I expected to see no output

What did you see instead?

I saw that goimports wanted to add an extra blank line in.

diff -u goimportstest.go.orig goimportstest.go
--- goimportstest.go.orig	2018-07-06 09:19:48.722405604 +0100
+++ goimportstest.go	2018-07-06 09:19:48.722405604 +0100
@@ -7,6 +7,7 @@
 	"github.com/ncw/rclone/vfs/vfsflags"
 	"github.com/spf13/cobra"
 	"golang.org/x/net/context" // switch to "context" when we stop supporting go1.8
+
 	"golang.org/x/net/webdav"
 )
 

Discussion

goimports didn't used to add this blank line in, so I used git bisect to discover where the behaviour changed, and I discovered it was introduced in b23eb62 by @Gnouc as part of fixing #23709

I haven't worked out the exact situation that causes the problem - it seems quite sensitive to exactly which import lines are present. For example if you remove the vfsflags import then it doesn't insert that extra blank line. If you remove the comment // switch to ... then it doesn't exhibit the problem.

@mvdan
Copy link
Member

@mvdan mvdan commented Jul 6, 2018

Comments are tricky to handle, so I wouldn't be surprised if some edge case was mishandled in the fix above.

@mvdan mvdan changed the title x/tools/cmd/goimports: inserts unexpected blank space x/tools/cmd/goimports: inserts unexpected blank space after an inline comment Jul 6, 2018
@fraenkel
Copy link
Contributor

@fraenkel fraenkel commented Jul 6, 2018

This new behavior was introduced by imports: fixup comments on import lines correctly

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 9, 2018

Change https://golang.org/cl/122538 mentions this issue: imports: fix fixup comment insert extra blank line

@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Jul 9, 2018

@ncw I push a fix for that case, would you mind checking?

@ncw
Copy link
Contributor Author

@ncw ncw commented Jul 9, 2018

@fraenkel that fixes the issue I posted but not this one

package main

import (
	"encoding/json"
	"io"
	"net/http"
	_ "net/http/pprof" // install the pprof http handlers
	"strings"

	"github.com/pkg/errors"
)

func main() {
	_ = strings.ToUpper("hello")
	_ = io.EOF
	var (
		_ json.Number
		_ *http.Request
		_ errors.Frame
	)
}
diff -u Go/goimportstest2.go.orig Go/goimportstest2.go
--- Go/goimportstest2.go.orig	2018-07-09 09:53:26.451454978 +0100
+++ Go/goimportstest2.go	2018-07-09 09:53:26.451454978 +0100
@@ -5,6 +5,7 @@
 	"io"
 	"net/http"
 	_ "net/http/pprof" // install the pprof http handlers
+
 	"strings"
 
 	"github.com/pkg/errors"
@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Jul 9, 2018

@ncw should ok now.

@ncw
Copy link
Contributor Author

@ncw ncw commented Jul 9, 2018

@Gnouc That fixes all my test cases :-) I also tried it on a larger corpus of go files and that looks fine too -so looked fixed to me :-)

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
5 participants
You can’t perform that action at this time.