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.

Sign up for GitHub

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

Jump to bottom

proposal: Remove redundant package aliases from import blocks #296

Closed
wants to merge 1 commit into from
Closed

proposal: Remove redundant package aliases from import blocks #296

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,24 @@ import (

</details>

**Redundant package aliases are removed**

<details><summary><i>Example</i></summary>

```go
import (
math "math"
)
```

```go
import (
"math"
)
```

</details>

**Short case clauses should take a single line**

<details><summary><i>Example</i></summary>
Expand Down
31 changes: 23 additions & 8 deletions format/format.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"github.com/google/go-cmp/cmp"
"golang.org/x/mod/semver"
"golang.org/x/tools/go/ast/astutil"
"golang.org/x/tools/go/packages"

"mvdan.cc/gofumpt/internal/govendor/go/format"
"mvdan.cc/gofumpt/internal/version"
Expand Down Expand Up @@ -479,7 +480,7 @@ func (f *fumpter) applyPre(c *astutil.Cursor) {

case *ast.GenDecl:
if node.Tok == token.IMPORT && node.Lparen.IsValid() {
f.joinStdImports(node)
f.tidyImports(node)
}

// Single var declarations shouldn't use parentheses, unless
Expand Down Expand Up @@ -930,9 +931,10 @@ func isCgoImport(decl *ast.GenDecl) bool {
return v == "C"
}

// joinStdImports ensures that all standard library imports are together and at
// the top of the imports list.
func (f *fumpter) joinStdImports(d *ast.GenDecl) {
// tidyImports ensures that:
// - all standard library imports are together and at the top of the imports list, and
// - any redundant package aliases are removed.
func (f *fumpter) tidyImports(d *ast.GenDecl) {
var std, other []ast.Spec
firstGroup := true
lastEnd := d.Pos()
Expand All @@ -956,6 +958,23 @@ func (f *fumpter) joinStdImports(d *ast.GenDecl) {

for i, spec := range d.Specs {
spec := spec.(*ast.ImportSpec)
path, err := strconv.Unquote(spec.Path.Value)
if err != nil {
panic(err) // should never error
}

// 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)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if err != nil || len(packages) != 1 {
panic(fmt.Sprintf("loading the package %q: %v", path, err)) // should never error
}
if packages[0].Name == spec.Name.Name {
spec.Name = nil
}
}

if coms := f.commentsBetween(lastEnd, spec.Pos()); len(coms) > 0 {
lastEnd = coms[len(coms)-1].End()
}
Expand All @@ -966,10 +985,6 @@ func (f *fumpter) joinStdImports(d *ast.GenDecl) {
lastEnd = spec.End()
}

path, err := strconv.Unquote(spec.Path.Value)
if err != nil {
panic(err) // should never error
}
periodIndex := strings.IndexByte(path, '.')
slashIndex := strings.IndexByte(path, '/')
switch {
Expand Down
16 changes: 14 additions & 2 deletions testdata/script/std-imports.txtar
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ import (
import (
"foo.local"
"foo.local/three"
math "math"
maths "math"
)

import (
Expand Down Expand Up @@ -99,6 +99,12 @@ import (
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/yaml"
)

// Redundant package aliases are removed.
import (
filepath "path/filepath"
"time"
)
-- foo.go.golden --
package p

Expand Down Expand Up @@ -135,7 +141,7 @@ import (

// We need to split std vs non-std in this case too.
import (
math "math"
maths "math"

"foo.local"
"foo.local/three"
Expand Down Expand Up @@ -191,3 +197,9 @@ import (

"sigs.k8s.io/yaml"
)

// Redundant package aliases are removed.
import (
"path/filepath"
"time"
)