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: interspersed comments can be associated with the wrong import #28200

Open
benesch opened this issue Oct 14, 2018 · 10 comments

Comments

@benesch
Copy link
Contributor

commented Oct 14, 2018

Given the following file:

# foo.go

package main

import (
	_ "bar"
	// foo has side effects.
	_ "foo"
)

goimports will incorrectly associate the comment with "bar" instead of "foo", producing this file:

package main

import (
	_ "bar" // foo has side effects.
	_ "foo"
)

This is using the latest master of the tools repo (5e66757b835f155f7e50931f54c9f6af8af86f75).

@gopherbot gopherbot added this to the Unreleased milestone Oct 14, 2018

@lopezator

This comment has been minimized.

Copy link

commented Oct 15, 2018

We also ran into this issue. Searching arround we found that this commit to the tools repository introduces this bug:

golang/tools@12a7c31

Checking out/building any prior commit fixes the problem.

@ostromart

This comment has been minimized.

Copy link

commented Oct 16, 2018

We hit this too - seems to be a regression introduced in the above commit, since it's clearly not doing the right thing.

@benesch

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2018

/cc @bradfitz

@hklai hklai referenced this issue Oct 16, 2018
@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

@saheienko, can you look into this bug? It seems to have been caused by your previous change.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

(And if it's easier for you, we can just revert your change for now until you have a fixed version of it. But if you can easily fix quickly, we can roll forward instead of back.)

@saheienko

This comment has been minimized.

Copy link

commented Oct 16, 2018

@bradfitz yes, I'll try, but not sure it's related. Looks like goimports grabs a comment that follows the import path (as in the linked issues).

#26921 similar to this, I've even added a test case for it: golang/tools@12a7c31#diff-2ce00388d8d7ca66d8abc5ae7c3954b5R1127

@glerchundi

This comment has been minimized.

Copy link

commented Oct 18, 2018

As @lopezator said, the problem is exactly in that concrete commit 12a7c317e894c0a622b5ed27f0a102fcdd56d1ad:

$ git checkout 87312bc3edd028a31d662e145d02b38790f4c716
HEAD is now at 87312bc3 go/packages: use effective GOARCH to determine type size function
$ go build cmd/goimports/goimports{.go,_not_gc.go}
$ ./goimports main.go 
package main

import (
	_ "bar"
	// foo has side effects.
	_ "foo"
)
$ git checkout 12a7c317e894c0a622b5ed27f0a102fcdd56d1ad
Previous HEAD position was 87312bc3 go/packages: use effective GOARCH to determine type size function
HEAD is now at 12a7c317 imports: support repairing import grouping/ordering
$ go build cmd/goimports/goimports{.go,_not_gc.go} 
$ ./goimports main.go 
package main

import (
	_ "bar" // foo has side effects.
	_ "foo"
)
@saheienko

This comment has been minimized.

Copy link

commented Oct 18, 2018

Hi @lopezator, let's use another instances.

Build goimports:

$ git checkout 87312bc3edd028a31d662e145d02b38790f4c716
HEAD is now at 87312bc3 go/packages: use effective GOARCH to determine type size function

$ go build -o /tmp/goimports-87312bc3 golang.org/x/tools/cmd/goimports

$ git checkout 12a7c317e894c0a622b5ed27f0a102fcdd56d1ad
Previous HEAD position was 87312bc3 go/packages: use effective GOARCH to determine type size function
HEAD is now at 12a7c317 imports: support repairing import grouping/ordering

$ go build -o /tmp/goimports-12a7c317 golang.org/x/tools/cmd/goimports

In both cases it fails to place the comment.

$ cat <<EOF > /tmp/main1.go
package main

import (
        _ "foo2"
        // foo3 has side effects.
        _ "foo3"
        _ "foo1"

)
EOF

$ /tmp/goimports-87312bc3 /tmp/main1.go
package main

import (
        _ "foo2"
        // foo3 has side effects.
        _ "foo1"
        _ "foo3"
)

$ /tmp/goimports-12a7c317 /tmp/main1.go
package main

import (
        _ "foo1"
        _ "foo2" // foo3 has side effects.
        _ "foo3"
)

Valid import comments:

$ cat <<EOF > /tmp/main2.go
package main

import (
        _ "foo2"
       
        _ "foo3" // foo3 has side effects.
        _ "foo1"

)
EOF

$ /tmp/goimports-87312bc3 /tmp/main2.go
package main

import (
        _ "foo2"

        _ "foo1"
        _ "foo3" // foo3 has side effects.
)

$ /tmp/goimports-12a7c317 /tmp/main2.go
package main

import (
        _ "foo1"
        _ "foo2"
        _ "foo3" // foo3 has side effects.
)

More groups:

$ cat <<EOF > /tmp/main3.go
package main

import (
        _ "foo1" // foo1, "std" package
        _ "github.com/bar/foo1" // github.com/bar/foo1, third-party package
        _ "local/foo1" // local/foo1, local package
        
        _ "appengine"
       
        _ "foo2"
        // local/foo3 comment
        _ "local/foo3"
        _ "github.com/bar/foo2"

)
EOF

$ /tmp/goimports-87312bc3 -local local /tmp/main3.go
package main

import (
        _ "foo1" // foo1, "std" package

        _ "github.com/bar/foo1" // github.com/bar/foo1, third-party package

        _ "local/foo1" // local/foo1, local package

        _ "appengine"

        _ "foo2"
        // local/foo3 comment
        _ "github.com/bar/foo2"

        _ "local/foo3"
)

$ /tmp/goimports-12a7c317 -local local /tmp/main3.go 
package main

import (
        _ "foo1" // foo1, "std" package
        _ "foo2" // local/foo3 comment

        _ "github.com/bar/foo1" // github.com/bar/foo1, third-party package
        _ "github.com/bar/foo2"

        _ "appengine"

        _ "local/foo1" // local/foo1, local package
        _ "local/foo3"
)

You can see, the mentioned commit(#20818) just ensures grouping of imports(std, third-party, appegine, local) and it's not the root problem for this issue.

As I understand, for the ast parser a valid import comment shoud follow the spec on the same line. Other comments goimports tries to assign to some import spec.

@lopezator

This comment has been minimized.

Copy link

commented Oct 19, 2018

Hello @saheienko

I went over your examples and your are right, both of them fail given that input.

However, in our specific usecases golang/tools@87312bc doesn't break, and golang/tools@12a7c31 indeed does.

Supposing we have both binaries built and in place as you explain in #28200 (comment)

Lets try this:

$ cat << EOF > /tmp/main.go
package test

import (
	"time"
	"fmt"

	// foo3 has side effects.
	_ "foo3"
	// foo2 has side effects.
	_ "foo2"

	"github.com/go-kit/kit/log"
)

func main() {
	fmt.Println("")
	log.Timestamp(time.Now)
}
EOF
 
$ /tmp/goimports-87312bc3 /tmp/main.go | tee /tmp/main-87312bc3.go
package test

import (
	"fmt"
	"time"

	// foo3 has side effects.
	_ "foo3"
	// foo2 has side effects.
	_ "foo2"

	"github.com/go-kit/kit/log"
)

func main() {
	fmt.Println("")
	log.Timestamp(time.Now)
}

$ /tmp/goimports-12a7c317 /tmp/main.go | tee /tmp/main-12a7c317.go
package test

import (
	"fmt" // foo3 has side effects.
	_ "foo2"
	_ "foo3" // foo2 has side effects.
	"time"

	"github.com/go-kit/kit/log"
)

func main() {
	fmt.Println("")
	log.Timestamp(time.Now)
}

When using golang/tools@87312bc and empty "_" imports, it's easy to guess that you only have to take care of doing a specific group for it/them, goimports will not break anything.

Unfortunalety with golang/tools@12a7c31 isn't that clear IMHO what you should do, and what should be the expect as output.

If you run golint against both outputs:

$ golint /tmp/main-87312bc3.go

Gives no output, passing the check.

$ golint /tmp/main-12a7c317.go
/tmp/main-12a7c317.go:5:2: a blank import should be only in a main or test package, or have a comment justifying it

Fails.

Not saying your commit is a problem by itself, but definitively breaks the way we used to work with goimports.

Until a reasonable explanation/fix is given we are pinning to golang/tools@87312bc

Maybe is more of a documentation problem and there should be a document explaining what we should give to goimports as input and what we should expect as output.

cc/ @benesch @bradfitz

@gopherbot

This comment has been minimized.

Copy link

commented Oct 20, 2018

Change https://golang.org/cl/143657 mentions this issue: imports: recognise ImportSpec.Doc comments

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