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: goimports and gopls don't order new import groups of 'local' dependencies correctly in some situations #51969

Open
cespare opened this issue Mar 26, 2022 · 1 comment
Labels
NeedsInvestigation Tools
Milestone

Comments

@cespare
Copy link
Contributor

@cespare cespare commented Mar 26, 2022

This issue is related to #20818 (but is much more specific/limited). It's also similar to this issue I previously filed: #19190.

Background

This is a longstanding issue we've faced at my company. The problem occurs when running goimports or when gopls automatically adds imports.

Goimports and gopls have a notion of a "local" name prefix. The idea is that you can say that there are some imports which should be grouped separately from normal third-party imports. We set local to be the name of our module. Therefore, we have three groups of imports:

  1. standard library
  2. third-party
  3. other packages in the module

The groups should be listed in this order, and the documentation for the "local" feature says as much. For instance, to quote goimports -h:

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

What did you do?

To demo the issue, consider this minimal tree (or see a GitHub repo):

go.mod:

module mycorp

go 1.17

require golang.org/x/tools v0.1.10

d/d.go:

package d

const D = 3

main/main.go:

package main

import (
	"fmt"

	"golang.org/x/tools/container/intsets"

	"mycorp/d"
)

func main() {
	_ = intsets.MaxInt
	_ = d.D
	_ = fmt.Print
}

For this demo, we are running goimports with -local mycorp/. (Equivalently, gopls can have the option local = "mycorp/".)

Note that main/main.go has three import groups (each with one import), and they are listed in the correct order:

  1. "fmt"
  2. "golang.org/x/tools/container/intsets"
  3. "mycorp/d"

If I delete all the imports and rerun goimports, the groups are created correctly (as above).

If I delete the "fmt" import and rerun goimports, the groups are created correctly.

If I delete the "golang.org/x/tools/container/intsets" import and rerun goimports, the groups are created correctly.

However, if I delete the "mycorp/d" import and rerun goimports, then I get a different result:

package main

import (
	"fmt"

	"mycorp/d"

	"golang.org/x/tools/container/intsets"
)

func main() {
	_ = intsets.MaxInt
	_ = d.D
	_ = fmt.Print
}

Note that the "mycorp/d" import group is before the "golang.org/x/tools/container/intsets" import group.

What did you expect to see?

I expected that if I delete the "mycorp/d" import group and rerun goimports, I would get the correct import group order:

package main

import (
	"fmt"

	"golang.org/x/tools/container/intsets"

	"mycorp/d"
)

func main() {
	_ = intsets.MaxInt
	_ = d.D
	_ = fmt.Print
}

Discussion

I dug into the code and I understand why this is happening.

There is code in golang.org/x/tools/internal/imports (importGroup) which has a notion of import groups. It assigns a "group number" to each import and that looks correct to me:

  • "fmt" -> group 0
  • "golang.org/x/tools/container/intsets" -> group 1 (first import path component has a dot)
  • "mycorp/d" -> group 3 (matches local prefix)

In most cases, if goimports/gopls is responsible for adding all the imports, it will create the groups in that order. However, this issue is an example of a case where it does not do so.

The problem is that astutil.AddNamedImport does not have a notion of local import groups. It only uses "does the first import path component have a dot?" as the heuristic for whether an import is in the stdlib. Goimports/gopls first use astutil.AddNamedImport to add the imports, then sort the import groups, and then break apart groups by group number (if necessary).

So in the case where we first delete "golang.org/x/tools/container/intsets", we have

import (
        "fmt"

        "mycorp/d"
)

Then after astutil.AddNamedImport runs, we have


import (
        "fmt"

        "golang.org/x/tools/container/intsets"
        "mycorp/d"
)

And then after sorting and group splitting, we have

import (
        "fmt"

        "golang.org/x/tools/container/intsets"

        "mycorp/d"
)

However, if we start by deleting "mycorp/d", then we have

import (
        "fmt"

        "golang.org/x/tools/container/intsets"
)

After running astutil.AddNamedImport, we get:

import (
        "fmt"
        "mycorp/d"

        "golang.org/x/tools/container/intsets"
)

(Note how the "mycorp/d" import is grouped with the stdlib imports.)

Then after sorting and group splitting, we get the incorrect result:

import (
        "fmt"

        "mycorp/d"

        "golang.org/x/tools/container/intsets"
)

One fix could be to have a way of telling astutil.AddNamedImport that the new import is third-party rather than using the dot heuristic. (This would probably need to be a new function.)

Alternatively, a more drastic solution to #20818, where goimports/gopls discards the original grouping and sorts/groups the entire set of imports would also work.

@gopherbot gopherbot added the Tools label Mar 26, 2022
@gopherbot gopherbot added this to the Unreleased milestone Mar 26, 2022
@cespare cespare changed the title x/tools: goimports/gopls don't order new import groups of 'local' dependencies correctly in some situations x/tools: goimports and gopls don't order new import groups of 'local' dependencies correctly in some situations Mar 26, 2022
@cherrymui cherrymui added the NeedsInvestigation label Apr 5, 2022
@cherrymui
Copy link
Member

@cherrymui cherrymui commented Apr 5, 2022

cc @heschi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Tools
Projects
None yet
Development

No branches or pull requests

3 participants