proposal: Remove redundant package aliases from import blocks #296
Conversation
I noticed that
```go
package main
import (
foo "gofumpt-ex/foo"
)
func main() {
foo.Foo("goodbye, world!")
}
```
wasn't being simplified to
```go
package main
import (
"gofumpt-ex/foo"
)
func main() {
foo.Foo("goodbye, world!")
}
```
when the package name of "gofumpt-ex/foo" is "foo". This commit adds
handling for this.
| // If we import a package with an alias (i.e., if spec.Name is non-nil), and | ||
| // if the alias is the same as the package's name, then we remove the alias. | ||
| if spec.Name != nil { | ||
| packages, err := packages.Load(&packages.Config{Mode: packages.NeedName}, path) |
There was a problem hiding this comment.
Unfortunately I think this is kind of a deal breaker - it would slow down formatting significantly. Right now the formatting is syntax-based alone, it doesn't load type information or any package information. All it looks at is what the main module path is, for the sake of grouping imports. gofmt has the same limitation, also for the sake of performance.
For example, I regularly run gofumpt -l -w . on large projects, so if that has to load every package in a named import for every file, that would likely mean hundreds of calls to go list, as well as orders of magnitude more opened files and syscalls.
That said, I agree that this is a good rule to enforce. Perhaps it should go into a tool that already loads type information - the two obvious candidates that come to mind are gopls (since it already has an "organize imports" code action akin to the old goimports) and staticcheck (since it has similar rules already like https://staticcheck.dev/docs/checks#ST1019).
There was a problem hiding this comment.
You're right: it does seem to slow things down quite a lot.
The monorepo I work in has around 1.1m lines of Go. Before the change, it took ~9s to act on the whole codebase; after, it takes ~27s. It's a pity: I try to be in the business of making things faster, not slower.
I'll see about contributing a new check to staticcheck.
Thanks for the quick response!
There was a problem hiding this comment.
I should note that you could make this faster in gofumpt by e.g. caching the calls to go/packages, or calling it upfront on all packages to format to reduce the number of execs to go list. But I still think that would be a significant slow-down, and going against the gofmt philosophy of formatting based on syntax alone.
For instance, we aim to provide a Go API compatible with https://pkg.go.dev/go/format. Such an API is self-contained, it doesn't require type information nor the ability to call go list.
My comment aside, thanks for contributing, and for being understanding :) |
|
I've gone ahead and put together a proposed ST1024 check here: dominikh/go-tools#1497. Thanks again for the quick response (and, on a totally unrelated note, for the work on encoding/json/v2—I'm only on the sidelines, watching, but it's exciting to see). |
adamroyjones
changed the title
I noticed that
wasn't being simplified to
when the package name of "gofumpt-ex/foo" is "foo".
I thought it might be a nice addition, and I wanted to have a bit of a poke around in gofumpt, and so I've this branch.
If this is an unwelcome change, please don't be shy in saying so.