internal/lsp/source: unify import adding code

We used to need our own copy of astutil.AddNamedImport to use during
completion for a variety of reasons, but I think the major one was
needing to not format the whole file. The same problem applied to using
the imports package.

Happily, that was resolved in CL 205678. Now we can use the same
implementation on both paths. In addition to removing a bunch of code,
that means that unimported completions now add their imports in the
right place, respecting goimports grouping and the local configuration
setting.

Fixes golang/go#35519.

Change-Id: I693c2e8b5ced9bac62b1febf1e2db23c770e5a7a
Reviewed-on: https://go-review.googlesource.com/c/tools/+/206881
Run-TryBot: Heschi Kreinick <heschi@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
This commit is contained in:
Heschi Kreinick 2019-11-12 16:58:00 -05:00
parent aa38f8e97a
commit c81e7ae886
5 changed files with 118 additions and 655 deletions

View File

@ -10,6 +10,7 @@ import (
"go/constant"
"go/token"
"go/types"
"strconv"
"strings"
"time"
@ -772,6 +773,26 @@ func (c *completer) lexical() error {
return nil
}
// alreadyImports reports whether f has an import with the specified path.
func alreadyImports(f *ast.File, path string) bool {
for _, s := range f.Imports {
if importPath(s) == path {
return true
}
}
return false
}
// importPath returns the unquoted import path of s,
// or "" if the path is not properly quoted.
func importPath(s *ast.ImportSpec) string {
t, err := strconv.Unquote(s.Path.Value)
if err != nil {
return ""
}
return t
}
func nodeContains(n ast.Node, pos token.Pos) bool {
return n != nil && n.Pos() <= pos && pos < n.End()
}

View File

@ -13,6 +13,7 @@ import (
"go/types"
"strings"
"golang.org/x/tools/internal/imports"
"golang.org/x/tools/internal/lsp/protocol"
"golang.org/x/tools/internal/lsp/snippet"
"golang.org/x/tools/internal/lsp/telemetry"
@ -190,12 +191,25 @@ func (c *completer) importEdits(imp *importInfo) ([]protocol.TextEdit, error) {
return nil, nil
}
edit, err := addNamedImport(c.view.Session().Cache().FileSet(), c.file, imp.name, imp.importPath)
if err != nil {
return nil, err
uri := span.FileURI(c.filename)
var ph ParseGoHandle
for _, h := range c.pkg.Files() {
if h.File().Identity().URI == uri {
ph = h
}
}
if ph == nil {
return nil, errors.Errorf("no ParseGoHandle for %s", c.filename)
}
return ToProtocolEdits(c.mapper, edit)
return computeOneImportFixEdits(c.ctx, c.view, ph, &imports.ImportFix{
StmtInfo: imports.ImportInfo{
ImportPath: imp.importPath,
Name: imp.name,
},
// IdentName is unused on this path and is difficult to get.
FixType: imports.AddImport,
})
}
func (c *completer) formatBuiltin(cand candidate) CompletionItem {

View File

@ -161,65 +161,12 @@ func computeImportEdits(ctx context.Context, view View, ph ParseGoHandle, option
}
origImports, origImportOffset := trimToImports(view.Session().Cache().FileSet(), origAST, origData)
computeFixEdits := func(fixes []*imports.ImportFix) ([]protocol.TextEdit, error) {
// Apply the fixes and re-parse the file so that we can locate the
// new imports.
fixedData, err := imports.ApplyFixes(fixes, filename, origData, options)
if err != nil {
return nil, err
}
fixedFset := token.NewFileSet()
fixedAST, err := parser.ParseFile(fixedFset, filename, fixedData, parser.ImportsOnly)
if err != nil {
return nil, err
}
fixedImports, fixedImportsOffset := trimToImports(fixedFset, fixedAST, fixedData)
// Prepare the diff. If both sides had import statements, we can diff
// just those sections against each other, then shift the resulting
// edits to the right lines in the original file.
left, right := origImports, fixedImports
converter := span.NewContentConverter(filename, origImports)
offset := origImportOffset
// If one side or the other has no imports, we won't know where to
// anchor the diffs. Instead, use the beginning of the file, up to its
// first non-imports decl. We know the imports code will insert
// somewhere before that.
if origImportOffset == 0 || fixedImportsOffset == 0 {
left = trimToFirstNonImport(view.Session().Cache().FileSet(), origAST, origData)
// We need the whole AST here, not just the ImportsOnly AST we parsed above.
fixedAST, err = parser.ParseFile(fixedFset, filename, fixedData, 0)
if err != nil {
return nil, err
}
right = trimToFirstNonImport(fixedFset, fixedAST, fixedData)
// We're now working with a prefix of the original file, so we can
// use the original converter, and there is no offset on the edits.
converter = origMapper.Converter
offset = 0
}
// Perform the diff and adjust the results for the trimming, if any.
edits := view.Options().ComputeEdits(ph.File().Identity().URI, string(left), string(right))
for i := range edits {
s, err := edits[i].Span.WithPosition(converter)
if err != nil {
return nil, err
}
start := span.NewPoint(s.Start().Line()+offset, s.Start().Column(), -1)
end := span.NewPoint(s.End().Line()+offset, s.End().Column(), -1)
edits[i].Span = span.New(s.URI(), start, end)
}
return ToProtocolEdits(origMapper, edits)
}
allFixes, err := imports.FixImports(filename, origData, options)
if err != nil {
return nil, nil, err
}
allFixEdits, err = computeFixEdits(allFixes)
allFixEdits, err = computeFixEdits(view, ph, options, origData, origAST, origMapper, origImports, origImportOffset, allFixes)
if err != nil {
return nil, nil, err
}
@ -227,7 +174,7 @@ func computeImportEdits(ctx context.Context, view View, ph ParseGoHandle, option
// Apply all of the import fixes to the file.
// Add the edits for each fix to the result.
for _, fix := range allFixes {
edits, err := computeFixEdits([]*imports.ImportFix{fix})
edits, err := computeFixEdits(view, ph, options, origData, origAST, origMapper, origImports, origImportOffset, []*imports.ImportFix{fix})
if err != nil {
return nil, nil, err
}
@ -239,6 +186,83 @@ func computeImportEdits(ctx context.Context, view View, ph ParseGoHandle, option
return allFixEdits, editsPerFix, nil
}
func computeOneImportFixEdits(ctx context.Context, view View, ph ParseGoHandle, fix *imports.ImportFix) ([]protocol.TextEdit, error) {
origData, _, err := ph.File().Read(ctx)
if err != nil {
return nil, err
}
origAST, origMapper, _, err := ph.Parse(ctx)
if err != nil {
return nil, err
}
origImports, origImportOffset := trimToImports(view.Session().Cache().FileSet(), origAST, origData)
options := &imports.Options{
// Defaults.
AllErrors: true,
Comments: true,
Fragment: true,
FormatOnly: false,
TabIndent: true,
TabWidth: 8,
}
return computeFixEdits(view, ph, options, origData, origAST, origMapper, origImports, origImportOffset, []*imports.ImportFix{fix})
}
func computeFixEdits(view View, ph ParseGoHandle, options *imports.Options, origData []byte, origAST *ast.File, origMapper *protocol.ColumnMapper, origImports []byte, origImportOffset int, fixes []*imports.ImportFix) ([]protocol.TextEdit, error) {
filename := ph.File().Identity().URI.Filename()
// Apply the fixes and re-parse the file so that we can locate the
// new imports.
fixedData, err := imports.ApplyFixes(fixes, filename, origData, options)
if err != nil {
return nil, err
}
fixedFset := token.NewFileSet()
fixedAST, err := parser.ParseFile(fixedFset, filename, fixedData, parser.ImportsOnly)
if err != nil {
return nil, err
}
fixedImports, fixedImportsOffset := trimToImports(fixedFset, fixedAST, fixedData)
// Prepare the diff. If both sides had import statements, we can diff
// just those sections against each other, then shift the resulting
// edits to the right lines in the original file.
left, right := origImports, fixedImports
converter := span.NewContentConverter(filename, origImports)
offset := origImportOffset
// If one side or the other has no imports, we won't know where to
// anchor the diffs. Instead, use the beginning of the file, up to its
// first non-imports decl. We know the imports code will insert
// somewhere before that.
if origImportOffset == 0 || fixedImportsOffset == 0 {
left = trimToFirstNonImport(view.Session().Cache().FileSet(), origAST, origData)
// We need the whole AST here, not just the ImportsOnly AST we parsed above.
fixedAST, err = parser.ParseFile(fixedFset, filename, fixedData, 0)
if err != nil {
return nil, err
}
right = trimToFirstNonImport(fixedFset, fixedAST, fixedData)
// We're now working with a prefix of the original file, so we can
// use the original converter, and there is no offset on the edits.
converter = origMapper.Converter
offset = 0
}
// Perform the diff and adjust the results for the trimming, if any.
edits := view.Options().ComputeEdits(ph.File().Identity().URI, string(left), string(right))
for i := range edits {
s, err := edits[i].Span.WithPosition(converter)
if err != nil {
return nil, err
}
start := span.NewPoint(s.Start().Line()+offset, s.Start().Column(), -1)
end := span.NewPoint(s.End().Line()+offset, s.End().Column(), -1)
edits[i].Span = span.New(s.URI(), start, end)
}
return ToProtocolEdits(origMapper, edits)
}
// trimToImports returns a section of the source file that covers all of the
// import declarations, and the line offset into the file that section starts at.
func trimToImports(fset *token.FileSet, f *ast.File, src []byte) ([]byte, int) {

View File

@ -1,264 +0,0 @@
// Copyright 2019 The Go Authors. All rights reserved.
// Use of this source code is governed by a BSD-style
// license that can be found in the LICENSE file.
package source
import (
"bytes"
"fmt"
"go/ast"
"go/format"
"go/token"
"strconv"
"strings"
"golang.org/x/tools/internal/lsp/diff"
"golang.org/x/tools/internal/span"
)
// Taken and then modified from golang.org/x/tools/go/ast/astutil.
//
// We currently choose to create our own version of AddNamedImport for the following reasons:
// 1. We do not want to edit the current ast. This is so that we can use the same ast
// to get the changes from multiple distinct modifications.
// 2. We need the changes that *only* affect the import declarations, because the edits
// are not allowed to overlap with the position in the source that is being edited.
// 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.
// If name is not empty, it is used to rename the import.
//
// For example, calling
// 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 []diff.TextEdit, err error) {
if alreadyImportsNamed(f, name, path) {
return nil, nil
}
newImport := &ast.ImportSpec{
Path: &ast.BasicLit{
Kind: token.STRING,
Value: strconv.Quote(path),
},
}
if name != "" {
newImport.Name = &ast.Ident{Name: name}
}
// 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)
}
}
}
var insertPos token.Pos
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)
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()
}
}
rng := span.NewRange(fset, insertPos, insertPos)
spn, err := rng.Span()
if err != nil {
return nil, err
}
edits = append(edits, diff.TextEdit{
Span: spn,
NewText: newText,
})
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 path.
func alreadyImports(f *ast.File, path string) bool {
for _, s := range f.Imports {
if importPath(s) == path {
return true
}
}
return false
}
// alreadyImportsNamed reports whether f has an import with the specified name
// and path.
func alreadyImportsNamed(f *ast.File, name, path string) bool {
for _, s := range f.Imports {
if importName(s) == name && importPath(s) == path {
return true
}
}
return false
}
// importName returns the name of s,
// or "" if the import is not named.
func importName(s *ast.ImportSpec) string {
if s.Name == nil {
return ""
}
return s.Name.Name
}
// importPath returns the unquoted import path of s,
// or "" if the path is not properly quoted.
func importPath(s *ast.ImportSpec) string {
t, err := strconv.Unquote(s.Path.Value)
if err != nil {
return ""
}
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
}

