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

[vasi-stripe]

I'm working on a tool to fix up non-conventional or incorrect imports grouping/ordering, but I'd much rather have this be part of goimports. Would it be welcome?

What goimports does now

When goimports adds or removes an import, it heuristically attempts to find a good place for it. So if I add an os.Open() call, and my imports already look like this:

import (
    "context"
    "testing"

    "github.com/Sirupsen/logrus"
)

Then goimports will put "os" in the proper spot, beween "context" and "testing". That's great!

What goimports does not do

If my imports are not grouped and sorted in the conventional way, goimports will not usually fix the problem. For example, if I was using the old context package, my imports might look like this:

import (
    "testing"

    "github.com/Sirupsen/logrus"
    "golang.org/x/net/context"
)

When I edit the context import to the new package, "context", I have this:

import (
    "testing"

    "github.com/Sirupsen/logrus"
    "context"
)

Running goimports will notice that logrus and context don't make a proper group, so it will separate them:

import (
    "testing"

    "context"

    "github.com/Sirupsen/logrus"
)

But this isn't what we really want. Ideally, it would just re-group and re-order all the imports, to yield:

import (
    "context"
    "testing"

    "github.com/Sirupsen/logrus"
)

Work so far

I have a tool that will globally reorder imports according to the grouping rules, including the associated doc-comments; see below. I could also develop similar funcitonality inside goimports.

My tool is not nearly perfect: It doesn't handle multiple import declarations; it doesn't know about import "C". It's not even clear what should happen to random non-doc-comments inside the import block, eg:

import (
    "github.com/Sirupsen/logrus"

    // When we re-order, what happens to this comment?

    "testing"
)

Automatically grouping and ordering would also be a behavioral change for goimports, so it would have to be behind a flag for now.

[bradfitz]

Improving goimports is welcome.

[meggamind]

Hey can I please pick this issue ? I would need a bit guidance though.

[vasi-stripe]

Here's what I did before: https://github.com/vasi-stripe/gogroup . Note that this is not exactly what we want in goimports, it still has a number of issues.

[vasi-stripe]

So it turns out that goimports already does this, but only in limited cases. If a file is missing an import, and has multiple import decls, they get all merged together and then sorted. The merging seems to lose associated comments, but otherwise this should be possible to repurpose.

See https://github.com/golang/tools/blob/master/go/ast/astutil/imports.go#L151-L173

[wedneyyuri]

Hi @vasi-stripe, in my opinion the goimports should have an option to remove all blank spaces before formatting. Today, if I have imports unordered and separated by spaces the tool doesn't work as expected.

Example (goimports will not do anything):

import (
    "strings"

    "github.com/pkg/errors"

    "net/http"
)

Expected:

import (        
    "net/http"
        "strings"
        "github.com/pkg/errors"
)

[bradfitz]

@wedneyyuri, I disagree with two things there:

(1) "goimports should have an option". We don't like options in code formatters. (2) that you expect no blank line between the standard library imports and github stuff. goimports is opinionated in that regard.

I agree that the original bug report at the top is a bug, though. It should try to pack things nicely.

[aaronbee]

I'd like to extend this proposal with a specific grouping of imports: stdlib, VCS repo-local, non-local.

For example,

package foo // github.com/org/repo1/foo

import (
    "context"
    "os"

    "github.com/org/repo1/foo/bar" // Same VCS repo as this file
    "github.com/org/repo1/foo/baz"

    "github.com/org/repo2/quux" // Different VCS repo
    "github.com/thirdparty/repo"
)

I have written a tool, importsort, that achieves this by finding the VCS root directory for the file that it is processing, then it looks at the imports in the file and checks if a path is prefixed by the VCS root directory import path.

[aaronbee]

I just learned that goimports does allow grouping imports with the -local flag, but does so in the order of stdlib, non-local, local. That ordering is fine.

My proposal is determining local/non-local programmatically by looking at the VCS root directory.

[ash2k]

I think ideally ordering should only depend on contents of the imports block. Reasons why not VCS/etc:

• vendor vs GOPATH vs GOROOT vs ... Package may reside in various places and it may or may not be under VCS control. Different users of the same project may have different setup of their dependencies and goimports must (IMO) produce exactly the same output for them so it cannot rely on assumptions like this. Also some other tools store packages in completely different places - Google Bazel is an example (see Working with external dependencies).
• I don't know how people use it but I personally like to run goimports before tests/builds to remove unnecessary imports, etc. The more work goimports needs to do to format the imports the less viable this option is. Doing more disk I/O is not great.

Why not something simple like this

import (
    "context" // Std lib goest first
    "os"

    "github.com/org/repo1/foo/bar" // Everything else goes second after one blank line
    "github.com/org/repo1/foo/baz"
    "github.com/org/repo2/quux" // Comment X
    "github.com/thirdparty/repo"
)

I would really like goimports to be as deterministic and as opinionated as possible. Like gofmt - nobody's favourite formatting but one of the most liked features. During PR reviews I've never had to say anything about formatting but many times about the "wrong" grouping/ordering of imports. Would be great to solve this problem once and for everybody.

[dahankzter]

Is anyone working on this? The algos for groupings is one thing but repairing holes in the simples possible stdlib/otherlibs can perhaps be done first?

[bradfitz]

@dahankzter, I don't know of anybody working on this. They would generally announce so here.

[gopherbot]

Change https://golang.org/cl/116795 mentions this issue: x/tools/cmd/goimports: support repairing import grouping/ordering

[lhecker]

@bradfitz Is anything blocking the above "pull request"? I've been using Serhii's changeset for a while now and IMO it makes working with goimports quite significantly more pleasant to use. I'd be nice if we could merge the changes sooner than later. 🙂

