gopls/internal/settings: add renameMovesSubpackages setting

Removes the packageMove setting - now we always prompt with
the full package path during a package rename.

The new renameMovesSubpackages setting, false by default,
controls whether a package rename also results in moving
its subpackages.

Change-Id: I191f85cec09e244217fadf820aa7ee60a93063c7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/721660
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Alan Donovan <adonovan@google.com>

Author: madelinekalil

gopls/doc/features/transformation.md

@@ -340,18 +340,22 @@ Special cases:
   ```
 
 Using Rename to move a package:
-- When initiating a rename request on a package identifier, users can specify
-a new package name which will result in the package's contents moving to a new
-directory at the same directory level. Subpackages are not affected.
-- When the setting "packageMove" is set to true, users are prompted with
-the package path instead of the package name but can specify either an arbitrary
-package path or a package name. Subpackages are not affected.
+To rename a package, execute the Rename operation over the `p` in a
+`package p` declaration at the start of a file.
+You will be prompted to edit the package's path and choose its new location.
+The Rename operation will move all the package's files to the resulting directory,
+creating it if necessary.
+By default, subpackages will remain where they are. To include subpackages
+in the renaming, set [renameMovesSubpackages](../settings.md#renamemovessubpackages-bool) to true.
+Existing imports of the package will be updated to reflect its new path.
 
 Package moves are rejected if they would break the build. For example:
-- Packages cannot move across a module boundary
-- Packages cannot be moved into existing packages; gopls does not support package merging
-- Packages cannot be moved to internal directories that would make them inaccessible to any of their current importers
+- Packages cannot move across a module boundary.
+- Packages cannot be moved into existing packages; gopls does not support package merging.
+- Packages cannot be moved to internal directories that would make them inaccessible to any of their current importers.
 
+Renaming package main is not supported, because the main package has special meaning to the linker.
+Renaming x_test packages is currently not supported.
 
 Some tips for best results:
 

gopls/doc/settings.md

@@ -269,13 +269,13 @@ and package declaration in a newly created Go file.
 
 Default: `true`.
 
-<a id='packageMove'></a>
-### `packageMove bool`
+<a id='renameMovesSubpackages'></a>
+### `renameMovesSubpackages bool`
 
 **This setting is experimental and may be deleted.**
 
-packageMove enables PrepareRename to send the full package path
-and allows users to move a package via renaming.
+renameMovesSubpackages enables Rename operations on packages to
+move subdirectories of the target package.
 
 Default: `false`.
 

gopls/internal/doc/api.json

@@ -2189,9 +2189,9 @@
 				"DeprecationMessage": ""
 			},
 			{
-				"Name": "packageMove",
+				"Name": "renameMovesSubpackages",
 				"Type": "bool",
-				"Doc": "packageMove enables PrepareRename to send the full package path\nand allows users to move a package via renaming.\n",
+				"Doc": "renameMovesSubpackages enables Rename operations on packages to\nmove subdirectories of the target package.\n",
 				"EnumKeys": {
 					"ValueType": "",
 					"Keys": null

gopls/internal/golang/rename.go

@@ -53,7 +53,6 @@ import (
 	"go/types"
 	"maps"
 	"os"
-	"path"
 	"path/filepath"
 	"regexp"
 	"slices"
@@ -199,12 +198,14 @@ func prepareRenamePackageName(ctx context.Context, snapshot *cache.Snapshot, pgf
 	if meta.Name == "main" {
 		return nil, fmt.Errorf("can't rename package \"main\"")
 	}
+	// TODO(mkalil): support renaming x_test packages.
 	if strings.HasSuffix(string(meta.Name), "_test") {
 		return nil, fmt.Errorf("can't rename x_test packages")
 	}
 	if meta.Module == nil {
 		return nil, fmt.Errorf("can't rename package: missing module information for package %q", meta.PkgPath)
 	}
+	// TODO(mkalil): why is renaming the root package of a module unsupported?
 	if meta.Module.Path == string(meta.PkgPath) {
 		return nil, fmt.Errorf("can't rename package: package path %q is the same as module path %q", meta.PkgPath, meta.Module.Path)
 	}
@@ -215,20 +216,9 @@ func prepareRenamePackageName(ctx context.Context, snapshot *cache.Snapshot, pgf
 		return nil, err
 	}
 
-	pkgName := string(meta.Name)
-	fullPath := string(meta.PkgPath)
-	text := pkgName
-	// Before displaying the full package path, verify that the PackageMove
-	// setting is enabled and that the package name matches its directory
-	// basename. Checking the value of meta.Module above ensures that the
-	// current view is either a GoMod or a GoWork view, which are the only views
-	// for which we should enable package move.
-	if snapshot.Options().PackageMove && path.Base(fullPath) == pkgName {
-		text = fullPath
-	}
 	return &PrepareItem{
 		Range: rng,
-		Text:  text,
+		Text:  string(meta.PkgPath),
 	}, nil
 }
 
@@ -420,7 +410,7 @@ func editsToDocChanges(ctx context.Context, snapshot *cache.Snapshot, edits map[
 
 // Rename returns a map of TextEdits for each file modified when renaming a
 // given identifier within a package.
-func Rename(ctx context.Context, snapshot *cache.Snapshot, f file.Handle, pp protocol.Position, newName string, renameSubpkgs bool) ([]protocol.DocumentChange, error) {
+func Rename(ctx context.Context, snapshot *cache.Snapshot, f file.Handle, pp protocol.Position, newName string) ([]protocol.DocumentChange, error) {
 	ctx, done := event.Start(ctx, "golang.Rename")
 	defer done()
 
@@ -461,10 +451,11 @@ func Rename(ctx context.Context, snapshot *cache.Snapshot, f file.Handle, pp pro
 			newPkgPath PackagePath
 			err        error
 		)
-		if newPkgDir, newPkgName, newPkgPath, err = checkPackageRename(snapshot.Options(), pkg, f, newName); err != nil {
+		moveSubpackages := snapshot.Options().RenameMovesSubpackages
+		if newPkgDir, newPkgName, newPkgPath, err = checkPackageRename(pkg, f, newName, moveSubpackages); err != nil {
 			return nil, err
 		}
-		editMap, err = renamePackage(ctx, snapshot, f, newPkgName, newPkgPath, newPkgDir, renameSubpkgs)
+		editMap, err = renamePackage(ctx, snapshot, f, newPkgName, newPkgPath, newPkgDir, moveSubpackages)
 		if err != nil {
 			return nil, err
 		}
@@ -524,7 +515,7 @@ func Rename(ctx context.Context, snapshot *cache.Snapshot, f file.Handle, pp pro
 	}
 	if inPackageName {
 		oldDir := f.URI().DirPath()
-		if renameSubpkgs {
+		if snapshot.Options().RenameMovesSubpackages {
 			// Update the last component of the file's enclosing directory.
 			changes = append(changes, protocol.DocumentChangeRename(
 				protocol.URIFromPath(oldDir),
@@ -962,15 +953,15 @@ func renameExported(pkgs []*cache.Package, declPkgPath PackagePath, declObjPath
 //
 // f is the file originating the rename, and therefore f.URI().Dir() is the
 // current package directory. newName, newPath, and newDir describe the renaming.
-func renamePackage(ctx context.Context, s *cache.Snapshot, f file.Handle, newName PackageName, newPath PackagePath, newDir string, renameSubpkgs bool) (map[protocol.DocumentURI][]diff.Edit, error) {
+func renamePackage(ctx context.Context, s *cache.Snapshot, f file.Handle, newName PackageName, newPath PackagePath, newDir string, moveSubpackages bool) (map[protocol.DocumentURI][]diff.Edit, error) {
 	// Rename the package decl and all imports.
 	renamingEdits := make(map[protocol.DocumentURI][]diff.Edit)
-	err := updatePackageDeclsAndImports(ctx, s, f, newName, newPath, renamingEdits, renameSubpkgs)
+	err := updatePackageDeclsAndImports(ctx, s, f, newName, newPath, renamingEdits, moveSubpackages)
 	if err != nil {
 		return nil, err
 	}
 
-	err = updateModFiles(ctx, s, f.URI().DirPath(), newDir, renamingEdits, renameSubpkgs)
+	err = updateModFiles(ctx, s, f.URI().DirPath(), newDir, renamingEdits, moveSubpackages)
 	if err != nil {
 		return nil, err
 	}
@@ -981,7 +972,7 @@ func renamePackage(ctx context.Context, s *cache.Snapshot, f file.Handle, newNam
 // Update any affected replace directives in go.mod files.
 // TODO(adonovan): should this operate on all go.mod files,
 // irrespective of whether they are included in the workspace?
-func updateModFiles(ctx context.Context, s *cache.Snapshot, oldDir string, newPkgDir string, renamingEdits map[protocol.DocumentURI][]diff.Edit, renameSubpkgs bool) error {
+func updateModFiles(ctx context.Context, s *cache.Snapshot, oldDir string, newPkgDir string, renamingEdits map[protocol.DocumentURI][]diff.Edit, moveSubpackages bool) error {
 	modFiles := s.View().ModFiles()
 	for _, m := range modFiles {
 		fh, err := s.ReadFile(ctx, m)
@@ -1008,8 +999,8 @@ func updateModFiles(ctx context.Context, s *cache.Snapshot, oldDir string, newPk
 
 			// TODO: Is there a risk of converting a '\' delimited replacement to a '/' delimited replacement?
 
-			if renameSubpkgs && !strings.HasPrefix(filepath.ToSlash(replacedDir)+"/", filepath.ToSlash(oldDir)+"/") ||
-				!renameSubpkgs && !(filepath.ToSlash(replacedDir) == filepath.ToSlash(oldDir)) {
+			if moveSubpackages && !strings.HasPrefix(filepath.ToSlash(replacedDir)+"/", filepath.ToSlash(oldDir)+"/") ||
+				!moveSubpackages && !(filepath.ToSlash(replacedDir) == filepath.ToSlash(oldDir)) {
 				continue //not affected by the package renanming
 			}
 
@@ -1068,7 +1059,7 @@ func updateModFiles(ctx context.Context, s *cache.Snapshot, oldDir string, newPk
 // It updates package clauses and import paths for the renamed package as well
 // as any other packages affected by the directory renaming among all packages
 // known to the snapshot.
-func updatePackageDeclsAndImports(ctx context.Context, s *cache.Snapshot, f file.Handle, newName PackageName, newPkgPath PackagePath, renamingEdits map[protocol.DocumentURI][]diff.Edit, renameSubpkgs bool) error {
+func updatePackageDeclsAndImports(ctx context.Context, s *cache.Snapshot, f file.Handle, newName PackageName, newPkgPath PackagePath, renamingEdits map[protocol.DocumentURI][]diff.Edit, moveSubpackages bool) error {
 	if strings.HasSuffix(string(newName), "_test") {
 		return fmt.Errorf("cannot rename to _test package")
 	}
@@ -1112,8 +1103,8 @@ func updatePackageDeclsAndImports(ctx context.Context, s *cache.Snapshot, f file
 		// 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 renameSubpkgs && !pathutil.InDir(string(oldPkgPath), string(mp.PkgPath)) ||
-			!renameSubpkgs && mp.PkgPath != oldPkgPath {
+		if moveSubpackages && !pathutil.InDir(filepath.FromSlash(string(oldPkgPath)), filepath.FromSlash(string(mp.PkgPath))) ||
+			!moveSubpackages && mp.PkgPath != oldPkgPath {
 			continue // not affected by the package renaming
 		}
 
@@ -1138,7 +1129,6 @@ func updatePackageDeclsAndImports(ctx context.Context, s *cache.Snapshot, f file
 				return err
 			}
 		}
-
 		imp := ImportPath(newImportPath) // TODO(adonovan): what if newImportPath has vendor/ prefix?
 		if err := renameImports(ctx, s, mp, imp, pkgName, renamingEdits); err != nil {
 			return err

gopls/internal/golang/rename_check.go

@@ -51,7 +51,6 @@ import (
 	"golang.org/x/tools/go/ast/inspector"
 	"golang.org/x/tools/gopls/internal/cache"
 	"golang.org/x/tools/gopls/internal/file"
-	"golang.org/x/tools/gopls/internal/settings"
 	"golang.org/x/tools/gopls/internal/util/safetoken"
 	"golang.org/x/tools/internal/typeparams"
 	"golang.org/x/tools/internal/typesinternal"
@@ -923,7 +922,7 @@ func isValidIdentifier(id string) bool {
 // resulting from renaming the current package to newName, which may be an
 // identifier or a package path. An error is returned if the renaming is
 // invalid.
-func checkPackageRename(opts *settings.Options, curPkg *cache.Package, f file.Handle, newName string) (newPkgDir string, newPkgName PackageName, newPkgPath PackagePath, err error) {
+func checkPackageRename(curPkg *cache.Package, f file.Handle, newName string, moveSubpackages bool) (newPkgDir string, newPkgName PackageName, newPkgPath PackagePath, err error) {
 	// Path unchanged
 	if newName == curPkg.String() || newName == string(curPkg.Metadata().Name) {
 		return f.URI().DirPath(), curPkg.Metadata().Name, curPkg.Metadata().PkgPath, nil
@@ -949,21 +948,20 @@ func checkPackageRename(opts *settings.Options, curPkg *cache.Package, f file.Ha
 		// path, in which case we should not allow renaming.
 		root := filepath.Dir(f.URI().DirPath())
 		newPkgDir = filepath.Join(root, newName)
-		empty, err := dirIsEmpty(newPkgDir)
+		exists, empty, err := dirIsEmpty(newPkgDir)
 		if err != nil {
 			return "", "", "", err
 		}
-		if !empty {
-			return "", "", "", fmt.Errorf("invalid package identifier: %q is not empty", newName)
+		// When rename moves subpackages, we merely change the directory name, which cannot
+		// happen if the target directory already exists.
+		if moveSubpackages && exists || !empty {
+			return "", "", "", fmt.Errorf("invalid package identifier: %q is not empty", newPkgDir)
 		}
 		parentPkgPath := strings.TrimSuffix(string(curPkg.Metadata().PkgPath), string(curPkg.Metadata().Name)) // leaves a trailing slash
 		newPkgPath = PackagePath(parentPkgPath + newName)
 		newPkgName = PackageName(newName)
 		return newPkgDir, newPkgName, newPkgPath, nil
 	}
-	if !opts.PackageMove {
-		return "", "", "", fmt.Errorf("a full package path is specified but internal setting 'packageMove' is not enabled")
-	}
 	// Don't allow moving packages across module boundaries.
 	curModPath := curPkg.Metadata().Module.Path
 	if !strings.HasPrefix(newName+"/", curModPath+"/") {
@@ -980,17 +978,22 @@ func checkPackageRename(opts *settings.Options, curPkg *cache.Package, f file.Ha
 		return "", "", "", fmt.Errorf("invalid package path %q", newName)
 	}
 	newPkgName = PackageName(filepath.Base(newPkgDir))
-	empty, err := dirIsEmpty(newPkgDir)
+	exists, empty, err := dirIsEmpty(newPkgDir)
 	if err != nil {
 		return "", "", "", err
 	}
-	if !empty {
-		return "", "", "", fmt.Errorf("invalid package path: %q is not empty", newName)
+	// When rename moves subpackages, we merely change the directory name, which cannot
+	// happen if the target directory already exists.
+	if moveSubpackages && exists || !empty {
+		return "", "", "", fmt.Errorf("invalid package path: %q is not empty", newPkgDir)
 	}
 	// Verify that the new package name is a valid identifier.
 	if !isValidIdentifier(string(newPkgName)) {
 		return "", "", "", fmt.Errorf("invalid package name %q", newPkgName)
 	}
+	if f.URI().Dir().Base() != string(curPkg.Metadata().Name) {
+		return "", "", "", fmt.Errorf("can't move package: package name %q does not match directory base name %q", string(curPkg.Metadata().Name), f.URI().Dir().Base())
+	}
 	return newPkgDir, newPkgName, PackagePath(newName), nil
 }
 
@@ -1006,23 +1009,24 @@ func hasIgnoredGoFiles(pkg *cache.Package) bool {
 	return false
 }
 
-// dirIsEmpty returns true if the directory does not exist or if the entries in
-// dir consist of only directories.
-func dirIsEmpty(dir string) (bool, error) {
+// dirIsEmpty returns two values: whether the directory exists and whether it is
+// empty (contains only directory entries). If it does not exist, empty is
+// always true.
+func dirIsEmpty(dir string) (exists bool, empty bool, err error) {
 	files, err := os.ReadDir(dir)
 	if err != nil {
 		if errors.Is(err, fs.ErrNotExist) {
-			return true, nil
+			return false, true, nil
 		} else {
-			return false, err
+			return false, false, err
 		}
 	}
 	for _, f := range files {
 		if !f.IsDir() {
-			return false, nil
+			return true, false, nil
 		}
 	}
-	return true, nil
+	return true, true, nil
 }
 
 // isLocal reports whether obj is local to some function.

gopls/internal/mcp/rename_symbol.go

@@ -38,7 +38,7 @@ func (h *handler) renameSymbolHandler(ctx context.Context, req *mcp.CallToolRequ
 	if err != nil {
 		return nil, nil, err
 	}
-	changes, err := golang.Rename(ctx, snapshot, fh, loc.Range.Start, params.NewName, false)
+	changes, err := golang.Rename(ctx, snapshot, fh, loc.Range.Start, params.NewName)
 	if err != nil {
 		return nil, nil, err
 	}

gopls/internal/server/rename.go

@@ -30,10 +30,7 @@ func (s *server) Rename(ctx context.Context, params *protocol.RenameParams) (*pr
 		return nil, fmt.Errorf("cannot rename in file of type %s", kind)
 	}
 
-	// TODO(mkalil): By default, we don't move subpackages on a package rename.
-	// When dialog support is enabled, the user will be able to specify whether
-	// they do want to move subpackages.
-	changes, err := golang.Rename(ctx, snapshot, fh, params.Position, params.NewName, false)
+	changes, err := golang.Rename(ctx, snapshot, fh, params.Position, params.NewName)
 	if err != nil {
 		return nil, err
 	}

gopls/internal/settings/default.go

@@ -132,7 +132,8 @@ func DefaultOptions(overrides ...func(*Options)) *Options {
 						CodeLensVendor:            true,
 						CodeLensRunGovulncheck:    true,
 					},
-					NewGoFileHeader: true,
+					NewGoFileHeader:        true,
+					RenameMovesSubpackages: false,
 				},
 			},
 			InternalOptions: InternalOptions{

gopls/internal/settings/settings.go

@@ -251,9 +251,9 @@ type UIOptions struct {
 	// and package declaration in a newly created Go file.
 	NewGoFileHeader bool
 
-	// PackageMove enables PrepareRename to send the full package path
-	// and allows users to move a package via renaming.
-	PackageMove bool `status:"experimental"`
+	// RenameMovesSubpackages enables Rename operations on packages to
+	// move subdirectories of the target package.
+	RenameMovesSubpackages bool `status:"experimental"`
 }
 
 // A CodeLensSource identifies an (algorithmic) source of code lenses.
@@ -1368,8 +1368,8 @@ func (o *Options) setOne(name string, value any) (applied []CounterPath, _ error
 	case "mcpTools":
 		return setBoolMap(&o.MCPTools, value)
 
-	case "packageMove":
-		return setBool(&o.PackageMove, value)
+	case "renameMovesSubpackages":
+		return setBool(&o.RenameMovesSubpackages, value)
 
 	// deprecated and renamed settings
 	//