build/analysis/importsort: init
This adds an analyzer which enforces import sorting as per
CODING_STANDARDS.md. It is not yet enabled in nogo.
This effectively duplicates some logic that is present in goimports.
However, that logic is mostly within an 'internal' package in x/tools,
and would be somewhat tricky to make work within the framework of an
analysis.Analyser (as it expects to also mutate a file). Thus, we
end up writing the analysis logic ourselves. Tests are provided to make
sure this logic doesn't rot away.
We also move some common logic from 'noioutil' to a new 'lib' package,
and implement the last piece of the puzzle there: a code generator to
provide information about the toolchain's stdlib as a map/set.
Change-Id: Ia0f32d6f9122e13117d18ae781d8255c6e3a887d
Reviewed-on: https://review.monogon.dev/c/monogon/+/494
Reviewed-by: Leopold Schabel <leo@nexantic.com>
diff --git a/build/analysis/importsort/BUILD.bazel b/build/analysis/importsort/BUILD.bazel
new file mode 100644
index 0000000..309edfb
--- /dev/null
+++ b/build/analysis/importsort/BUILD.bazel
@@ -0,0 +1,38 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+
+go_library(
+ name = "go_default_library",
+ srcs = [
+ "classify.go",
+ "importsort.go",
+ ],
+ importpath = "source.monogon.dev/build/analysis/importsort",
+ visibility = ["//visibility:public"],
+ deps = [
+ "//build/analysis/lib:go_default_library",
+ "@org_golang_x_tools//go/analysis:go_default_library",
+ ],
+)
+
+go_test(
+ name = "go_default_test",
+ srcs = ["importsort_test.go"],
+ data = glob(["testdata/**"]),
+ embed = [":go_default_library"],
+ embedsrcs = [
+ "testdata/README.md",
+ "testdata/example.com/extlib/extlib.notgo",
+ "testdata/example.com/extlib/foo/foo.notgo",
+ "testdata/source.monogon.dev/dut/mixed_in_group.notgo",
+ "testdata/source.monogon.dev/dut/okay.notgo",
+ "testdata/source.monogon.dev/dut/unsorted_group.notgo",
+ "testdata/source.monogon.dev/dut/wrong_group_order.notgo",
+ "testdata/source.monogon.dev/lib/lib.notgo",
+ "testdata/source.monogon.dev/project/a/a.notgo",
+ "testdata/source.monogon.dev/project/b/b.notgo",
+ ],
+ deps = [
+ "//build/toolbase/gotoolchain:go_default_library",
+ "@org_golang_x_tools//go/analysis/analysistest:go_default_library",
+ ],
+)
diff --git a/build/analysis/importsort/classify.go b/build/analysis/importsort/classify.go
new file mode 100644
index 0000000..dc5c45d
--- /dev/null
+++ b/build/analysis/importsort/classify.go
@@ -0,0 +1,54 @@
+package importsort
+
+import (
+ "strings"
+
+ alib "source.monogon.dev/build/analysis/lib"
+)
+
+// groupClass is the 'class' of a given import path or import group.
+type groupClass string
+
+const (
+ // groupClassMixed are import group that contain multiple different classes of
+ // import paths.
+ groupClassMixed = "mixed"
+ // groupClassStdlib is an import path or group that contains only Go standard
+ // library imports.
+ groupClassStdlib = "stdlib"
+ // groupClassGlobal is an import path or group that contains only third-party
+ // packages, ie. all packages that aren't part of stdlib and aren't local to the
+ // Monogon codebase.
+ groupClassGlobal = "global"
+ // groupClassLocal is an import path or group that contains only package that
+ // are local to the Monogon codebase.
+ groupClassLocal = "local"
+)
+
+// classifyImport returns a groupClass for a given import path.
+func classifyImport(path string) groupClass {
+ if alib.StdlibPackages[path] {
+ return groupClassStdlib
+ }
+ if strings.HasPrefix(path, "source.monogon.dev/") {
+ return groupClassLocal
+ }
+ return groupClassGlobal
+}
+
+// classifyImportGroup returns a groupClass for a list of import paths that are
+// part of a single import group.
+func classifyImportGroup(paths []string) groupClass {
+ res := groupClass("")
+ for _, p := range paths {
+ if res == "" {
+ res = classifyImport(p)
+ continue
+ }
+ class := classifyImport(p)
+ if res != class {
+ return groupClassMixed
+ }
+ }
+ return res
+}
diff --git a/build/analysis/importsort/importsort.go b/build/analysis/importsort/importsort.go
new file mode 100644
index 0000000..1b33b3b
--- /dev/null
+++ b/build/analysis/importsort/importsort.go
@@ -0,0 +1,208 @@
+// importsort implements import grouping style checks as per
+// CODING_STANDARDS.md.
+package importsort
+
+import (
+ "fmt"
+ "go/ast"
+ "go/token"
+ "sort"
+ "strconv"
+
+ "golang.org/x/tools/go/analysis"
+
+ alib "source.monogon.dev/build/analysis/lib"
+)
+
+var Analyzer = &analysis.Analyzer{
+ Name: "importsort",
+ Doc: "importsort ensures imports are properly sorted and grouped",
+ Run: run,
+}
+
+func run(p *analysis.Pass) (interface{}, error) {
+ for _, file := range p.Files {
+ if alib.IsGeneratedFile(file) {
+ continue
+ }
+ imp := getImportBlock(p, file)
+ if imp == nil {
+ continue
+ }
+ ensureSorted(p, imp, file, p.Fset)
+ }
+ return nil, nil
+}
+
+// getImportBlock returns the 'main' import block from a file. If more than one
+// import block is present, the first one is returned and an diagnostic is
+// added. If no import blocks are present, nil is returned.
+func getImportBlock(p *analysis.Pass, f *ast.File) *ast.GenDecl {
+ var first *ast.GenDecl
+ for _, decl := range f.Decls {
+ // Only interested in import declarations that aren't CGO import blocks.
+ gen, ok := decl.(*ast.GenDecl)
+ if !ok || gen.Tok != token.IMPORT {
+ continue
+ }
+ for _, spec := range gen.Specs {
+ path, _ := strconv.Unquote(spec.(*ast.ImportSpec).Path.Value)
+ if path == "C" {
+ continue
+ }
+ }
+
+ // Got our first import block.
+ if first == nil {
+ first = gen
+ continue
+ }
+
+ // Second import block. Shouldn't happen.
+ p.Report(analysis.Diagnostic{
+ Pos: gen.Pos(),
+ End: gen.End(),
+ Message: "more than one import block",
+ })
+ }
+ return first
+}
+
+// ensureSorted performs a style pass on a given import block/decl, reporting
+// any issues found.
+func ensureSorted(p *analysis.Pass, gen *ast.GenDecl, f *ast.File, fset *token.FileSet) {
+ // Not a block but a single import - nothing to do here.
+ if !gen.Lparen.IsValid() {
+ return
+ }
+
+ // Find comment lines. These are the only entries allowed in an import block
+ // apart from actual imports and newlines, so we need to know them to figure out
+ // where newlines are, which in turn will be used to split imports into groups.
+ commentLines := make(map[int]bool)
+ for _, comment := range f.Comments {
+ line := fset.Position(comment.Pos()).Line
+ commentLines[line] = true
+ }
+
+ // Split imports into groups, where each group contains a list of indices into
+ // gen.Specs.
+ var groups [][]int
+ var curGroup []int
+ for i, spec := range gen.Specs {
+ line := fset.Position(spec.Pos()).Line
+
+ // First group.
+ if len(curGroup) == 0 {
+ curGroup = []int{i}
+ continue
+ }
+ prevInGroup := curGroup[len(curGroup)-1]
+
+ // Check for difference between the line number of this import and the expected
+ // next line per the last recorded import.
+ expectedNext := fset.Position(gen.Specs[prevInGroup].Pos()).Line + 1
+
+ // No extra lines between this decl and expected decl per previous decl. Still
+ // part of the same group.
+ if line == expectedNext {
+ curGroup = append(curGroup, i)
+ continue
+ }
+
+ // Some lines between previous spec and this spec. If they're not all comments,
+ // this makes a new group.
+ allComments := true
+ for j := expectedNext; j < line; j++ {
+ if !commentLines[j] {
+ allComments = false
+ break
+ }
+ }
+ if !allComments {
+ groups = append(groups, curGroup)
+ curGroup = []int{i}
+ continue
+ }
+
+ // All lines in between were comments. Still part of the same group.
+ curGroup = append(curGroup, i)
+ }
+ // Close last group.
+ if len(curGroup) > 0 {
+ groups = append(groups, curGroup)
+ }
+
+ // This shouldn't happened, but let's just make sure.
+ if len(groups) == 0 {
+ return
+ }
+
+ // Helper function to report a diagnoses on a given group.
+ reportGroup := func(i int, msg string) {
+ group := groups[i]
+ groupStart := gen.Specs[group[0]].Pos()
+ groupEnd := gen.Specs[group[len(group)-1]].End()
+ p.Report(analysis.Diagnostic{
+ Pos: groupStart,
+ End: groupEnd,
+ Message: msg,
+ })
+
+ }
+
+ // Imports are now grouped. Ensure each group individually is sorted. Also use
+ // this pass to classify all groups into kinds (stdlib, global, local).
+ groupClasses := make([]groupClass, len(groups))
+ mixed := false
+ for i, group := range groups {
+ importNames := make([]string, len(group))
+ for i, j := range group {
+ spec := gen.Specs[j]
+ path := spec.(*ast.ImportSpec).Path.Value
+ path, err := strconv.Unquote(path)
+ if err != nil {
+ p.Report(analysis.Diagnostic{
+ Pos: spec.Pos(),
+ End: spec.End(),
+ Message: fmt.Sprintf("could not unquote import: %v", err),
+ })
+ }
+ importNames[i] = path
+ }
+ groupClasses[i] = classifyImportGroup(importNames)
+
+ if !sort.StringsAreSorted(importNames) {
+ reportGroup(i, "imports within group are not sorted")
+ }
+ if groupClasses[i] == groupClassMixed {
+ reportGroup(i, "import classes within group are mixed")
+ mixed = true
+ }
+ }
+
+ // If we had any mixed up group, abort here and let the user figure that out
+ // first.
+ if mixed {
+ return
+ }
+
+ // Ensure group classes are in the right order.
+ seenGlobal := false
+ seenLocal := false
+ for i, class := range groupClasses {
+ switch class {
+ case groupClassStdlib:
+ if seenGlobal || seenLocal {
+ reportGroup(i, "stdlib import group after non-stdlib import group")
+ }
+ case groupClassGlobal:
+ if seenLocal {
+ reportGroup(i, "global import group after local import group")
+ }
+ seenGlobal = true
+ case groupClassLocal:
+ seenLocal = true
+ }
+ }
+}
diff --git a/build/analysis/importsort/importsort_test.go b/build/analysis/importsort/importsort_test.go
new file mode 100644
index 0000000..723e7d2
--- /dev/null
+++ b/build/analysis/importsort/importsort_test.go
@@ -0,0 +1,73 @@
+package importsort
+
+import (
+ "embed"
+ "io/fs"
+ "os"
+ "path/filepath"
+ "strings"
+ "testing"
+
+ "golang.org/x/tools/go/analysis/analysistest"
+
+ "source.monogon.dev/build/toolbase/gotoolchain"
+)
+
+//go:embed testdata/*
+var testdata embed.FS
+
+func init() {
+ // analysistest uses x/go/packages which in turns uses runtime.GOROOT().
+ // runtime.GOROOT itself is neutered by rules_go for hermeticity. We provide our
+ // own GOROOT that we get from gotoolchain (which is still hermetic, but depends
+ // on runfiles). However, for runtime.GOROOT to pick it up, this env var must be
+ // set in init().
+ os.Setenv("GOROOT", gotoolchain.Root)
+}
+
+func TestImportsort(t *testing.T) {
+ // Add `go` to PATH for x/go/packages.
+ os.Setenv("PATH", filepath.Dir(gotoolchain.Go))
+
+ // Make an empty GOCACHE for x/go/packages.
+ gocache, err := os.MkdirTemp("/tmp", "gocache")
+ if err != nil {
+ panic(err)
+ }
+ defer os.RemoveAll(gocache)
+ os.Setenv("GOCACHE", gocache)
+
+ // Make an empty GOPATH for x/go/packages.
+ gopath, err := os.MkdirTemp("/tmp", "gopath")
+ if err != nil {
+ panic(err)
+ }
+ defer os.RemoveAll(gopath)
+ os.Setenv("GOPATH", gopath)
+
+ // Convert testdata from an fs.FS to a path->contents map as expected by
+ // analysistest.WriteFiles, rewriting paths to build a correct GOPATH-like
+ // layout.
+ filemap := make(map[string]string)
+ fs.WalkDir(testdata, ".", func(path string, d fs.DirEntry, err error) error {
+ if err != nil {
+ t.Fatalf("WalkDir: %v", err)
+ }
+ if d.IsDir() {
+ return nil
+ }
+ bytes, _ := testdata.ReadFile(path)
+ path = strings.TrimPrefix(path, "testdata/")
+ path = strings.ReplaceAll(path, ".notgo", ".go")
+ filemap[path] = string(bytes)
+ return nil
+ })
+
+ // Run the actual tests, which are all declared within testdata/**.
+ dir, cleanup, err := analysistest.WriteFiles(filemap)
+ if err != nil {
+ t.Fatalf("WriteFiles: %v", err)
+ }
+ defer cleanup()
+ analysistest.Run(t, dir, Analyzer, "source.monogon.dev/...")
+}
diff --git a/build/analysis/importsort/testdata/README.md b/build/analysis/importsort/testdata/README.md
new file mode 100644
index 0000000..97c32a8
--- /dev/null
+++ b/build/analysis/importsort/testdata/README.md
@@ -0,0 +1,4 @@
+Test data for //build/analysis/importsort
+===
+
+This directory contains a fake GOPATH-like structure used to test the importsort analyzer. Files have the .notgo extension to prevent smart editors/IDEs from reformatting broken files, and to prevent Gazelle and other automation from ingesting these files as part of the main build.
\ No newline at end of file
diff --git a/build/analysis/importsort/testdata/example.com/extlib/extlib.notgo b/build/analysis/importsort/testdata/example.com/extlib/extlib.notgo
new file mode 100644
index 0000000..8f018ab
--- /dev/null
+++ b/build/analysis/importsort/testdata/example.com/extlib/extlib.notgo
@@ -0,0 +1 @@
+package extlib
diff --git a/build/analysis/importsort/testdata/example.com/extlib/foo/foo.notgo b/build/analysis/importsort/testdata/example.com/extlib/foo/foo.notgo
new file mode 100644
index 0000000..f52652b
--- /dev/null
+++ b/build/analysis/importsort/testdata/example.com/extlib/foo/foo.notgo
@@ -0,0 +1 @@
+package foo
diff --git a/build/analysis/importsort/testdata/source.monogon.dev/dut/mixed_in_group.notgo b/build/analysis/importsort/testdata/source.monogon.dev/dut/mixed_in_group.notgo
new file mode 100644
index 0000000..fabf98a
--- /dev/null
+++ b/build/analysis/importsort/testdata/source.monogon.dev/dut/mixed_in_group.notgo
@@ -0,0 +1,8 @@
+package dut
+
+import (
+ _ "errors"
+
+ _ "example.com/extlib" // want `import classes within group are mixed`
+ _ "source.monogon.dev/lib"
+)
diff --git a/build/analysis/importsort/testdata/source.monogon.dev/dut/okay.notgo b/build/analysis/importsort/testdata/source.monogon.dev/dut/okay.notgo
new file mode 100644
index 0000000..8c20859
--- /dev/null
+++ b/build/analysis/importsort/testdata/source.monogon.dev/dut/okay.notgo
@@ -0,0 +1,16 @@
+package dut
+
+import (
+ _ "errors"
+ _ "math"
+
+ _ "example.com/extlib"
+ // Comments within groups are okay.
+ _ "example.com/extlib/foo"
+
+ _ "source.monogon.dev/lib"
+
+ // Repeated group classes are okay.
+ _ "source.monogon.dev/project/a"
+ _ "source.monogon.dev/project/b"
+)
\ No newline at end of file
diff --git a/build/analysis/importsort/testdata/source.monogon.dev/dut/unsorted_group.notgo b/build/analysis/importsort/testdata/source.monogon.dev/dut/unsorted_group.notgo
new file mode 100644
index 0000000..902ccfe
--- /dev/null
+++ b/build/analysis/importsort/testdata/source.monogon.dev/dut/unsorted_group.notgo
@@ -0,0 +1,6 @@
+package dut
+
+import (
+ _ "math" // want `imports within group are not sorted`
+ _ "errors"
+)
diff --git a/build/analysis/importsort/testdata/source.monogon.dev/dut/wrong_group_order.notgo b/build/analysis/importsort/testdata/source.monogon.dev/dut/wrong_group_order.notgo
new file mode 100644
index 0000000..d9b021d
--- /dev/null
+++ b/build/analysis/importsort/testdata/source.monogon.dev/dut/wrong_group_order.notgo
@@ -0,0 +1,9 @@
+package dut
+
+import (
+ _ "errors"
+
+ _ "source.monogon.dev/lib"
+
+ _ "example.com/extlib" // want `global import group after local import group`
+)
\ No newline at end of file
diff --git a/build/analysis/importsort/testdata/source.monogon.dev/lib/lib.notgo b/build/analysis/importsort/testdata/source.monogon.dev/lib/lib.notgo
new file mode 100644
index 0000000..55c21f8
--- /dev/null
+++ b/build/analysis/importsort/testdata/source.monogon.dev/lib/lib.notgo
@@ -0,0 +1 @@
+package lib
diff --git a/build/analysis/importsort/testdata/source.monogon.dev/project/a/a.notgo b/build/analysis/importsort/testdata/source.monogon.dev/project/a/a.notgo
new file mode 100644
index 0000000..2a93cde
--- /dev/null
+++ b/build/analysis/importsort/testdata/source.monogon.dev/project/a/a.notgo
@@ -0,0 +1 @@
+package a
diff --git a/build/analysis/importsort/testdata/source.monogon.dev/project/b/b.notgo b/build/analysis/importsort/testdata/source.monogon.dev/project/b/b.notgo
new file mode 100644
index 0000000..e0836a8
--- /dev/null
+++ b/build/analysis/importsort/testdata/source.monogon.dev/project/b/b.notgo
@@ -0,0 +1 @@
+package b