Repository: go
Issue: x/tools: goimports and gopls don't order new import groups of 'local' dependencies correctly in some situations
Labels: NeedsInvestigation,Tools

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](https://github.com/cespare/misc/tree/main/importgroup)):

`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`](https://pkg.go.dev/golang.org/x/tools/go/ast/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.