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) + } + } +}