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:
standard library
third-party
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
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
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.
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:The groups should be listed in this order, and the documentation for the "local" feature says as much. For instance, to quote
goimports -h
:What did you do?
To demo the issue, consider this minimal tree (or see a GitHub repo):
go.mod
:d/d.go
:main/main.go
:For this demo, we are running
goimports
with-local mycorp/
. (Equivalently, gopls can have the optionlocal = "mycorp/"
.)Note that
main/main.go
has three import groups (each with one import), and they are listed in the correct order:"fmt"
"golang.org/x/tools/container/intsets"
"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: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: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 useastutil.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 haveThen after
astutil.AddNamedImport
runs, we haveAnd then after sorting and group splitting, we have
However, if we start by deleting
"mycorp/d"
, then we haveAfter running
astutil.AddNamedImport
, we get:(Note how the
"mycorp/d"
import is grouped with the stdlib imports.)Then after sorting and group splitting, we get the incorrect result:
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.