Fix an issue where rename on Windows would delete the target directory.

TEST=tests updated
Bug: https://github.com/dart-lang/sdk/issues/35217, https://github.com/dart-lang/sdk/issues/47633, https://github.com/dart-lang/sdk/issues/47634
Change-Id: I9582c76e8eb75548ba2d67205353de63d110c294
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/219501
Commit-Queue: Brian Quinlan <bquinlan@google.com>
Reviewed-by: Alexander Aprelev <aam@google.com>

CHANGELOG.md

@@ -1,3 +1,11 @@
+## 2.17.0
+
+### Core libraries
+
+- **Breaking Change** [#47653](https://github.com/dart-lang/sdk/issues/47653):
+  On Windows, `Directory.rename` will no longer delete a directory if
+  `newPath` specifies one. Instead, a `FileSystemException` will be thrown.
+
 ## 2.16.0
 
 ### Core libraries
@@ -12,7 +20,8 @@
 
 #### Dart command line
 
-- **Breaking Change** [#46100][]: The standalone `dartanalyzer` tool has been
+- **Breaking Change** [#46100](https://github.com/dart-lang/sdk/issues/46100):
+  The standalone `dartanalyzer` tool has been
   marked deprecated as previously announced.
   Its replacement is the `dart analyze` command.
   Should you find any issues, or missing features, in the replacement

runtime/bin/directory_win.cc

@@ -512,16 +512,6 @@ bool Directory::Rename(Namespace* namespc,
   }
   const char* prefixed_new_dir = PrefixLongDirectoryPath(new_path);
   Utf8ToWideScope system_new_path(prefixed_new_dir);
-  ExistsResult new_exists = ExistsHelper(system_new_path.wide());
-  // MoveFile does not allow replacing existing directories. Therefore,
-  // if the new_path is currently a directory we need to delete it
-  // first.
-  if (new_exists == EXISTS) {
-    bool success = Delete(namespc, prefixed_new_dir, true);
-    if (!success) {
-      return false;
-    }
-  }
   DWORD flags = MOVEFILE_WRITE_THROUGH;
   int move_status =
       MoveFileExW(system_path.wide(), system_new_path.wide(), flags);

sdk/lib/io/directory.dart

@@ -233,20 +233,28 @@ abstract class Directory implements FileSystemEntity {
   /// Returns a `Future<Directory>` that completes
   /// with a [Directory] for the renamed directory.
   ///
-  /// If [newPath] identifies an existing directory, that directory is
-  /// removed first.
+  /// If [newPath] identifies an existing directory, then the behavior is
+  /// platform-specific. On all platforms, the future completes with an
+  /// [FileSystemException] if the existing directory is not empty. On POSIX
+  /// systems, if [newPath] identifies an existing empty directory then that
+  /// directory is deleted before this directory is renamed.
+  ///
   /// If [newPath] identifies an existing file, the operation
-  /// fails and the future completes with an exception.
+  /// fails and the future completes with a [FileSystemException].
   Future<Directory> rename(String newPath);
 
   /// Synchronously renames this directory.
   ///
   /// Returns a [Directory] for the renamed directory.
   ///
-  /// If [newPath] identifies an existing directory, that directory is
-  /// removed first.
+  /// If [newPath] identifies an existing directory, then the behavior is
+  /// platform-specific. On all platforms, a [FileSystemException] is thrown
+  /// if the existing directory is not empty. On POSIX systems, if [newPath]
+  /// identifies an existing empty directory then that directory is deleted
+  /// before this directory is renamed.
+  ///
   /// If [newPath] identifies an existing file the operation
-  /// fails and an exception is thrown.
+  /// fails and a [FileSystemException] is thrown.
   Directory renameSync(String newPath);
 
   /// A [Directory] whose path is the absolute path of [this].

tests/standalone/io/directory_rename_test.dart

@@ -19,16 +19,22 @@ testRenameToNewPath() async {
   });
 }
 
-testRenameDoesNotAdjustPath() async {
+testRenamePath() async {
+  // Verifies that the returned directory has the correct path.
   await withTempDir('testRenameToNewPath', (Directory tempDir) async {
-    final dir1 = Directory("${tempDir.path}/dir1");
-    dir1.createSync();
-    final originalPath = dir1.path;
+    final oldDir = Directory("${tempDir.path}/dir1");
+    oldDir.createSync();
 
-    dir1.renameSync("${tempDir.path}/dir2");
-    final finalPath = dir1.path;
-    Expect.isTrue(originalPath == finalPath,
-        "$originalPath != $finalPath - path should not be updated");
+    final newDir = oldDir.renameSync("${tempDir.path}/dir2");
+
+    Expect.isTrue(
+        oldDir.path == "${tempDir.path}/dir1",
+        "${oldDir.path} != '${tempDir.path}/dir1'"
+        "- path should not be updated");
+    Expect.isTrue(
+        newDir.path == "${tempDir.path}/dir2",
+        "${newDir.path} != '${tempDir.path}/dir2'"
+        "- path should be updated");
   });
 }
 
@@ -39,26 +45,8 @@ testRenameToSamePath() async {
     final file = File("${dir.path}/file");
     file.createSync();
 
-    try {
-      dir.renameSync(dir.path);
-      if (Platform.isWindows) {
-        Expect.fail('Directory.rename to same path should fail on Windows');
-      } else {
-        Expect.isTrue(file.existsSync());
-      }
-    } on FileSystemException catch (e) {
-      if (Platform.isWindows) {
-        // On Windows, the directory will be *deleted*.
-        Expect.isFalse(dir.existsSync());
-        Expect.isTrue(
-            e.osError!.message.contains('cannot find the file specified'),
-            'Unexpected error: $e');
-      } else {
-        Expect.fail('Directory.rename to same path should not fail on '
-            '${Platform.operatingSystem} (${Platform.operatingSystemVersion}): '
-            '$e');
-      }
-    }
+    dir.renameSync(dir.path);
+    Expect.isTrue(file.existsSync());
   });
 }
 
@@ -74,12 +62,12 @@ testRenameToExistingFile() async {
       dir.renameSync(file.path);
       Expect.fail('Directory.rename should fail to rename a non-directory');
     } on FileSystemException catch (e) {
-      if (Platform.isLinux || Platform.isMacOS) {
-        Expect.isTrue(e.osError!.message.contains('Not a directory'),
-            'Unexpected error: $e');
-      } else if (Platform.isWindows) {
+      if (Platform.isWindows) {
         Expect.isTrue(e.osError!.message.contains('file already exists'),
             'Unexpected error: $e');
+      } else if (Platform.isLinux || Platform.isMacOS) {
+        Expect.isTrue(e.osError!.message.contains('Not a directory'),
+            'Unexpected error: $e');
       }
     }
   });