View File

@ -1,332 +0,0 @@
package source
import (
"bytes"
"fmt"
"go/ast"
"go/format"
"go/parser"
"go/token"
"testing"
"golang.org/x/tools/internal/lsp/diff"
)
var fset = token.NewFileSet()
func parse(t *testing.T, name, in string) *ast.File {
file, err := parser.ParseFile(fset, name, in, parser.ParseComments)
if err != nil {
t.Fatalf("%s parse: %v", name, err)
}
return file
}
func print(t *testing.T, name string, f *ast.File) string {
var buf bytes.Buffer
if err := format.Node(&buf, fset, f); err != nil {
t.Fatalf("%s gofmt: %v", name, err)
}
return buf.String()
}
type test struct {
name string
renamedPkg string
pkg string
in string
want []imp
unchanged bool // Expect added/deleted return value to be false.
}
type imp struct {
name string
path string
}
var addTests = []test{
{
name: "leave os alone",
pkg: "os",
in: `package main
import (
"os"
)
`,
want: []imp{
{
name: "",
path: "os",
},
},
unchanged: true,
},
{
name: "package statement only",
pkg: "os",
in: `package main
`,
want: []imp{
{
name: "",
path: "os",
},
},
},
{
name: "package statement no new line",
pkg: "os",
in: `package main`,
want: []imp{
{
name: "",
path: "os",
},
},
},
{
// Issue 33721: add import statement after package declaration preceded by comments.
name: "issue 33721 package statement comments before",
pkg: "os",
in: `// Here is a comment before
package main
`,
want: []imp{
{
name: "",
path: "os",
},
},
},
{
name: "package statement comments same line",
pkg: "os",
in: `package main // Here is a comment after
`,
want: []imp{
{
name: "",
path: "os",
},
},
},
{
name: "package statement comments before and after",
pkg: "os",
in: `// Here is a comment before
package main // Here is a comment after`,
want: []imp{
{
name: "",
path: "os",
},
},
},
{
name: "package statement multiline comments",
pkg: "os",
in: `package main /* This is a multiline comment
and it extends
further down*/`,
want: []imp{
{
name: "",
path: "os",
},
},
},
{
name: "import c",
pkg: "os",
in: `package main
import "C"
`,
want: []imp{
{
name: "",
path: "os",
},
{
name: "",
path: "C",
},
},
},
{
name: "existing imports",
pkg: "os",
in: `package main
import "io"
`,
want: []imp{
{
name: "",
path: "os",
},
{
name: "",
path: "io",
},
},
},
{
name: "existing imports with comment",
pkg: "os",
in: `package main
import "io" // A comment
`,
want: []imp{
{
name: "",
path: "os",
},
{
name: "",
path: "io",
},
},
},
{
name: "existing imports multiline comment",
pkg: "os",
in: `package main
import "io" /* A comment
that
extends */
`,
want: []imp{
{
name: "",
path: "os",
},
{
name: "",
path: "io",
},
},
},
{
name: "renamed import",
renamedPkg: "o",
pkg: "os",
in: `package main
`,
want: []imp{
{
name: "o",
path: "os",
},
},
},
}
func TestAddImport(t *testing.T) {
for _, test := range addTests {
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)
if err != nil && !test.unchanged {
t.Errorf("error adding import: %s", err)
continue
}
// Apply the edits and parse the file.
got := applyEdits(test.in, edits)
gotFile := parse(t, test.name, got)
compareImports(t, fmt.Sprintf("first run: %s:\n", test.name), gotFile.Imports, test.want)
// 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)
if err != nil && !test.unchanged {
t.Errorf("error adding import: %s", err)
continue
}
// Apply the edits and parse the file.
got = applyEdits(got, edits)
gotFile = parse(t, test.name, got)
compareImports(t, test.name, gotFile.Imports, test.want)
}
}
func TestDoubleAddNamedImport(t *testing.T) {
name := "doublenamedimport"
in := "package main\n"
file := parse(t, name, in)
// Add a named import
edits, err := addNamedImport(fset, file, "o", "os")
if err != nil {
t.Errorf("error adding import: %s", err)
return
}
got := applyEdits(in, edits)
gotFile := parse(t, name, got)
// Add a second named import
edits, err = addNamedImport(fset, gotFile, "i", "io")
if err != nil {
t.Errorf("error adding import: %s", err)
return
}
got = applyEdits(got, edits)
gotFile = parse(t, name, got)
want := []imp{
{
name: "o",
path: "os",
},
{
name: "i",
path: "io",
},
}
compareImports(t, "", gotFile.Imports, want)
}
func compareImports(t *testing.T, prefix string, got []*ast.ImportSpec, want []imp) {
if len(got) != len(want) {
t.Errorf("%s\ngot %d imports\nwant %d", prefix, len(got), len(want))
return
}
for _, imp := range got {
name := importName(imp)
path := importPath(imp)
found := false
for _, want := range want {
if want.name == name && want.path == path {
found = true
break
}
}
if !found {
t.Errorf("%s\n\ngot unexpected import: name: %q,path: %q", prefix, name, path)
continue
}
}
}
func applyEdits(contents string, edits []diff.TextEdit) string {
res := contents
// Apply the edits from the end of the file forward
// to preserve the offsets
for i := len(edits) - 1; i >= 0; i-- {
edit := edits[i]
start := edit.Span.Start().Offset()
end := edit.Span.End().Offset()
tmp := res[0:start] + edit.NewText
res = tmp + res[end:]
}
return res
}