x/tools/cmd/goimports: -local adds local packages before external packages

[rayjcwu]

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

$ go version
go version go1.11.2 darwin/amd64

Does this issue reproduce with the latest release?

Yes

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

go env Output

$ go env
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/raywu/Library/Caches/go-build"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/raywu/go:/Users/raywu"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.11.2/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.11.2/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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 -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/v8/q5brnv35699835dr_mt01t0m0000gn/T/go-build432413042=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Use goimports to add missing timeutil import, which is our local package.

package main

import (
        "fmt"

        "github.com/cespare/wait"
)

func main() {
        fmt.Println("asdf")
        var wg wait.Group
        wg.Wait()
        timeutil.MustParseRFC3339("2019-01-01T00:00:00Z")
}

What did you expect to see?

▶ cat abc.go | ~/goimports -local liftoff/
package main

import (
	"fmt"

	"github.com/cespare/wait"

	"liftoff/go/timeutil"
)

func main() {
	fmt.Println("asdf")
	var wg wait.Group
	wg.Wait()
	timeutil.MustParseRFC3339("2019-01-01T00:00:00Z")
}

What did you see instead?

▶ cat abc.go | ~/goimports -local liftoff/
package main

import (
	"fmt"

	"liftoff/go/timeutil"

	"github.com/cespare/wait"
)

func main() {
	fmt.Println("asdf")
	var wg wait.Group
	wg.Wait()
	timeutil.MustParseRFC3339("2019-01-01T00:00:00Z")
}

[agnivade]

@bradfitz @josharian

[bradfitz]

This seems arbitrary either way, no? It chose something, so that should be fine, just like the rest of the spirit of gofmt?

[rayjcwu]

Help message said "put imports beginning with this string after 3rd-party packages; comma-separated list" though. Also if the import block is

import (
        "fmt"
        "github.com/cespare/wait"
)

, then goimports will change it to

import (
	"fmt"

	"github.com/cespare/wait"

	"liftoff/go/timeutil"
)

[bradfitz]

Okay, if it contradicts the docs, that might be reason enough to change it.

I don't object if you send a CL. Others might argue it's better to fix the docs, but I'll let them make that case if so.

[af-engineering]

We're seeing this as well. Our CI catches badly formatted imports and they've been failing recently, even though everyone has setup filewatchers in Goland with the -local option. goimports used to do what the docs mentioned (i.e., putting local packages last), so this is a regression.

Specifically, goimports does not reorganize the below:

import (
    "net/http"
    "strings"

    "github.com/local/repo/pkg/test"

    "github.com/pkg/errors"

    "github.com/local/repo/bar"
    "github.com/local/repo/foo"
)

[rayjcwu]

@af-engineering for Goland you can fix that by using goland settings https://youtrack.jetbrains.com/issue/GO-7321

[guelfey]

Looked into this a bit since I tripped over this as well. The fundamental issue here seems to be that goimports doesn't reorder existing imports across empty lines (only if there are e.g. external and local imports in the same "group", additional empty lines are inserted). This is also explicitly tested against in TestSimpleCases/new_section_for_all_kinds_of_imports. IMO just always reordering into three groups (stdlib, external and local), disregarding existing separation lines, would be clearer. Would that be an acceptable change or is this too opinionated?