@@ -95,9 +83,23 @@ testRenameToExistingEmptyDirectory() async {
     final dir2 = Directory("${tempDir.path}/dir2");
     dir2.createSync();
 
-    dir1.renameSync(dir2.path);
-    // Verify that the file contained in dir1 have been moved.
-    Expect.isTrue(File("${dir2.path}/file").existsSync());
+    try {
+      dir1.renameSync(dir2.path);
+      // Verify that the file contained in dir1 has been moved.
+      if (Platform.isWindows) {
+        Expect.fail(
+            'Directory.rename should fail to rename over an existing directory '
+            'on Windows');
+      } else {
+        Expect.isTrue(File("${dir2.path}/file").existsSync());
+      }
+    } on FileSystemException catch (e) {
+      if (Platform.isWindows) {
+        Expect.isTrue(e.osError!.message.contains('file already exists'));
+      } else {
+        Expect.fail('Directory.rename should allow moves to empty directories');
+      }
+    }
   });
 }
 
@@ -114,17 +116,13 @@ testRenameToExistingNonEmptyDirectory() async {
 
     try {
       dir1.renameSync(dir2.path);
-      if (Platform.isWindows) {
-        // Verify that the old directory is deleted.
-        Expect.isTrue(File("${dir2.path}/file1").existsSync());
-        Expect.isFalse(File("${dir2.path}/file2").existsSync());
-      } else {
-        Expect.fail(
-            'Directory.rename should fail to rename a non-empty directory '
-            'except on Windows');
-      }
+      Expect.fail(
+          'Directory.rename should fail to rename a non-empty directory');
     } on FileSystemException catch (e) {
-      if (Platform.isLinux || Platform.isMacOS) {
+      if (Platform.isWindows) {
+        Expect.isTrue(e.osError!.message.contains('file already exists'),
+            'Unexpected error: $e');
+      } else if (Platform.isLinux || Platform.isMacOS) {
         Expect.isTrue(e.osError!.message.contains('Directory not empty'),
             'Unexpected error: $e');
       }
@@ -134,7 +132,7 @@ testRenameToExistingNonEmptyDirectory() async {
 
 main() async {
   await testRenameToNewPath();
-  await testRenameDoesNotAdjustPath();
+  await testRenamePath();
   await testRenameToSamePath();
   await testRenameToExistingFile();
   await testRenameToExistingEmptyDirectory();

tests/standalone_2/io/directory_rename_test.dart

@@ -21,16 +21,22 @@ testRenameToNewPath() async {
   });
 }
 
-testRenameDoesNotAdjustPath() async {
+testRenamePath() async {
+  // Verifies that the returned directory has the correct path.
   await withTempDir('testRenameToNewPath', (Directory tempDir) async {
-    final dir1 = Directory("${tempDir.path}/dir1");
-    dir1.createSync();
-    final originalPath = dir1.path;
+    final oldDir = Directory("${tempDir.path}/dir1");
+    oldDir.createSync();
 
-    dir1.renameSync("${tempDir.path}/dir2");
-    final finalPath = dir1.path;
-    Expect.isTrue(originalPath == finalPath,
-        "$originalPath != $finalPath - path should not be updated");
+    final newDir = oldDir.renameSync("${tempDir.path}/dir2");
+
+    Expect.isTrue(
+        oldDir.path == "${tempDir.path}/dir1",
+        "${oldDir.path} != '${tempDir.path}/dir1'"
+        "- path should not be updated");
+    Expect.isTrue(
+        newDir.path == "${tempDir.path}/dir2",
+        "${newDir.path} != '${tempDir.path}/dir2'"
+        "- path should be updated");
   });
 }
 
@@ -41,26 +47,8 @@ testRenameToSamePath() async {
     final file = File("${dir.path}/file");
     file.createSync();
 
-    try {
-      dir.renameSync(dir.path);
-      if (Platform.isWindows) {
-        Expect.fail('Directory.rename to same path should fail on Windows');
-      } else {
-        Expect.isTrue(file.existsSync());
-      }
-    } on FileSystemException catch (e) {
-      if (Platform.isWindows) {
-        // On Windows, the directory will be *deleted*.
-        Expect.isFalse(dir.existsSync());
-        Expect.isTrue(
-            e.osError.message.contains('cannot find the file specified'),
-            'Unexpected error: $e');
-      } else {
-        Expect.fail('Directory.rename to same path should not fail on '
-            '${Platform.operatingSystem} (${Platform.operatingSystemVersion}): '
-            '$e');
-      }
-    }
+    dir.renameSync(dir.path);
+    Expect.isTrue(file.existsSync());
   });
 }
 
@@ -76,12 +64,12 @@ testRenameToExistingFile() async {
       dir.renameSync(file.path);
       Expect.fail('Directory.rename should fail to rename a non-directory');
     } on FileSystemException catch (e) {
-      if (Platform.isLinux || Platform.isMacOS) {
-        Expect.isTrue(e.osError.message.contains('Not a directory'),
-            'Unexpected error: $e');
-      } else if (Platform.isWindows) {
+      if (Platform.isWindows) {
         Expect.isTrue(e.osError.message.contains('file already exists'),
             'Unexpected error: $e');
+      } else if (Platform.isLinux || Platform.isMacOS) {
+        Expect.isTrue(e.osError.message.contains('Not a directory'),
+            'Unexpected error: $e');
       }
     }
   });
@@ -97,9 +85,23 @@ testRenameToExistingEmptyDirectory() async {
     final dir2 = Directory("${tempDir.path}/dir2");
     dir2.createSync();
 
-    dir1.renameSync(dir2.path);
-    // Verify that the file contained in dir1 has been moved.
-    Expect.isTrue(File("${dir2.path}/file").existsSync());
+    try {
+      dir1.renameSync(dir2.path);
+      // Verify that the file contained in dir1 has been moved.
+      if (Platform.isWindows) {
+        Expect.fail(
+            'Directory.rename should fail to rename over an existing directory '
+            'on Windows');
+      } else {
+        Expect.isTrue(File("${dir2.path}/file").existsSync());
+      }
+    } on FileSystemException catch (e) {
+      if (Platform.isWindows) {
+        Expect.isTrue(e.osError.message.contains('file already exists'));
+      } else {
+        Expect.fail('Directory.rename should allow moves to empty directories');
+      }
+    }
   });
 }
 
