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

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
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@ncw
Copy link
Contributor

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.
@gopherbot gopherbot added this to the Unreleased milestone Jul 6, 2018
@ncw ncw mentioned this issue Jul 6, 2018
Closed
@mvdan
Copy link
Member

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 added help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jul 6, 2018
@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 commented Jul 6, 2018

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

@gopherbot
Copy link

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

@cuonglm
Copy link
Member

cuonglm commented Jul 9, 2018

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

@ncw
Copy link
Contributor Author

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
Member

cuonglm commented Jul 9, 2018

@ncw should ok now.

@ncw
Copy link
Contributor Author

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 :-)

@mvdan mvdan mentioned this issue Jul 9, 2018
Closed
@bwplotka bwplotka mentioned this issue Mar 27, 2019
Closed
@golang golang locked and limited conversation to collaborators Jul 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
5 participants
@ncw @fraenkel @mvdan @cuonglm @gopherbot