Skip to content

Commit

Permalink
gopls/internal: support renaming packages with int. test variants
Browse files Browse the repository at this point in the history
We need to search intermediate test variants to find all imports of a
renamed package, but were missing them because we only considered
"active packages". Fix this by building up the set of renamed packages
based on metadata alone: it is unnecessary and potentially too costly to
request all (typechecked) known packages, when we only need access to
the metadata graph to determine which packages must be renamed.

In doing so, it becomes possible that we produce duplicate edits by
renaming through a test variant. Avoid this by keeping track of import
path changes that we have already processed.

While at it, add a few more checks for package renaming:
 - check for valid identifiers
 - disallow renaming x_test packages
 - disallow renaming to x_test packages

Also refactor package renaming slightly to pass around an edit map. This
fixes a bug where nested import paths were not renamed in the original
renaming package.

Fix some testing bugs that were exercised by new tests.

For golang/go#41567

Change-Id: I18ab442b33a3ee5bf527f078dcaa81d47f0220c7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/443637
Reviewed-by: Dylan Le <dungtuanle@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
  • Loading branch information
findleyr committed Oct 20, 2022
1 parent 649df2e commit 6128030
Show file tree
Hide file tree
Showing 11 changed files with 501 additions and 113 deletions.
1 change: 0 additions & 1 deletion gopls/internal/lsp/cache/check.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,6 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so
experimentalKey := s.View().Options().ExperimentalPackageCacheKey
phKey := computePackageKey(m.ID, compiledGoFiles, m, depKey, mode, experimentalKey)
promise, release := s.store.Promise(phKey, func(ctx context.Context, arg interface{}) interface{} {

pkg, err := typeCheckImpl(ctx, arg.(*snapshot), goFiles, compiledGoFiles, m.Metadata, mode, deps)
return typeCheckResult{pkg, err}
})
Expand Down
22 changes: 22 additions & 0 deletions gopls/internal/lsp/cache/snapshot.go
Original file line number Diff line number Diff line change
Expand Up @@ -1195,6 +1195,28 @@ func (s *snapshot) KnownPackages(ctx context.Context) ([]source.Package, error)
return pkgs, nil
}

func (s *snapshot) AllValidMetadata(ctx context.Context) ([]source.Metadata, error) {
if err := s.awaitLoaded(ctx); err != nil {
return nil, err
}

s.mu.Lock()
defer s.mu.Unlock()

var meta []source.Metadata
for _, m := range s.meta.metadata {
if m.Valid {
meta = append(meta, m)
}
}
return meta, nil
}

func (s *snapshot) WorkspacePackageByID(ctx context.Context, id string) (source.Package, error) {
packageID := PackageID(id)
return s.checkedPackage(ctx, packageID, s.workspaceParseMode(packageID))
}

func (s *snapshot) CachedImportPaths(ctx context.Context) (map[string]source.Package, error) {
// Don't reload workspace package metadata.
// This function is meant to only return currently cached information.
Expand Down
21 changes: 20 additions & 1 deletion gopls/internal/lsp/fake/editor.go
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,12 @@ func (e *Editor) onFileChanges(ctx context.Context, evts []FileEvent) {
}

// OpenFile creates a buffer for the given workdir-relative file.
//
// If the file is already open, it is a no-op.
func (e *Editor) OpenFile(ctx context.Context, path string) error {
if e.HasBuffer(path) {
return nil
}
content, err := e.sandbox.Workdir.ReadFile(path)
if err != nil {
return err
Expand All @@ -383,6 +388,11 @@ func (e *Editor) CreateBuffer(ctx context.Context, path, content string) error {
func (e *Editor) createBuffer(ctx context.Context, path string, dirty bool, content string) error {
e.mu.Lock()

if _, ok := e.buffers[path]; ok {
e.mu.Unlock()
return fmt.Errorf("buffer %q already exists", path)
}

buf := buffer{
windowsLineEndings: e.config.WindowsLineEndings,
version: 1,
Expand Down Expand Up @@ -712,7 +722,7 @@ func (e *Editor) editBufferLocked(ctx context.Context, path string, edits []Edit
}
content, err := applyEdits(buf.lines, edits)
if err != nil {
return err
return fmt.Errorf("editing %q: %v; edits:\n%v", path, err, edits)
}
return e.setBufferContentLocked(ctx, path, true, content, edits)
}
Expand Down Expand Up @@ -1171,6 +1181,15 @@ func (e *Editor) Rename(ctx context.Context, path string, pos Pos, newName strin
if e.Server == nil {
return nil
}

// Verify that PrepareRename succeeds.
prepareParams := &protocol.PrepareRenameParams{}
prepareParams.TextDocument = e.TextDocumentIdentifier(path)
prepareParams.Position = pos.ToProtocolPosition()
if _, err := e.Server.PrepareRename(ctx, prepareParams); err != nil {
return fmt.Errorf("preparing rename: %v", err)
}

params := &protocol.RenameParams{
TextDocument: e.TextDocumentIdentifier(path),
Position: pos.ToProtocolPosition(),
Expand Down
17 changes: 17 additions & 0 deletions gopls/internal/lsp/fake/workdir.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"os"
"path/filepath"
"runtime"
"sort"
"strings"
"sync"
"time"
Expand Down Expand Up @@ -349,6 +350,22 @@ func (w *Workdir) RenameFile(ctx context.Context, oldPath, newPath string) error
return nil
}

// ListFiles returns a new sorted list of the relative paths of files in dir,
// recursively.
func (w *Workdir) ListFiles(dir string) ([]string, error) {
m, err := w.listFiles(dir)
if err != nil {
return nil, err
}

var paths []string
for p := range m {
paths = append(paths, p)
}
sort.Strings(paths)
return paths, nil
}

// listFiles lists files in the given directory, returning a map of relative
// path to contents and modification time.
func (w *Workdir) listFiles(dir string) (map[string]fileID, error) {
Expand Down
11 changes: 11 additions & 0 deletions gopls/internal/lsp/regtest/wrappers.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,17 @@ func (e *Env) WriteWorkspaceFiles(files map[string]string) {
}
}

// ListFiles lists relative paths to files in the given directory.
// It calls t.Fatal on any error.
func (e *Env) ListFiles(dir string) []string {
e.T.Helper()
paths, err := e.Sandbox.Workdir.ListFiles(dir)
if err != nil {
e.T.Fatal(err)
}
return paths
}

// OpenFile opens a file in the editor, calling t.Fatal on any error.
func (e *Env) OpenFile(name string) {
e.T.Helper()
Expand Down
2 changes: 2 additions & 0 deletions gopls/internal/lsp/source/references.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ func References(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Posit
}

if inPackageName {
// TODO(rfindley): this is inaccurate, excluding test variants, and
// redundant with package renaming. Refactor to share logic.
renamingPkg, err := s.PackageForFile(ctx, f.URI(), TypecheckWorkspace, NarrowestPackage)
if err != nil {
return nil, err
Expand Down
Loading

0 comments on commit 6128030

Please sign in to comment.