diff --git a/internal/lsp/source/completion_format.go b/internal/lsp/source/completion_format.go index bacd57858b..3d02feb64a 100644 --- a/internal/lsp/source/completion_format.go +++ b/internal/lsp/source/completion_format.go @@ -89,7 +89,7 @@ func (c *completer) item(cand candidate) (CompletionItem, error) { // If this candidate needs an additional import statement, // add the additional text edits needed. if cand.imp != nil { - edit, err := AddNamedImport(c.view.Session().Cache().FileSet(), c.file, cand.imp.Name, cand.imp.ImportPath) + edit, err := addNamedImport(c.view.Session().Cache().FileSet(), c.file, cand.imp.Name, cand.imp.ImportPath) if err != nil { return CompletionItem{}, err } diff --git a/internal/lsp/source/imports.go b/internal/lsp/source/imports.go index 0f3e3fd23d..1e80d1057b 100644 --- a/internal/lsp/source/imports.go +++ b/internal/lsp/source/imports.go @@ -6,10 +6,12 @@ package source import ( "bytes" + "fmt" "go/ast" "go/format" "go/token" "strconv" + "strings" "golang.org/x/tools/internal/span" ) @@ -24,16 +26,16 @@ import ( // astutil.AddNamedImport makes it hard to determine what changes need to be made // to the source document from the ast, as astutil.AddNamedImport includes a merging pass. -// AddNamedImport adds the import with the given name and path to the file f, if absent. +// addNamedImport adds the import with the given name and path to the file f, if absent. // If name is not empty, it is used to rename the import. // // For example, calling -// AddNamedImport(fset, f, "pathpkg", "path") +// addNamedImport(fset, f, "pathpkg", "path") // adds // import pathpkg "path" // -// AddNamedImport only returns edits that affect the import declarations. -func AddNamedImport(fset *token.FileSet, f *ast.File, name, path string) (edits []TextEdit, err error) { +// addNamedImport only returns edits that affect the import declarations. +func addNamedImport(fset *token.FileSet, f *ast.File, name, path string) (edits []TextEdit, err error) { if alreadyImports(f, name, path) { return nil, nil } @@ -48,48 +50,128 @@ func AddNamedImport(fset *token.FileSet, f *ast.File, name, path string) (edits newImport.Name = &ast.Ident{Name: name} } - // TODO(suzmue): insert the import statement in the location that would be chosen - // by astutil.AddNamedImport - // Find the last import decl. - var lastImport = -1 // index in f.Decls of the file's final import decl + // Find an import decl to add to. + // The goal is to find an existing import + // whose import path has the longest shared + // prefix with path. + var ( + bestMatch = -1 // length of longest shared prefix + lastImport = -1 // index in f.Decls of the file's final import decl + impDecl *ast.GenDecl // import decl containing the best match + impIndex = -1 // spec index in impDecl containing the best match + + isThirdPartyPath = isThirdParty(path) + ) for i, decl := range f.Decls { gen, ok := decl.(*ast.GenDecl) if ok && gen.Tok == token.IMPORT { lastImport = i + // Do not add to import "C", to avoid disrupting the + // association with its doc comment, breaking cgo. + if declImports(gen, "C") { + continue + } + + // Do not add to single imports. + if !gen.Lparen.IsValid() { + continue + } + + // Match an empty import decl if that's all that is available. + if len(gen.Specs) == 0 && bestMatch == -1 { + impDecl = gen + } + + // Compute longest shared prefix with imports in this group and find best + // matched import spec. + // 1. Always prefer import spec with longest shared prefix. + // 2. While match length is 0, + // - for stdlib package: prefer first import spec. + // - for third party package: prefer first third party import spec. + // We cannot use last import spec as best match for third party package + // because grouped imports are usually placed last by goimports -local + // flag. + // See issue #19190. + seenAnyThirdParty := false + for j, spec := range gen.Specs { + impspec := spec.(*ast.ImportSpec) + p := importPath(impspec) + n := matchLen(p, path) + if n > bestMatch || (bestMatch == 0 && !seenAnyThirdParty && isThirdPartyPath) { + bestMatch = n + impDecl = gen + impIndex = j + } + seenAnyThirdParty = seenAnyThirdParty || isThirdParty(p) + } } } - // Add an import decl after the last import. - impDecl := &ast.GenDecl{ - Tok: token.IMPORT, - } - impDecl.Specs = append(impDecl.Specs, newImport) - var insertPos token.Pos - if lastImport >= 0 { - insertPos = f.Decls[lastImport].End() - } else { - // There are no existing imports. - // Our new import, preceded by a blank line, goes after the package declaration - // and after the comment, if any, that starts on the same line as the - // package declaration. - insertPos = f.Name.End() + var newText string + // If no import decl found, add one after the last import. + if impDecl == nil { + // Add an import decl after the last import. + impDecl = &ast.GenDecl{ + Tok: token.IMPORT, + } + impDecl.Specs = append(impDecl.Specs, newImport) - file := fset.File(f.Package) - pkgLine := file.Line(f.Package) - for _, c := range f.Comments { - if file.Line(c.Pos()) > pkgLine { - break + if lastImport >= 0 { + insertPos = f.Decls[lastImport].End() + } else { + // There are no existing imports. + // Our new import goes after the package declaration. + insertPos = f.Name.End() + } + + // Print the whole import declaration. + newText = fmt.Sprintf("\n\nimport %s", printImportSpec(fset, newImport)) + } else { + // Insert new import at insertAt. + insertAt := 0 + if impIndex >= 0 { + // insert after the found import + insertAt = impIndex + 1 + } + + insertPos = impDecl.Lparen + 1 // insert after the parenthesis + if len(impDecl.Specs) > 0 { + insertPos = impDecl.Specs[0].Pos() // insert at the beginning + } + if insertAt > 0 { + // If there is a comment after an existing import, preserve the comment + // position by adding the new import after the comment. + if spec, ok := impDecl.Specs[insertAt-1].(*ast.ImportSpec); ok && spec.Comment != nil { + insertPos = spec.Comment.End() + } else { + // Assign same position as the previous import, + // so that the sorter sees it as being in the same block. + insertPos = impDecl.Specs[insertAt-1].End() } + } + + // Print this import declaration. + newText = fmt.Sprintf("\n\t%s", printImportSpec(fset, newImport)) + } + + // If we didn't find a valid insert position, return no edits. + if !insertPos.IsValid() { + return edits, nil + } + + // Make sure that we are printed after any comments that start on the same line. + file := fset.File(insertPos) + pkgLine := file.Line(insertPos) + for _, c := range f.Comments { + if file.Line(c.Pos()) > pkgLine { + break + } + if c.End() > insertPos { insertPos = c.End() } } - // Print this import declaration. - var buf bytes.Buffer - format.Node(&buf, fset, impDecl) - newText := "\n\n" + buf.String() + "\n" - rng := span.NewRange(fset, insertPos, insertPos) spn, err := rng.Span() if err != nil { @@ -103,6 +185,18 @@ func AddNamedImport(fset *token.FileSet, f *ast.File, name, path string) (edits return edits, nil } +func printImportSpec(fset *token.FileSet, spec *ast.ImportSpec) string { + var buf bytes.Buffer + format.Node(&buf, fset, spec) + return buf.String() +} + +func isThirdParty(importPath string) bool { + // Third party package import path usually contains "." (".com", ".org", ...) + // This logic is taken from golang.org/x/tools/imports package. + return strings.Contains(importPath, ".") +} + // alreadyImports reports whether f has an import with the specified name and path. func alreadyImports(f *ast.File, name, path string) bool { for _, s := range f.Imports { @@ -131,3 +225,28 @@ func importPath(s *ast.ImportSpec) string { } return t } + +// declImports reports whether gen contains an import of path. +func declImports(gen *ast.GenDecl, path string) bool { + if gen.Tok != token.IMPORT { + return false + } + for _, spec := range gen.Specs { + impspec := spec.(*ast.ImportSpec) + if importPath(impspec) == path { + return true + } + } + return false +} + +// matchLen returns the length of the longest path segment prefix shared by x and y. +func matchLen(x, y string) int { + n := 0 + for i := 0; i < len(x) && i < len(y) && x[i] == y[i]; i++ { + if x[i] == '/' { + n++ + } + } + return n +} diff --git a/internal/lsp/source/imports_test.go b/internal/lsp/source/imports_test.go index 76c86ae050..0a62f6f085 100644 --- a/internal/lsp/source/imports_test.go +++ b/internal/lsp/source/imports_test.go @@ -7,7 +7,6 @@ import ( "go/format" "go/parser" "go/token" - "log" "testing" ) @@ -203,7 +202,7 @@ func TestAddImport(t *testing.T) { file := parse(t, test.name, test.in) var before bytes.Buffer ast.Fprint(&before, fset, file, nil) - edits, err := AddNamedImport(fset, file, test.renamedPkg, test.pkg) + edits, err := addNamedImport(fset, file, test.renamedPkg, test.pkg) if err != nil && !test.unchanged { t.Errorf("error adding import: %s", err) continue @@ -217,7 +216,7 @@ func TestAddImport(t *testing.T) { // AddNamedImport should be idempotent. Verify that by calling it again, // expecting no change to the AST, and the returned added value to always be false. - edits, err = AddNamedImport(fset, gotFile, test.renamedPkg, test.pkg) + edits, err = addNamedImport(fset, gotFile, test.renamedPkg, test.pkg) if err != nil && !test.unchanged { t.Errorf("error adding import: %s", err) continue @@ -236,17 +235,16 @@ func TestDoubleAddNamedImport(t *testing.T) { in := "package main\n" file := parse(t, name, in) // Add a named import - edits, err := AddNamedImport(fset, file, "o", "os") + edits, err := addNamedImport(fset, file, "o", "os") if err != nil { t.Errorf("error adding import: %s", err) return } got := applyEdits(in, edits) - log.Println(got) gotFile := parse(t, name, got) // Add a second named import - edits, err = AddNamedImport(fset, gotFile, "i", "io") + edits, err = addNamedImport(fset, gotFile, "i", "io") if err != nil { t.Errorf("error adding import: %s", err) return