@@ -116,17 +118,13 @@ testRenameToExistingNonEmptyDirectory() async {
 
     try {
       dir1.renameSync(dir2.path);
-      if (Platform.isWindows) {
-        // Verify that the old directory is deleted.
-        Expect.isTrue(File("${dir2.path}/file1").existsSync());
-        Expect.isFalse(File("${dir2.path}/file2").existsSync());
-      } else {
-        Expect.fail(
-            'Directory.rename should fail to rename a non-empty directory '
-            'except on Windows');
-      }
+      Expect.fail(
+          'Directory.rename should fail to rename a non-empty directory');
     } on FileSystemException catch (e) {
-      if (Platform.isLinux || Platform.isMacOS) {
+      if (Platform.isWindows) {
+        Expect.isTrue(e.osError.message.contains('file already exists'),
+            'Unexpected error: $e');
+      } else if (Platform.isLinux || Platform.isMacOS) {
         Expect.isTrue(e.osError.message.contains('Directory not empty'),
             'Unexpected error: $e');
       }
@@ -136,7 +134,7 @@ testRenameToExistingNonEmptyDirectory() async {
 
 main() async {
   await testRenameToNewPath();
-  await testRenameDoesNotAdjustPath();
+  await testRenamePath();
   await testRenameToSamePath();
   await testRenameToExistingFile();
   await testRenameToExistingEmptyDirectory();