internal/analysisinternal: add std import at the start of the import group

This CL ensures that standard library imports are added at the start of the
import group rather than at the end.

In projects with third-party dependencies (such as gopls), imports are usually
organized in the following order: standard library imports, imports from the
module itself, and third-party imports.

Previously, when analysisinternal.AddImport was called to add a new standard
library package (e.g., "slices") to a file that already imports both standard
library and third-party modules, the newly added "slices" import would
inappropriately be placed among the third-party imports.

This violates widely accepted conventions, and gofmt does not automatically fix
such cases.

This CL solves the problem when running 'modernize -fix ./...' under gopls, we
need to manually move the new added std package to put std imports together.

Change-Id: I714c92c205542358dcdd1a6ed6db171be9c5fb56
Reviewed-on: https://go-review.googlesource.com/c/tools/+/664436
LUCI-TryBot-Result: Go LUCI <[email protected]>
Commit-Queue: Alan Donovan <[email protected]>
Auto-Submit: Alan Donovan <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
Reviewed-by: Junyang Shao <[email protected]>

Author: xieyuschen

gopls/internal/analysis/modernize/modernize.go

@@ -170,22 +170,10 @@ func enclosingFile(c cursor.Cursor) *ast.File {
 // specified standard packages or their dependencies.
 func within(pass *analysis.Pass, pkgs ...string) bool {
 	path := pass.Pkg.Path()
-	return standard(path) &&
+	return analysisinternal.IsStdPackage(path) &&
 		moreiters.Contains(stdlib.Dependencies(pkgs...), path)
 }
 
-// standard reports whether the specified package path belongs to a
-// package in the standard library (including internal dependencies).
-func standard(path string) bool {
-	// A standard package has no dot in its first segment.
-	// (It may yet have a dot, e.g. "vendor/golang.org/x/foo".)
-	slash := strings.IndexByte(path, '/')
-	if slash < 0 {
-		slash = len(path)
-	}
-	return !strings.Contains(path[:slash], ".") && path != "testdata"
-}
-
 var (
 	builtinAny     = types.Universe.Lookup("any")
 	builtinAppend  = types.Universe.Lookup("append")

internal/analysisinternal/addimport_test.go

@@ -238,10 +238,59 @@ func _(io.Reader) {
 			want: `package a
 
 import (
+	"fmt"
 	"io"
+)
+
+func _(io.Reader) {
+	fmt
+}`,
+		},
+		{
+			descr: descr("add import to group which imports std and a 3rd module"),
+			src: `package a
+
+import (
+	"io"
+
+	"vendor/golang.org/x/net/dns/dnsmessage"
+)
+
+func _(io.Reader) {
+	«fmt fmt»
+}`,
+			want: `package a
+
+import (
 	"fmt"
+	"io"
+
+	"vendor/golang.org/x/net/dns/dnsmessage"
 )
 
+func _(io.Reader) {
+	fmt
+}`,
+		},
+		{
+			descr: descr("add import to group which imports std and a 3rd module without parens"),
+			src: `package a
+
+import "io"
+
+import "vendor/golang.org/x/net/dns/dnsmessage"
+
+func _(io.Reader) {
+	«fmt fmt»
+}`,
+			want: `package a
+
+import "fmt"
+
+import "io"
+
+import "vendor/golang.org/x/net/dns/dnsmessage"
+
 func _(io.Reader) {
 	fmt
 }`,
@@ -315,3 +364,25 @@ func _(io.Reader) {
 		})
 	}
 }
+
+func TestIsStdPackage(t *testing.T) {
+	testCases := []struct {
+		pkgpath string
+		isStd   bool
+	}{
+		{pkgpath: "os", isStd: true},
+		{pkgpath: "net/http", isStd: true},
+		{pkgpath: "vendor/golang.org/x/net/dns/dnsmessage", isStd: true},
+		{pkgpath: "golang.org/x/net/dns/dnsmessage", isStd: false},
+		{pkgpath: "testdata", isStd: false},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.pkgpath, func(t *testing.T) {
+			got := analysisinternal.IsStdPackage(tc.pkgpath)
+			if got != tc.isStd {
+				t.Fatalf("got %t want %t", got, tc.isStd)
+			}
+		})
+	}
+}

internal/analysisinternal/analysis.go

@@ -280,7 +280,15 @@ func AddImport(info *types.Info, file *ast.File, preferredName, pkgpath, member
 	// If the first decl is an import group, add this new import at the end.
 	if gd, ok := before.(*ast.GenDecl); ok && gd.Tok == token.IMPORT && gd.Rparen.IsValid() {
 		pos = gd.Rparen
-		newText = "\t" + newText + "\n"
+		// if it's a std lib, we should append it at the beginning of import group.
+		// otherwise we may see the std package is put at the last behind a 3rd module which doesn't follow our convention.
+		// besides, gofmt doesn't help in this case.
+		if IsStdPackage(pkgpath) && len(gd.Specs) != 0 {
+			pos = gd.Specs[0].Pos()
+			newText += "\n\t"
+		} else {
+			newText = "\t" + newText + "\n"
+		}
 	} else {
 		pos = before.Pos()
 		newText = "import " + newText + "\n\n"
@@ -637,3 +645,15 @@ func Comments(file *ast.File, start, end token.Pos) iter.Seq[*ast.Comment] {
 		}
 	}
 }
+
+// IsStdPackage reports whether the specified package path belongs to a
+// package in the standard library (including internal dependencies).
+func IsStdPackage(path string) bool {
+	// A standard package has no dot in its first segment.
+	// (It may yet have a dot, e.g. "vendor/golang.org/x/foo".)
+	slash := strings.IndexByte(path, '/')
+	if slash < 0 {
+		slash = len(path)
+	}
+	return !strings.Contains(path[:slash], ".") && path != "testdata"
+}