[bradfitz]

@lhecker, vacation is. I'm back in a week. I'll get to it at some point after then.

[lhecker]

@bradfitz Oh I'm sorry. Don't bother with my silly question - Please do enjoy your vacation instead. 😄 We'll surely get around to this at some point either way. 🙂

[akshayjshah]

Thanks for taking this on, @lhecker - I was just looking at how to integrate exactly this functionality into goimports.

[saheienko]

I'm using this too. There were some cases when it doesn't work properly, so it's better to use the last patch.

[ashi009]

Given the issues (#27673, #28200) introduced by this CL, please roll golang/tools@12a7c31 back.

[ashi009]

IMHO a tool pretending to be smart and doing wrong things should be avoided.

We actually use a 4-group layout: standard, local, third-party, and protos.

And for some cases, we use a 5th group for packages that are imported for side-effects.

This CL doesn't really make the already organized code more readable, but only helping the real chaos ones -- and it's not capable to tell if such regroup is actually needed.

[saheienko]

@bradfitz please, revert this CL. Needs to handle the comments issue and clarify what import groups should be on the output.

@ashi009 Do you manage import grouping manually?

[ashi009]

@saheienko Yep. goimports will do the initial sweep, then we'll give it a final touch. This grouping layout is covered by our version of go review comments.

[gopherbot]

Change https://golang.org/cl/144339 mentions this issue: Revert "imports: support repairing import grouping/ordering"

[bradfitz]

@saheienko, the revert no longer merges cleanly due to changes made since then.

Could you send a manual revert?

[bradfitz]

Nevermind, I rebased it. It's now reverted.

[shuyuwu]

@ashi009 Maybe we can allow goimports to give people the right to sort imports in both the old way and the new way. By doing so, everyone's need can be satisfied.

[ash2k]

gofmt does it one way and is not configurable and people like it that way. Why goimports should work differently? In fact, why imports formatting is not part of gofmt?

[bradfitz]

We are not adding style preference knobs to goimports.

[nhooyr]

@bradfitz if I sent a change for determining the local import path with the module path, sorting all imports as the reverted commit does and also fixed the comment issues, is there a high chance of it being merged?

[nhooyr]

For anyone who wants this now and doesn't really care that the comments get misplaced, feel free to use go.coder.com/go-tools/cmd/goimports. That fork will be kept up to date for Coder's usage.

[ernado]

There is already a "knob":

  -local string
        put imports beginning with this string after 3rd-party packages; comma-separated list

But it is pretty useless without full re-grouping :(

[alecbz]

Is there any prescriptive behavior that goimports could adopt here that doesn't break the use-cases above?

One idea that addresses the original concern is to maybe reorganize imports more prescriptively from any import block that's "wrong" (contains imports from multiple sources) but leave any other blocks alone. So, e.g.:

1. import (
       "testing"
   
       "github.com/Sirupsen/logrus"
       "context"
   )

   Second block contains a mix of stdlib and 3rdparty, and should be broken up. goimports sees that there's an existing stdlib block, so it moves "context" there:

   import (
       "context"
       "testing"
   
       "github.com/Sirupsen/logrus"
   )

2. import (
       "testing"
   
       "github.com/Sirupsen/logrus"
   
       "github.com/org/repo1/foo/bar"
   )

   No blocks contain a mix of sources: nothing to do.

[alecbz]

Though honestly, I would prefer a tool that was more prescriptive.

I see the benefits to not having knobs (things are more similar across different codebases, no need to argue about the values of the knobs), but by both not being prescriptive by default and not allowing for prescriptive knobs, we're back to needing humans to catch style violations in code review, needing authors to fix things manually, and maintaining a style guide, all of which feel worse than having knobs.

And as @ernado points out, there already is a knob fairly related to this functionality.

Spitballing: instead of a -local knob, maybe there could be a more generic knob that takes some specification of what sections are desired? Something like -sections stdlib,3rdparty,gitlab.com/mycompany/repo,gitlab.com/mycompany/repo/go/genproto

[gopherbot]

Change https://golang.org/cl/321409 mentions this issue: internal/imports: merge mergeable import groups

[thockin]

I'm +1 on being more prescriptive and making goimports more aggressively normalize import blocks. There should be one true way to do it, including whitespace.

If we really need to add a knob, I'd want it to be IN the codebase (per file or per module) rather than a flag to the binary.

IMO, running goimports with no special args should be the canonical guidance to import ordering.

[rinchsan]

As a workaround, I am using https://github.com/rinchsan/gosimports as an alternative goimports to make an import block style more consistent, but I am so happy if goimports gets more prescriptive and say goodbye to https://github.com/rinchsan/gosimports .

[pete-woods]

I've never seen so many engineers agree with each other! We at CircleCI are also planning to switch away from this tool to gosimports because it actually enforces the 3 sections, and we're wasting too much time manually correcting the results of goimports

[daixiang0]

GCI can do this and it is built-in in golangci-lint.

[yujiachen-y]

GCI can do this and it is built-in in golangci-lint.

@daixiang0 But GCI does not support multiple prefixes in one group https://github.com/daixiang0/gci/issues/107

[daixiang0]

@yjc567 you can implement it in GCI.

[NonLogicalDev]

I have recently implemented a tool and framework for programmatically managing imports ordering that is

• Deterministic
• Handles tricky whitespace around imports gracefully
• Handles comments gracefully
• Handles user import groupings gracefully
• Highly configurable and easy to extend and build on top of
• Compatible with https://pkg.go.dev/golang.org/x/tools/go/analysis
• Built directly on top of golang AST

https://github.com/NonLogicalDev/gofancyimports