gopls/internal/golang: allow package move into empty directories

Previously, we would prevent moving a package if the resulting
package directory already exists. Instead, we should allow the
move as long as the target directory is empty -- either it doesn't
exist or it contains only directory entries.

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

Author: madelinekalil

gopls/internal/golang/rename_check.go

@@ -33,6 +33,7 @@ package golang
 //   method, not the concrete method.
 
 import (
+	"errors"
 	"fmt"
 	"go/ast"
 	"go/build"
@@ -948,10 +949,12 @@ 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)
-		_, err := os.Stat(newPkgDir)
-		if err == nil {
-			// Directory already exists, return an error.
-			return "", "", "", fmt.Errorf("invalid package identifier: %q already exists", newName)
+		empty, err := dirIsEmpty(newPkgDir)
+		if err != nil {
+			return "", "", "", err
+		}
+		if !empty {
+			return "", "", "", fmt.Errorf("invalid package identifier: %q is not empty", newName)
 		}
 		parentPkgPath := strings.TrimSuffix(string(curPkg.Metadata().PkgPath), string(curPkg.Metadata().Name)) // leaves a trailing slash
 		newPkgPath = PackagePath(parentPkgPath + newName)
@@ -977,10 +980,12 @@ func checkPackageRename(opts *settings.Options, curPkg *cache.Package, f file.Ha
 		return "", "", "", fmt.Errorf("invalid package path %q", newName)
 	}
 	newPkgName = PackageName(filepath.Base(newPkgDir))
-	_, err = os.Stat(newPkgDir)
-	if err == nil {
-		// Directory or file already exists at this path; return an error.
-		return "", "", "", fmt.Errorf("invalid package path: %q already exists", newName)
+	empty, err := dirIsEmpty(newPkgDir)
+	if err != nil {
+		return "", "", "", err
+	}
+	if !empty {
+		return "", "", "", fmt.Errorf("invalid package path: %q is not empty", newName)
 	}
 	// Verify that the new package name is a valid identifier.
 	if !isValidIdentifier(string(newPkgName)) {
@@ -1001,6 +1006,25 @@ 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) {
+	files, err := os.ReadDir(dir)
+	if err != nil {
+		if errors.Is(err, fs.ErrNotExist) {
+			return true, nil
+		} else {
+			return false, err
+		}
+	}
+	for _, f := range files {
+		if !f.IsDir() {
+			return false, nil
+		}
+	}
+	return true, nil
+}
+
 // isLocal reports whether obj is local to some function.
 // Precondition: not a struct field or interface method.
 func isLocal(obj types.Object) bool {

gopls/internal/test/integration/misc/rename_test.go

@@ -848,7 +848,7 @@ const A = 1 + nested.B
 
 		for _, badName := range []string{"$$$", "lib_test"} {
 			if err := env.Editor.Rename(env.Ctx, loc, badName); err == nil {
-				t.Errorf("Rename(lib, libx) succeeded, want non-nil error")
+				t.Errorf("Rename(lib, libx) succeeded unexpectedly")
 			}
 		}
 	})
@@ -926,17 +926,19 @@ package test2
 	WithOptions(Settings{"packageMove": true}).Run(t, files, func(t *testing.T, env *Env) {
 		env.OpenFile("test1/test1.go")
 		loc := env.RegexpSearch("test1/test1.go", "test1")
-		badNames := []string{"test2", "mod.com/test2", "differentmod.com/test2",
-			"mod.com/$$$", "mod*.$+", "mod.commmm"}
-		expectedErrMatches := []string{"invalid package identifier", "already exists",
-			"cannot move package across module boundary", "invalid package name",
-			"invalid package path", "cannot move package across module boundary"}
-		for i, badName := range badNames {
-			err := env.Editor.Rename(env.Ctx, loc, badName)
+		for _, test := range []struct{ badName, expectedErrMatch string }{
+			{"test2", "invalid package identifier"},
+			{"mod.com/test2", "not empty"},
+			{"differentmod.com/test2", "cannot move package across module boundary"},
+			{"mod.com/$$$", "invalid package name"},
+			{"mod*.$+", "invalid package path"},
+			{"mod.commmm", "cannot move package across module boundary"},
+		} {
+			err := env.Editor.Rename(env.Ctx, loc, test.badName)
 			if err == nil {
-				t.Errorf("Rename to %s succeeded, want non-nil error", badName)
-			} else if !strings.Contains(err.Error(), expectedErrMatches[i]) {
-				t.Errorf("Rename to %s produced incorrect error message, got %s, want %s", badName, err.Error(), expectedErrMatches[i])
+				t.Errorf("Rename to %s succeeded unexpectedly", test.badName)
+			} else if !strings.Contains(err.Error(), test.expectedErrMatch) {
+				t.Errorf("Rename to %s produced incorrect error message, got %s, want %s", test.badName, err.Error(), test.expectedErrMatch)
 			}
 		}
 	})
@@ -958,15 +960,18 @@ package test2
 	WithOptions(Settings{"packageMove": false}).Run(t, files, func(t *testing.T, env *Env) {
 		env.OpenFile("test1/test1.go")
 		loc := env.RegexpSearch("test1/test1.go", "test1")
-		badNames := []string{"test2", "mod.com/test3"}
-		expectedErrMatches := []string{"invalid package identifier", "setting 'packageMove' is not enabled"}
-		for i, badName := range badNames {
-			err := env.Editor.Rename(env.Ctx, loc, badName)
+
+		for _, test := range []struct{ badName, expectedErrMatch string }{
+			{"test2", "invalid package identifier"},
+			{"mod.com/test3", "setting 'packageMove' is not enabled"},
+		} {
+			err := env.Editor.Rename(env.Ctx, loc, test.badName)
 			if err == nil {
-				t.Errorf("Rename to %s succeeded, want non-nil error", badName)
-			} else if !strings.Contains(err.Error(), expectedErrMatches[i]) {
-				t.Errorf("Rename to %s produced incorrect error message, got %s, want %s", badName, err.Error(), expectedErrMatches[i])
+				t.Errorf("Rename to %s succeeded unexpectedly", test.badName)
+			} else if !strings.Contains(err.Error(), test.expectedErrMatch) {
+				t.Errorf("Rename to %s produced incorrect error message, got %s, want %s", test.badName, err.Error(), test.expectedErrMatch)
 			}
+
 		}
 	})
 }

gopls/internal/test/marker/testdata/rename/packagedecl.txt

@@ -34,7 +34,7 @@ go 1.20
 package one //@ rename("one", "golang.org/lsptests/rename/one", sameName), rename("one", "golang.org/lsptests/rename/two", newNameSameDir), renameerr("one", "golang.org/lsptests/otherdir/one", re"cannot move package across module boundary")
 
 -- three/three.go --
-package three //@ renameerr("three", "golang.org/lsptests/othermod/three", re"cannot move package across module boundary"), renameerr("three", "golang.org/lsptests/rename/one", re"already exists")
+package three //@ renameerr("three", "golang.org/lsptests/othermod/three", re"cannot move package across module boundary"), renameerr("three", "golang.org/lsptests/rename/one", re"not empty")
 
 
 -- six/six.go --
@@ -91,6 +91,9 @@ import (
 -- seven/eight/other/other.go --
 package other //@ rename("other", "golang.org/lsptests/rename/seven/eight/other/newDir1/newDir2", createNewDir)
 
+-- seven/eight/other/newDir1/newDir2/newDir3/test.go --
+package newDir3 // verifies that we can move the package above into a directory that already exists but is empty
+
 -- fix/fix_test.go --
 package fix_test
 -- othermoddir/othermod/other.go --