x/tools/cmd/goimports: interspersed comments can be associated with the wrong import #28200

benesch · 2018-10-14T22:48:49Z

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 (5e66757).

The text was updated successfully, but these errors were encountered:

lopezator · 2018-10-15T13:18:07Z

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 · 2018-10-16T00:39:12Z

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

benesch · 2018-10-16T01:07:38Z

/cc @bradfitz

bradfitz · 2018-10-16T19:17:01Z

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

bradfitz · 2018-10-16T19:17:37Z

(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 · 2018-10-16T20:08:02Z

@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 · 2018-10-18T15:23:58Z

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

$ 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 · 2018-10-18T20:19:19Z

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 · 2018-10-19T11:00:25Z

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 · 2018-10-20T17:34:42Z

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

heschi · 2019-11-07T19:25:24Z

The CL that was the cause of this bug was rolled back.

gopherbot added this to the Unreleased milestone Oct 14, 2018

ttapjinda mentioned this issue Oct 15, 2018

fix all missing arguments/formatting directives elastic/beats#8612

Closed

hklai mentioned this issue Oct 16, 2018

lint fixing. istio/istio#9345

Closed

ymesika mentioned this issue Oct 19, 2018

Temporarily disable the format linter istio/istio#9424

Merged

ashi009 mentioned this issue Oct 24, 2018

x/tools/cmd/goimports: support repairing import grouping/ordering #20818

Open

gopherbot added the Tools label Sep 12, 2019

heschi closed this as completed Nov 7, 2019

golang locked and limited conversation to collaborators Nov 6, 2020

gopherbot added the FrozenDueToAge label Nov 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

x/tools/cmd/goimports: interspersed comments can be associated with the wrong import #28200

x/tools/cmd/goimports: interspersed comments can be associated with the wrong import #28200

benesch commented Oct 14, 2018 •

edited

Loading

lopezator commented Oct 15, 2018

ostromart commented Oct 16, 2018

benesch commented Oct 16, 2018

bradfitz commented Oct 16, 2018

bradfitz commented Oct 16, 2018

saheienko commented Oct 16, 2018

glerchundi commented Oct 18, 2018 •

edited

Loading

saheienko commented Oct 18, 2018 •

edited

Loading

lopezator commented Oct 19, 2018 •

edited

Loading

gopherbot commented Oct 20, 2018

heschi commented Nov 7, 2019

x/tools/cmd/goimports: interspersed comments can be associated with the wrong import #28200

x/tools/cmd/goimports: interspersed comments can be associated with the wrong import #28200

Comments

benesch commented Oct 14, 2018 • edited Loading

lopezator commented Oct 15, 2018

ostromart commented Oct 16, 2018

benesch commented Oct 16, 2018

bradfitz commented Oct 16, 2018

bradfitz commented Oct 16, 2018

saheienko commented Oct 16, 2018

glerchundi commented Oct 18, 2018 • edited Loading

saheienko commented Oct 18, 2018 • edited Loading

lopezator commented Oct 19, 2018 • edited Loading

gopherbot commented Oct 20, 2018

heschi commented Nov 7, 2019

benesch commented Oct 14, 2018 •

edited

Loading

glerchundi commented Oct 18, 2018 •

edited

Loading

saheienko commented Oct 18, 2018 •

edited

Loading

lopezator commented Oct 19, 2018 •

edited

Loading