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: moves comment to inappropriate line #23709

Closed
dfawley opened this issue Feb 5, 2018 · 11 comments
Closed

x/tools/cmd/goimports: moves comment to inappropriate line #23709

dfawley opened this issue Feb 5, 2018 · 11 comments

Comments

@dfawley
Copy link

@dfawley dfawley commented Feb 5, 2018

Create a file with the following contents:

package main

import (
	"math" // fun
)

func main() {
	x := math.MaxInt64
	fmt.Println(strings.Join(",", []string{"hi"}), x)
}
$ goimports -d /tmp/a.go
diff -u /tmp/a.go.orig /tmp/a.go
--- /tmp/a.go.orig	2018-02-05 15:44:54.612402902 -0800
+++ /tmp/a.go	2018-02-05 15:44:54.612402902 -0800
@@ -1,7 +1,9 @@
 package main
 
 import (
-	"math" // fun
+	"fmt"
+	"math"
+	"strings" // fun
 )
 
 func main() {

Expected:

$ goimports -d /tmp/a.go
diff -u /tmp/a.go.orig /tmp/a.go
--- /tmp/a.go.orig	2018-02-05 15:44:54.612402902 -0800
+++ /tmp/a.go	2018-02-05 15:44:54.612402902 -0800
@@ -1,7 +1,9 @@
 package main
 
 import (
+	"fmt"
	"math" // fun
+	"strings"
 )
 
 func main() {

Versions tested:

go version go1.10rc1 linux/amd64
go version go1.9.3 linux/amd64

@gopherbot gopherbot added this to the Unreleased milestone Feb 5, 2018
@gopherbot
Copy link

@gopherbot gopherbot commented Feb 9, 2018

Change https://golang.org/cl/93055 mentions this issue: imports: fixup comments on import lines correctly

@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Feb 9, 2018

@bradfitz Can you take a look?

@lukelafountaine
Copy link

@lukelafountaine lukelafountaine commented Feb 13, 2018

I would like to start contributing to Go. Would this be a good issue to cut my teeth on?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Feb 13, 2018

@lukelafountaine it looks like someone may have already sent a patch for this--see the comment about CL 93055 above.

@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Jun 29, 2018

I'm not sure it's a problem, but this line:

https://github.com/golang/tools/blob/master/go/ast/astutil/imports.go#L141

set all imported line to the same position in file, then when sorting it, the current position is tracked:

https://github.com/golang/tools/blob/master/imports/sortimports.go#L106

In this example, math and strings both have EndPos at 38, the same for // fun, so it ends up display beside strings.

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 1, 2018

Change https://golang.org/cl/121878 mentions this issue: imports: fixup comments on import lines correctly

@cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Jul 2, 2018

cc @bradfitz I sent new CL for this issue, please take a look

@ncw
Copy link
Contributor

@ncw ncw commented Jul 3, 2018

This change appears to have broken goimports in a different way putting spurious blank lines after import lines with comments.

~/go/src/github.com/ncw/rclone$ find . -name *.go | grep -v /vendor/ | xargs goimports -d

diff -u ./cmd/serve/webdav/webdav.go.orig ./cmd/serve/webdav/webdav.go
--- ./cmd/serve/webdav/webdav.go.orig	2018-07-03 22:24:12.339811431 +0100
+++ ./cmd/serve/webdav/webdav.go	2018-07-03 22:24:12.339811431 +0100
@@ -16,6 +16,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"
 )
 
diff -u ./fs/rc/rc.go.orig ./fs/rc/rc.go
--- ./fs/rc/rc.go.orig	2018-07-03 22:24:13.099814169 +0100
+++ ./fs/rc/rc.go	2018-07-03 22:24:13.099814169 +0100
@@ -12,6 +12,7 @@
 	"io"
 	"net/http"
 	_ "net/http/pprof" // install the pprof http handlers
+
 	"strings"
 
 	"github.com/ncw/rclone/cmd/serve/httplib"

I don't know if that was intended but before this change goimports didn't insert that spurious blank line.

ncw added a commit to rclone/rclone that referenced this issue Jul 3, 2018
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Jul 6, 2018

@ncw, could you file a new bug and cc the author on it and reference this bug?

@ncw
Copy link
Contributor

@ncw ncw commented Jul 6, 2018

@bradfitz I've made a new issue here #26246

@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

gopherbot pushed a commit to golang/tools that referenced this issue Jul 9, 2018
The fix in golang/go#23709 introduced a separate bug where extra blank
lines were sometimes inserted. This fixes that newly introduced bug.

Fixes golang/go#26246

Change-Id: I78131cc1d01ae246922ed9e4336ebb31d1c6cfa1
Reviewed-on: https://go-review.googlesource.com/122538
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@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.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.