diff --git a/internal/lsp/source/completion.go b/internal/lsp/source/completion.go index 277d2cee89..af1131c0bd 100644 --- a/internal/lsp/source/completion.go +++ b/internal/lsp/source/completion.go @@ -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() } diff --git a/internal/lsp/source/completion_format.go b/internal/lsp/source/completion_format.go index f56d7a2584..d0683c3135 100644 --- a/internal/lsp/source/completion_format.go +++ b/internal/lsp/source/completion_format.go @@ -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 { diff --git a/internal/lsp/source/format.go b/internal/lsp/source/format.go index 6b0e409bd5..1d5b3c2f6c 100644 --- a/internal/lsp/source/format.go +++ b/internal/lsp/source/format.go @@ -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) { diff --git a/internal/lsp/source/imports.go b/internal/lsp/source/imports.go deleted file mode 100644 index 53fb4358e1..0000000000 --- a/internal/lsp/source/imports.go +++ /dev/null @@ -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 -} diff --git a/internal/lsp/source/imports_test.go b/internal/lsp/source/imports_test.go deleted file mode 100644 index 0927c59948..0000000000 --- a/internal/lsp/source/imports_test.go +++ /dev/null @@ -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 -}