From 61280309af36ad3b716dc3d4a1703b362ff5fc35 Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Mon, 17 Oct 2022 16:35:50 -0400 Subject: [PATCH] gopls/internal: support renaming packages with int. test variants 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 gopls-CI: kokoro TryBot-Result: Gopher Robot Run-TryBot: Robert Findley Reviewed-by: Alan Donovan --- gopls/internal/lsp/cache/check.go | 1 - gopls/internal/lsp/cache/snapshot.go | 22 ++ gopls/internal/lsp/fake/editor.go | 21 +- gopls/internal/lsp/fake/workdir.go | 17 + gopls/internal/lsp/regtest/wrappers.go | 11 + gopls/internal/lsp/source/references.go | 2 + gopls/internal/lsp/source/rename.go | 300 ++++++++++++------ gopls/internal/lsp/source/view.go | 7 + .../completion/postfix_snippet_test.go | 4 +- .../internal/regtest/misc/definition_test.go | 1 - gopls/internal/regtest/misc/rename_test.go | 228 ++++++++++++- 11 files changed, 501 insertions(+), 113 deletions(-) diff --git a/gopls/internal/lsp/cache/check.go b/gopls/internal/lsp/cache/check.go index 235eacd2d91..70eaed0d193 100644 --- a/gopls/internal/lsp/cache/check.go +++ b/gopls/internal/lsp/cache/check.go @@ -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} }) diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go index eda1ff7e137..c7e5e8ad640 100644 --- a/gopls/internal/lsp/cache/snapshot.go +++ b/gopls/internal/lsp/cache/snapshot.go @@ -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. diff --git a/gopls/internal/lsp/fake/editor.go b/gopls/internal/lsp/fake/editor.go index b9f62f3028d..dfd17c7e55a 100644 --- a/gopls/internal/lsp/fake/editor.go +++ b/gopls/internal/lsp/fake/editor.go @@ -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 @@ -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, @@ -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) } @@ -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(), diff --git a/gopls/internal/lsp/fake/workdir.go b/gopls/internal/lsp/fake/workdir.go index f194e263fdd..2b426e40c3b 100644 --- a/gopls/internal/lsp/fake/workdir.go +++ b/gopls/internal/lsp/fake/workdir.go @@ -13,6 +13,7 @@ import ( "os" "path/filepath" "runtime" + "sort" "strings" "sync" "time" @@ -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) { diff --git a/gopls/internal/lsp/regtest/wrappers.go b/gopls/internal/lsp/regtest/wrappers.go index 63b59917c10..13e5f7bc819 100644 --- a/gopls/internal/lsp/regtest/wrappers.go +++ b/gopls/internal/lsp/regtest/wrappers.go @@ -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() diff --git a/gopls/internal/lsp/source/references.go b/gopls/internal/lsp/source/references.go index de6687ad49f..e26091c9fe8 100644 --- a/gopls/internal/lsp/source/references.go +++ b/gopls/internal/lsp/source/references.go @@ -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 diff --git a/gopls/internal/lsp/source/rename.go b/gopls/internal/lsp/source/rename.go index 2f0ad56ec66..2d4a188b8ac 100644 --- a/gopls/internal/lsp/source/rename.go +++ b/gopls/internal/lsp/source/rename.go @@ -76,26 +76,44 @@ func PrepareRename(ctx context.Context, snapshot Snapshot, f FileHandle, pp prot err := errors.New("can't rename package: LSP client does not support file renaming") return nil, err, err } - renamingPkg, err := snapshot.PackageForFile(ctx, f.URI(), TypecheckWorkspace, NarrowestPackage) + fileMeta, err := snapshot.MetadataForFile(ctx, f.URI()) if err != nil { return nil, err, err } - if renamingPkg.Name() == "main" { + if len(fileMeta) == 0 { + err := fmt.Errorf("no packages found for file %q", f.URI()) + return nil, err, err + } + + meta := fileMeta[0] + + if meta.PackageName() == "main" { err := errors.New("can't rename package \"main\"") return nil, err, err } - if renamingPkg.Version() == nil { - err := fmt.Errorf("can't rename package: missing module information for package %q", renamingPkg.PkgPath()) + if strings.HasSuffix(meta.PackageName(), "_test") { + err := errors.New("can't rename x_test packages") return nil, err, err } - if renamingPkg.Version().Path == renamingPkg.PkgPath() { - err := fmt.Errorf("can't rename package: package path %q is the same as module path %q", renamingPkg.PkgPath(), renamingPkg.Version().Path) + if meta.ModuleInfo() == nil { + err := fmt.Errorf("can't rename package: missing module information for package %q", meta.PackagePath()) return nil, err, err } - result, err := computePrepareRenameResp(snapshot, renamingPkg, pgf.File.Name, renamingPkg.Name()) + + if meta.ModuleInfo().Path == meta.PackagePath() { + err := fmt.Errorf("can't rename package: package path %q is the same as module path %q", meta.PackagePath(), meta.ModuleInfo().Path) + return nil, err, err + } + // TODO(rfindley): we should not need the package here. + pkg, err := snapshot.WorkspacePackageByID(ctx, meta.PackageID()) + if err != nil { + err = fmt.Errorf("error building package to rename: %v", err) + return nil, err, err + } + result, err := computePrepareRenameResp(snapshot, pkg, pgf.File.Name, pkg.Name()) if err != nil { return nil, nil, err } @@ -164,65 +182,46 @@ func Rename(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Position, } if inPackageName { - // Since we only take one package below, no need to include test variants. - // - // TODO(rfindley): but is this correct? What about x_test packages that - // import the renaming package? - const includeTestVariants = false - pkgs, err := s.PackagesForFile(ctx, f.URI(), TypecheckWorkspace, includeTestVariants) + if !isValidIdentifier(newName) { + return nil, true, fmt.Errorf("%q is not a valid identifier", newName) + } + + fileMeta, err := s.MetadataForFile(ctx, f.URI()) if err != nil { return nil, true, err } - var pkg Package // TODO(rfindley): we should consider all packages, so that we get the full reverse transitive closure. - for _, p := range pkgs { - // pgf.File.Name must not be nil, else this will panic. - if pgf.File.Name.Name == p.Name() { - pkg = p - break - } + + if len(fileMeta) == 0 { + return nil, true, fmt.Errorf("no packages found for file %q", f.URI()) } - activePkgs, err := s.ActivePackages(ctx) - if err != nil { - return nil, true, err + + // We need metadata for the relevant package and module paths. These should + // be the same for all packages containing the file. + // + // TODO(rfindley): we mix package path and import path here haphazardly. + // Fix this. + meta := fileMeta[0] + oldPath := meta.PackagePath() + var modulePath string + if mi := meta.ModuleInfo(); mi == nil { + return nil, true, fmt.Errorf("cannot rename package: missing module information for package %q", meta.PackagePath()) + } else { + modulePath = mi.Path + } + + if strings.HasSuffix(newName, "_test") { + return nil, true, fmt.Errorf("cannot rename to _test package") } - renamingEdits, err := computeImportRenamingEdits(ctx, s, pkg, activePkgs, newName) + + metadata, err := s.AllValidMetadata(ctx) if err != nil { return nil, true, err } - pkgNameEdits, err := computePackageNameRenamingEdits(pkg, newName) + + renamingEdits, err := renamePackage(ctx, s, modulePath, oldPath, newName, metadata) if err != nil { return nil, true, err } - for uri, edits := range pkgNameEdits { - renamingEdits[uri] = edits - } - // Rename test packages - for _, activePkg := range activePkgs { - if activePkg.ForTest() != pkg.PkgPath() { - continue - } - // Filter out intermediate test variants. - if activePkg.PkgPath() != pkg.PkgPath() && activePkg.PkgPath() != pkg.PkgPath()+"_test" { - continue - } - newTestPkgName := newName - if strings.HasSuffix(activePkg.Name(), "_test") { - newTestPkgName += "_test" - } - perPackageEdits, err := computeRenamePackageImportEditsPerPackage(ctx, s, activePkg, newTestPkgName, pkg.PkgPath()) - for uri, edits := range perPackageEdits { - renamingEdits[uri] = append(renamingEdits[uri], edits...) - } - pkgNameEdits, err := computePackageNameRenamingEdits(activePkg, newTestPkgName) - if err != nil { - return nil, true, err - } - for uri, edits := range pkgNameEdits { - if _, ok := renamingEdits[uri]; !ok { - renamingEdits[uri] = edits - } - } - } return renamingEdits, true, nil } @@ -239,98 +238,187 @@ func Rename(ctx context.Context, s Snapshot, f FileHandle, pp protocol.Position, return result, false, nil } -// computeImportRenamingEdits computes all edits to files in other packages that import -// the renaming package. -func computeImportRenamingEdits(ctx context.Context, s Snapshot, renamingPkg Package, pkgs []Package, newName string) (map[span.URI][]protocol.TextEdit, error) { - result := make(map[span.URI][]protocol.TextEdit) +// renamePackage computes all workspace edits required to rename the package +// described by the given metadata, to newName, by renaming its package +// directory. +// +// It updates package clauses and import paths for the renamed package as well +// as any other packages affected by the directory renaming among packages +// described by allMetadata. +func renamePackage(ctx context.Context, s Snapshot, modulePath, oldPath, newName string, allMetadata []Metadata) (map[span.URI][]protocol.TextEdit, error) { + if modulePath == oldPath { + return nil, fmt.Errorf("cannot rename package: module path %q is the same as the package path, so renaming the package directory would have no effect", modulePath) + } + + newPathPrefix := path.Join(path.Dir(oldPath), newName) + + edits := make(map[span.URI][]protocol.TextEdit) + seen := make(seenPackageRename) // track per-file import renaming we've already processed + // Rename imports to the renamed package from other packages. - for _, pkg := range pkgs { - if renamingPkg.Version() == nil { - return nil, fmt.Errorf("cannot rename package: missing module information for package %q", renamingPkg.PkgPath()) + for _, m := range allMetadata { + // Special case: x_test packages for the renamed package will not have the + // package path as as a dir prefix, but still need their package clauses + // renamed. + if m.PackagePath() == oldPath+"_test" { + newTestName := newName + "_test" + + if err := renamePackageClause(ctx, m, s, newTestName, seen, edits); err != nil { + return nil, err + } + continue } - renamingPkgModulePath := renamingPkg.Version().Path - activePkgModulePath := pkg.Version().Path - if !strings.HasPrefix(pkg.PkgPath()+"/", renamingPkg.PkgPath()+"/") { - continue // not a nested package or the renaming package. + + // Subtle: check this condition before checking for valid module info + // below, because we should not fail this operation if unrelated packages + // lack module info. + if !strings.HasPrefix(m.PackagePath()+"/", oldPath+"/") { + continue // not affected by the package renaming } - if activePkgModulePath == pkg.PkgPath() { - continue // don't edit imports to nested package whose path and module path is the same. + if m.ModuleInfo() == nil { + return nil, fmt.Errorf("cannot rename package: missing module information for package %q", m.PackagePath()) } - if renamingPkgModulePath != "" && renamingPkgModulePath != activePkgModulePath { - continue // don't edit imports if nested package and renaming package has different module path. + if modulePath != m.ModuleInfo().Path { + continue // don't edit imports if nested package and renaming package have different module paths } - // Compute all edits for other files that import this nested package - // when updating the its path. - perFileEdits, err := computeRenamePackageImportEditsPerPackage(ctx, s, pkg, newName, renamingPkg.PkgPath()) - if err != nil { - return nil, err + // Renaming a package consists of changing its import path and package name. + suffix := strings.TrimPrefix(m.PackagePath(), oldPath) + newPath := newPathPrefix + suffix + + pkgName := m.PackageName() + if m.PackagePath() == oldPath { + pkgName = newName + + if err := renamePackageClause(ctx, m, s, newName, seen, edits); err != nil { + return nil, err + } } - for uri, edits := range perFileEdits { - result[uri] = append(result[uri], edits...) + + if err := renameImports(ctx, s, m, newPath, pkgName, seen, edits); err != nil { + return nil, err } } - return result, nil + return edits, nil } -// computePackageNameRenamingEdits computes all edits to files within the renming packages. -func computePackageNameRenamingEdits(renamingPkg Package, newName string) (map[span.URI][]protocol.TextEdit, error) { - result := make(map[span.URI][]protocol.TextEdit) +// seenPackageRename tracks import path renamings that have already been +// processed. +// +// Due to test variants, files may appear multiple times in the reverse +// transitive closure of a renamed package, or in the reverse transitive +// closure of different variants of a renamed package (both are possible). +// However, in all cases the resulting edits will be the same. +type seenPackageRename map[seenPackageKey]bool +type seenPackageKey struct { + uri span.URI + importPath string +} + +// add reports whether uri and importPath have been seen, and records them as +// seen if not. +func (s seenPackageRename) add(uri span.URI, importPath string) bool { + key := seenPackageKey{uri, importPath} + seen := s[key] + if !seen { + s[key] = true + } + return seen +} + +// renamePackageClause computes edits renaming the package clause of files in +// the package described by the given metadata, to newName. +// +// As files may belong to multiple packages, the seen map tracks files whose +// package clause has already been updated, to prevent duplicate edits. +// +// Edits are written into the edits map. +func renamePackageClause(ctx context.Context, m Metadata, s Snapshot, newName string, seen seenPackageRename, edits map[span.URI][]protocol.TextEdit) error { + pkg, err := s.WorkspacePackageByID(ctx, m.PackageID()) + if err != nil { + return err + } + // Rename internal references to the package in the renaming package. - for _, f := range renamingPkg.CompiledGoFiles() { + for _, f := range pkg.CompiledGoFiles() { + if seen.add(f.URI, m.PackagePath()) { + continue + } + if f.File.Name == nil { continue } pkgNameMappedRange := NewMappedRange(f.Tok, f.Mapper, f.File.Name.Pos(), f.File.Name.End()) - // Invalid range for the package name. rng, err := pkgNameMappedRange.Range() if err != nil { - return nil, err + return err } - result[f.URI] = append(result[f.URI], protocol.TextEdit{ + edits[f.URI] = append(edits[f.URI], protocol.TextEdit{ Range: rng, NewText: newName, }) } - return result, nil + return nil } -// computeRenamePackageImportEditsPerPackage computes the set of edits (to imports) -// among the files of package nestedPkg that are necessary when package renamedPkg -// is renamed to newName. -func computeRenamePackageImportEditsPerPackage(ctx context.Context, s Snapshot, nestedPkg Package, newName, renamingPath string) (map[span.URI][]protocol.TextEdit, error) { - rdeps, err := s.GetReverseDependencies(ctx, nestedPkg.ID()) +// renameImports computes the set of edits to imports resulting from renaming +// the package described by the given metadata, to a package with import path +// newPath and name newName. +// +// Edits are written into the edits map. +func renameImports(ctx context.Context, s Snapshot, m Metadata, newPath, newName string, seen seenPackageRename, edits map[span.URI][]protocol.TextEdit) error { + // TODO(rfindley): we should get reverse dependencies as metadata first, + // rather then building the package immediately. We don't need reverse + // dependencies if they are intermediate test variants. + rdeps, err := s.GetReverseDependencies(ctx, m.PackageID()) if err != nil { - return nil, err + return err } - result := make(map[span.URI][]protocol.TextEdit) for _, dep := range rdeps { + // Subtle: don't perform renaming in this package if it is not fully + // parsed. This can occur inside the workspace if dep is an intermediate + // test variant. ITVs are only ever parsed in export mode, and no file is + // found only in an ITV. Therefore the renaming will eventually occur in a + // full package. + // + // An alternative algorithm that may be more robust would be to first + // collect *files* that need to have their imports updated, and then + // perform the rename using s.PackageForFile(..., NarrowestPackage). + if dep.ParseMode() != ParseFull { + continue + } + for _, f := range dep.CompiledGoFiles() { + if seen.add(f.URI, m.PackagePath()) { + continue + } + for _, imp := range f.File.Imports { - if impPath, _ := strconv.Unquote(imp.Path.Value); impPath != nestedPkg.PkgPath() { - continue // not the import we're looking for. + if impPath, _ := strconv.Unquote(imp.Path.Value); impPath != m.PackagePath() { + continue // not the import we're looking for } // Create text edit for the import path (string literal). impPathMappedRange := NewMappedRange(f.Tok, f.Mapper, imp.Path.Pos(), imp.Path.End()) rng, err := impPathMappedRange.Range() if err != nil { - return nil, err + return err } - newText := strconv.Quote(path.Join(path.Dir(renamingPath), newName) + strings.TrimPrefix(nestedPkg.PkgPath(), renamingPath)) - result[f.URI] = append(result[f.URI], protocol.TextEdit{ + newText := strconv.Quote(newPath) + edits[f.URI] = append(edits[f.URI], protocol.TextEdit{ Range: rng, NewText: newText, }) - // If the nested package is not the renaming package or its import path already - // has an local package name then we don't need to update the local package name. - if nestedPkg.PkgPath() != renamingPath || imp.Name != nil { + // If the package name of an import has not changed or if its import + // path already has a local package name, then we don't need to update + // the local package name. + if newName == m.PackageName() || imp.Name != nil { continue } @@ -344,6 +432,7 @@ func computeRenamePackageImportEditsPerPackage(ctx context.Context, s Snapshot, var changes map[span.URI][]protocol.TextEdit localName := newName try := 0 + // Keep trying with fresh names until one succeeds. for fileScope.Lookup(localName) != nil || pkgScope.Lookup(localName) != nil { try++ @@ -351,8 +440,9 @@ func computeRenamePackageImportEditsPerPackage(ctx context.Context, s Snapshot, } changes, err = renameObj(ctx, s, localName, qos) if err != nil { - return nil, err + return err } + // If the chosen local package name matches the package's new name, delete the // change that would have inserted an explicit local name, which is always // the lexically first change. @@ -363,14 +453,14 @@ func computeRenamePackageImportEditsPerPackage(ctx context.Context, s Snapshot, }) changes[f.URI] = v[1:] } - for uri, edits := range changes { - result[uri] = append(result[uri], edits...) + for uri, changeEdits := range changes { + edits[uri] = append(edits[uri], changeEdits...) } } } } - return result, nil + return nil } // renameObj returns a map of TextEdits for renaming an identifier within a file diff --git a/gopls/internal/lsp/source/view.go b/gopls/internal/lsp/source/view.go index 5fb99ea4054..25fa7d704d0 100644 --- a/gopls/internal/lsp/source/view.go +++ b/gopls/internal/lsp/source/view.go @@ -166,6 +166,13 @@ type Snapshot interface { // mode, this is just the reverse transitive closure of open packages. ActivePackages(ctx context.Context) ([]Package, error) + // AllValidMetadata returns all valid metadata loaded for the snapshot. + AllValidMetadata(ctx context.Context) ([]Metadata, error) + + // WorkspacePackageByID returns the workspace package with id, type checked + // in 'workspace' mode. + WorkspacePackageByID(ctx context.Context, id string) (Package, error) + // Symbols returns all symbols in the snapshot. Symbols(ctx context.Context) map[span.URI][]Symbol diff --git a/gopls/internal/regtest/completion/postfix_snippet_test.go b/gopls/internal/regtest/completion/postfix_snippet_test.go index e02f4c8ce10..56e26a235bf 100644 --- a/gopls/internal/regtest/completion/postfix_snippet_test.go +++ b/gopls/internal/regtest/completion/postfix_snippet_test.go @@ -438,12 +438,14 @@ func foo() string { }, ) r.Run(t, mod, func(t *testing.T, env *Env) { + env.CreateBuffer("foo.go", "") + for _, c := range cases { t.Run(c.name, func(t *testing.T) { c.before = strings.Trim(c.before, "\n") c.after = strings.Trim(c.after, "\n") - env.CreateBuffer("foo.go", c.before) + env.SetBufferContent("foo.go", c.before) pos := env.RegexpSearch("foo.go", "\n}") completions := env.Completion("foo.go", pos) diff --git a/gopls/internal/regtest/misc/definition_test.go b/gopls/internal/regtest/misc/definition_test.go index f2b7b1adbec..a5030d71fb3 100644 --- a/gopls/internal/regtest/misc/definition_test.go +++ b/gopls/internal/regtest/misc/definition_test.go @@ -87,7 +87,6 @@ func TestUnexportedStdlib_Issue40809(t *testing.T) { Run(t, stdlibDefinition, func(t *testing.T, env *Env) { env.OpenFile("main.go") name, _ := env.GoToDefinition("main.go", env.RegexpSearch("main.go", `fmt.(Printf)`)) - env.OpenFile(name) pos := env.RegexpSearch(name, `:=\s*(newPrinter)\(\)`) diff --git a/gopls/internal/regtest/misc/rename_test.go b/gopls/internal/regtest/misc/rename_test.go index aa0a47ad7b8..a9fd920317f 100644 --- a/gopls/internal/regtest/misc/rename_test.go +++ b/gopls/internal/regtest/misc/rename_test.go @@ -10,6 +10,7 @@ import ( "golang.org/x/tools/gopls/internal/lsp/protocol" . "golang.org/x/tools/gopls/internal/lsp/regtest" + "golang.org/x/tools/gopls/internal/lsp/tests/compare" "golang.org/x/tools/internal/testenv" ) @@ -448,7 +449,7 @@ package b }) } -func TestRenameWithTestPackage(t *testing.T) { +func TestRenamePackage_Tests(t *testing.T) { testenv.NeedsGo1Point(t, 17) const files = ` -- go.mod -- @@ -518,7 +519,7 @@ func main() { }) } -func TestRenameWithNestedModule(t *testing.T) { +func TestRenamePackage_NestedModule(t *testing.T) { testenv.NeedsGo1Point(t, 17) const files = ` -- go.mod -- @@ -577,7 +578,7 @@ func main() { }) } -func TestRenamePackageWithNonBlankSameImportPaths(t *testing.T) { +func TestRenamePackage_DuplicateImport(t *testing.T) { testenv.NeedsGo1Point(t, 17) const files = ` -- go.mod -- @@ -620,7 +621,7 @@ func main() { }) } -func TestRenamePackageWithBlankSameImportPaths(t *testing.T) { +func TestRenamePackage_DuplicateBlankImport(t *testing.T) { testenv.NeedsGo1Point(t, 17) const files = ` -- go.mod -- @@ -662,3 +663,222 @@ func main() { env.RegexpSearch("main.go", `lib1 "mod.com/nested/nested"`) }) } + +func TestRenamePackage_TestVariant(t *testing.T) { + const files = ` +-- go.mod -- +module mod.com + +go 1.12 +-- foo/foo.go -- +package foo + +const Foo = 42 +-- bar/bar.go -- +package bar + +import "mod.com/foo" + +const Bar = foo.Foo +-- bar/bar_test.go -- +package bar + +import "mod.com/foo" + +const Baz = foo.Foo +-- testdata/bar/bar.go -- +package bar + +import "mod.com/foox" + +const Bar = foox.Foo +-- testdata/bar/bar_test.go -- +package bar + +import "mod.com/foox" + +const Baz = foox.Foo +` + Run(t, files, func(t *testing.T, env *Env) { + env.OpenFile("foo/foo.go") + env.Rename("foo/foo.go", env.RegexpSearch("foo/foo.go", "package (foo)"), "foox") + + checkTestdata(t, env) + }) +} + +func TestRenamePackage_IntermediateTestVariant(t *testing.T) { + // In this test set up, we have the following import edges: + // bar_test -> baz -> foo -> bar + // bar_test -> foo -> bar + // bar_test -> bar + // + // As a consequence, bar_x_test.go is in the reverse closure of both + // `foo [bar.test]` and `baz [bar.test]`. This test confirms that we don't + // produce duplicate edits in this case. + const files = ` +-- go.mod -- +module foo.mod + +go 1.12 +-- foo/foo.go -- +package foo + +import "foo.mod/bar" + +const Foo = 42 + +const _ = bar.Bar +-- baz/baz.go -- +package baz + +import "foo.mod/foo" + +const Baz = foo.Foo +-- bar/bar.go -- +package bar + +var Bar = 123 +-- bar/bar_test.go -- +package bar + +const _ = Bar +-- bar/bar_x_test.go -- +package bar_test + +import ( + "foo.mod/bar" + "foo.mod/baz" + "foo.mod/foo" +) + +const _ = bar.Bar + baz.Baz + foo.Foo +-- testdata/foox/foo.go -- +package foox + +import "foo.mod/bar" + +const Foo = 42 + +const _ = bar.Bar +-- testdata/baz/baz.go -- +package baz + +import "foo.mod/foox" + +const Baz = foox.Foo +-- testdata/bar/bar_x_test.go -- +package bar_test + +import ( + "foo.mod/bar" + "foo.mod/baz" + "foo.mod/foox" +) + +const _ = bar.Bar + baz.Baz + foox.Foo +` + + Run(t, files, func(t *testing.T, env *Env) { + env.OpenFile("foo/foo.go") + env.Rename("foo/foo.go", env.RegexpSearch("foo/foo.go", "package (foo)"), "foox") + + checkTestdata(t, env) + }) +} + +func TestRenamePackage_Nesting(t *testing.T) { + testenv.NeedsGo1Point(t, 17) + const files = ` +-- go.mod -- +module mod.com + +go 1.18 +-- lib/a.go -- +package lib + +import "mod.com/lib/nested" + +const A = 1 + nested.B +-- lib/nested/a.go -- +package nested + +const B = 1 +-- other/other.go -- +package other + +import ( + "mod.com/lib" + "mod.com/lib/nested" +) + +const C = lib.A + nested.B +-- testdata/libx/a.go -- +package libx + +import "mod.com/libx/nested" + +const A = 1 + nested.B +-- testdata/other/other.go -- +package other + +import ( + "mod.com/libx" + "mod.com/libx/nested" +) + +const C = libx.A + nested.B +` + Run(t, files, func(t *testing.T, env *Env) { + env.OpenFile("lib/a.go") + pos := env.RegexpSearch("lib/a.go", "package (lib)") + env.Rename("lib/a.go", pos, "libx") + + checkTestdata(t, env) + }) +} + +func TestRenamePackage_InvalidName(t *testing.T) { + testenv.NeedsGo1Point(t, 17) + const files = ` +-- go.mod -- +module mod.com + +go 1.18 +-- lib/a.go -- +package lib + +import "mod.com/lib/nested" + +const A = 1 + nested.B +` + + Run(t, files, func(t *testing.T, env *Env) { + env.OpenFile("lib/a.go") + pos := env.RegexpSearch("lib/a.go", "package (lib)") + + for _, badName := range []string{"$$$", "lib_test"} { + if err := env.Editor.Rename(env.Ctx, "lib/a.go", pos, badName); err == nil { + t.Errorf("Rename(lib, libx) succeeded, want non-nil error") + } + } + }) +} + +// checkTestdata checks that current buffer contents match their corresponding +// expected content in the testdata directory. +func checkTestdata(t *testing.T, env *Env) { + t.Helper() + files := env.ListFiles("testdata") + if len(files) == 0 { + t.Fatal("no files in testdata directory") + } + for _, file := range files { + suffix := strings.TrimPrefix(file, "testdata/") + got := env.Editor.BufferText(suffix) + want := env.ReadWorkspaceFile(file) + if diff := compare.Text(want, got); diff != "" { + t.Errorf("Rename: unexpected buffer content for %s (-want +got):\n%s", suffix, diff) + } + } +}