gofumpt and gofumports regroup imports that gofmt/goimports leave alone

[cespare]

When I run gofumpt or gofumports over our code, the diffs are huge because it regroups all the imports. For instance:

@@ -7,11 +7,10 @@ import (
        "encoding/binary"
        "errors"
        "hash"
+       "liftoff/go/bitset"
        "math"

        "github.com/cespare/xxhash"
-
-       "liftoff/go/bitset"
 )

This happens with gofumpt and with gofumports executed with

gofumpt -w .
gofumports -local 'liftoff/' -w .

but not with gofmt or goimports executed with

gofmt -w .
goimports -local 'liftoff/' -w .

[mvdan]

This is because it assumes imports with no domain extension belong in the standard library. I'm pretty sure this is the same rule that the go tool applies; any import with no TLD might be broken at any point.

[cespare]

I'm pretty sure this is the same rule that the go tool applies; any import with no TLD might be broken at any point.

Citation?

[mvdan]

Uh, I'm not sure! I'll get back to you on this :)

[mvdan]

So it turns out there's no official docs on this. I only knew this from interactions with Bryan on GitHub and Slack. I've opened golang/go#32819.

I'll keep this issue open for now, but I'm not sure if this can be done any other way in the future. Hard-coding a list of std is tedious and sloppy, and calling go list std is (relatively) expensive.

[cespare]

I suggest doing whatever gofmt/goimports do today.

FWIW we've used this scheme for many years and have had zero tooling problems due to it. We might change our paths when we move to modules, but for now I see no point in making up faking TLD-based import paths for packages that aren't publicly accessible on the internet anyway.

[mvdan]

I am indeed following what goimports does:

$ cat f.go
package p

import (
        "io"
        "might/be/standard"
        "foo.com/bar"
)

var (
        _ = io.Var
        _ = standard.Var
        _ = bar.Var
)
$ goimports f.go
package p

import (
        "io"
        "might/be/standard"

        "foo.com/bar"
)

var (
        _ = io.Var
        _ = standard.Var
        _ = bar.Var
)

You're only seeing problems here because gofumpt uses the distinction in one more way - to join the std group. goimports only uses the distinction to separate std imports from non-std ones.

[cespare]

I see. I didn't realize that gofumpt tries to do extra stuff with imports since that wasn't in the list of features.

[mvdan]

You're right! I must have forgotten to add that bit.

[cespare]

Anyway, I think that the fix here is indeed to know what is and is not in std rather than guess based on this heuristic.

But even without doing that, ISTM that if I provide gofumports with -local liftoff/, as I did, that should take precedence over the std heuristic.

[mvdan]

I think that the fix here is indeed to know what is and is not in std

I just don't think that's practical in the long term. Even if I take the time to keep the list up to date with time, what if a user has a version of the tool that's too old or too new? It could lead to surprising behavior.

In contrast, this heuristic is simple and shouldn't surprise anyone, if the doc issue above is fixed. If anyone wants to stick to these grey-area import paths for the time being, that's fine; they know what they are doing.

that should take precedence over the std heuristic.

That seems reasonable. Just bear in mind that only gofumports has the flag, so gofumpt would still annoy you. So it doesn't seem like a long-term fix to me, unless you're happy with only using gofumports.

[mvdan]

The upstream issue was labeled NeedsFix, so it looks like my thinking was correct. I'll close this as "works as intended" for now. Thanks for bringing it up though, as the documentation was lacking in both this repo and upstream.

[mvdan]

FYI, see #187 (comment); I'm currently thinking we could do better for everyone by looking at the main module's path.