fix: Deno.remove() to properly remove dangling symlinks (#3860)

For some reason, the unit tests for Deno.remove() were not being imported to 
unit_tests.ts and, consequently, not being executed. Thus, I imported them, 
refactored some existent ones and wrote new ones for the symlink removal case.

Since the creation of a symlink is not implemented for Windows yet, assertions
that consider this state were added when the tests are executed in this OS.

cli/js/remove.ts

@@ -6,7 +6,7 @@ export interface RemoveOption {
   recursive?: boolean;
 }
 
-/** Removes the named file or directory synchronously. Would throw
+/** Removes the named file, directory or symlink synchronously. Would throw
  * error if permission denied, not found, or directory not empty if `recursive`
  * set to false.
  * `recursive` is set to false by default.
@@ -17,7 +17,7 @@ export function removeSync(path: string, options: RemoveOption = {}): void {
   sendSync(dispatch.OP_REMOVE, { path, recursive: !!options.recursive });
 }
 
-/** Removes the named file or directory. Would throw error if
+/** Removes the named file, directory or symlink. Would throw error if
  * permission denied, not found, or directory not empty if `recursive` set
  * to false.
  * `recursive` is set to false by default.

cli/js/remove_test.ts

@@ -3,9 +3,9 @@ import { testPerm, assert, assertEquals } from "./test_util.ts";
 
 // SYNC
 
-testPerm({ write: true }, function removeSyncDirSuccess(): void {
+testPerm({ write: true, read: true }, function removeSyncDirSuccess(): void {
   // REMOVE EMPTY DIRECTORY
-  const path = Deno.makeTempDirSync() + "/dir/subdir";
+  const path = Deno.makeTempDirSync() + "/subdir";
   Deno.mkdirSync(path);
   const pathInfo = Deno.statSync(path);
   assert(pathInfo.isDirectory()); // check exist first
@@ -22,7 +22,7 @@ testPerm({ write: true }, function removeSyncDirSuccess(): void {
   assertEquals(err.name, "NotFound");
 });
 
-testPerm({ write: true }, function removeSyncFileSuccess(): void {
+testPerm({ write: true, read: true }, function removeSyncFileSuccess(): void {
   // REMOVE FILE
   const enc = new TextEncoder();
   const data = enc.encode("Hello");
@@ -43,11 +43,11 @@ testPerm({ write: true }, function removeSyncFileSuccess(): void {
   assertEquals(err.name, "NotFound");
 });
 
-testPerm({ write: true }, function removeSyncFail(): void {
+testPerm({ write: true, read: true }, function removeSyncFail(): void {
   // NON-EMPTY DIRECTORY
   const path = Deno.makeTempDirSync() + "/dir/subdir";
   const subPath = path + "/subsubdir";
-  Deno.mkdirSync(path);
+  Deno.mkdirSync(path, { recursive: true });
   Deno.mkdirSync(subPath);
   const pathInfo = Deno.statSync(path);
   assert(pathInfo.isDirectory()); // check exist first
@@ -74,6 +74,72 @@ testPerm({ write: true }, function removeSyncFail(): void {
   assertEquals(err.name, "NotFound");
 });
 
+testPerm(
+  { write: true, read: true },
+  function removeSyncDanglingSymlinkSuccess(): void {
+    const danglingSymlinkPath = Deno.makeTempDirSync() + "/dangling_symlink";
+    // TODO(#3832): Remove "Not Implemented" error checking when symlink creation is implemented for Windows
+    let errOnWindows;
+    try {
+      Deno.symlinkSync("unexistent_file", danglingSymlinkPath);
+    } catch (err) {
+      errOnWindows = err;
+    }
+    if (Deno.build.os === "win") {
+      assertEquals(errOnWindows.kind, Deno.ErrorKind.Other);
+      assertEquals(errOnWindows.message, "Not implemented");
+    } else {
+      const pathInfo = Deno.lstatSync(danglingSymlinkPath);
+      assert(pathInfo.isSymlink());
+      Deno.removeSync(danglingSymlinkPath);
+      let err;
+      try {
+        Deno.lstatSync(danglingSymlinkPath);
+      } catch (e) {
+        err = e;
+      }
+      assertEquals(err.kind, Deno.ErrorKind.NotFound);
+      assertEquals(err.name, "NotFound");
+    }
+  }
+);
+
+testPerm(
+  { write: true, read: true },
+  function removeSyncValidSymlinkSuccess(): void {
+    const encoder = new TextEncoder();
+    const data = encoder.encode("Test");
+    const tempDir = Deno.makeTempDirSync();
+    const filePath = tempDir + "/test.txt";
+    const validSymlinkPath = tempDir + "/valid_symlink";
+    Deno.writeFileSync(filePath, data, { perm: 0o666 });
+    // TODO(#3832): Remove "Not Implemented" error checking when symlink creation is implemented for Windows
+    let errOnWindows;
+    try {
+      Deno.symlinkSync(filePath, validSymlinkPath);
+    } catch (err) {
+      errOnWindows = err;
+    }
+    if (Deno.build.os === "win") {
+      assertEquals(errOnWindows.kind, Deno.ErrorKind.Other);
+      assertEquals(errOnWindows.message, "Not implemented");
+    } else {
+      const symlinkPathInfo = Deno.statSync(validSymlinkPath);
+      assert(symlinkPathInfo.isFile());
+      Deno.removeSync(validSymlinkPath);
+      let err;
+      try {
+        Deno.statSync(validSymlinkPath);
+      } catch (e) {
+        err = e;
+      }
+      Deno.removeSync(filePath);
+      assertEquals(err.kind, Deno.ErrorKind.NotFound);
+      assertEquals(err.name, "NotFound");
+    }
+  }
+);
+
 testPerm({ write: false }, function removeSyncPerm(): void {
   let err;
   try {
@@ -85,10 +151,10 @@ testPerm({ write: false }, function removeSyncPerm(): void {
   assertEquals(err.name, "PermissionDenied");
 });
 
-testPerm({ write: true }, function removeAllSyncDirSuccess(): void {
+testPerm({ write: true, read: true }, function removeAllSyncDirSuccess(): void {
   // REMOVE EMPTY DIRECTORY
   let path = Deno.makeTempDirSync() + "/dir/subdir";
-  Deno.mkdirSync(path);
+  Deno.mkdirSync(path, { recursive: true });
   let pathInfo = Deno.statSync(path);
   assert(pathInfo.isDirectory()); // check exist first
   Deno.removeSync(path, { recursive: true }); // remove
@@ -105,7 +171,7 @@ testPerm({ write: true }, function removeAllSyncDirSuccess(): void {
   // REMOVE NON-EMPTY DIRECTORY
   path = Deno.makeTempDirSync() + "/dir/subdir";
   const subPath = path + "/subsubdir";
-  Deno.mkdirSync(path);
+  Deno.mkdirSync(path, { recursive: true });
   Deno.mkdirSync(subPath);
   pathInfo = Deno.statSync(path);
   assert(pathInfo.isDirectory()); // check exist first
@@ -123,26 +189,29 @@ testPerm({ write: true }, function removeAllSyncDirSuccess(): void {
   assertEquals(err.name, "NotFound");
 });
 
-testPerm({ write: true }, function removeAllSyncFileSuccess(): void {
-  // REMOVE FILE
-  const enc = new TextEncoder();
-  const data = enc.encode("Hello");
-  const filename = Deno.makeTempDirSync() + "/test.txt";
-  Deno.writeFileSync(filename, data, { perm: 0o666 });
-  const fileInfo = Deno.statSync(filename);
-  assert(fileInfo.isFile()); // check exist first
-  Deno.removeSync(filename, { recursive: true }); // remove
-  // We then check again after remove
-  let err;
-  try {
-    Deno.statSync(filename);
-  } catch (e) {
-    err = e;
+testPerm(
+  { write: true, read: true },
+  function removeAllSyncFileSuccess(): void {
+    // REMOVE FILE
+    const enc = new TextEncoder();
+    const data = enc.encode("Hello");
+    const filename = Deno.makeTempDirSync() + "/test.txt";
+    Deno.writeFileSync(filename, data, { perm: 0o666 });
+    const fileInfo = Deno.statSync(filename);
+    assert(fileInfo.isFile()); // check exist first
+    Deno.removeSync(filename, { recursive: true }); // remove
+    // We then check again after remove
+    let err;
+    try {
+      Deno.statSync(filename);
+    } catch (e) {
+      err = e;
+    }
+    // File is gone
+    assertEquals(err.kind, Deno.ErrorKind.NotFound);
+    assertEquals(err.name, "NotFound");
   }
-  // File is gone
-  assertEquals(err.kind, Deno.ErrorKind.NotFound);
-  assertEquals(err.name, "NotFound");
-});
+);
 
 testPerm({ write: true }, function removeAllSyncFail(): void {
   // NON-EXISTENT DIRECTORY/FILE
@@ -170,51 +239,59 @@ testPerm({ write: false }, function removeAllSyncPerm(): void {
 
 // ASYNC
 
-testPerm({ write: true }, async function removeDirSuccess(): Promise<void> {
-  // REMOVE EMPTY DIRECTORY
-  const path = Deno.makeTempDirSync() + "/dir/subdir";
-  Deno.mkdirSync(path);
-  const pathInfo = Deno.statSync(path);
-  assert(pathInfo.isDirectory()); // check exist first
-  await Deno.remove(path); // remove
-  // We then check again after remove
-  let err;
-  try {
-    Deno.statSync(path);
-  } catch (e) {
-    err = e;
+testPerm(
+  { write: true, read: true },
+  async function removeDirSuccess(): Promise<void> {
+    // REMOVE EMPTY DIRECTORY
+    const path = Deno.makeTempDirSync() + "/dir/subdir";
+    Deno.mkdirSync(path, { recursive: true });
+    const pathInfo = Deno.statSync(path);
+    assert(pathInfo.isDirectory()); // check exist first
+    await Deno.remove(path); // remove
+    // We then check again after remove
+    let err;
+    try {
+      Deno.statSync(path);
+    } catch (e) {
+      err = e;
+    }
+    // Directory is gone
+    assertEquals(err.kind, Deno.ErrorKind.NotFound);
+    assertEquals(err.name, "NotFound");
   }
-  // Directory is gone
-  assertEquals(err.kind, Deno.ErrorKind.NotFound);
-  assertEquals(err.name, "NotFound");
-});
+);
 
-testPerm({ write: true }, async function removeFileSuccess(): Promise<void> {
-  // REMOVE FILE
-  const enc = new TextEncoder();
-  const data = enc.encode("Hello");
-  const filename = Deno.makeTempDirSync() + "/test.txt";
-  Deno.writeFileSync(filename, data, { perm: 0o666 });
-  const fileInfo = Deno.statSync(filename);
-  assert(fileInfo.isFile()); // check exist first
-  await Deno.remove(filename); // remove
-  // We then check again after remove
-  let err;
-  try {
-    Deno.statSync(filename);
-  } catch (e) {
-    err = e;
+testPerm(
+  { write: true, read: true },
+  async function removeFileSuccess(): Promise<void> {
+    // REMOVE FILE
+    const enc = new TextEncoder();
+    const data = enc.encode("Hello");
+    const filename = Deno.makeTempDirSync() + "/test.txt";
+    Deno.writeFileSync(filename, data, { perm: 0o666 });
+    const fileInfo = Deno.statSync(filename);
+    assert(fileInfo.isFile()); // check exist first
+    await Deno.remove(filename); // remove
+    // We then check again after remove
+    let err;
+    try {
+      Deno.statSync(filename);
+    } catch (e) {
+      err = e;
+    }
+    // File is gone
+    assertEquals(err.kind, Deno.ErrorKind.NotFound);
+    assertEquals(err.name, "NotFound");
   }
-  // File is gone
-  assertEquals(err.kind, Deno.ErrorKind.NotFound);
-  assertEquals(err.name, "NotFound");
-});
+);
 
-testPerm({ write: true }, async function removeFail(): Promise<void> {
+testPerm({ write: true, read: true }, async function removeFail(): Promise<
+  void
+> {
   // NON-EMPTY DIRECTORY
   const path = Deno.makeTempDirSync() + "/dir/subdir";
   const subPath = path + "/subsubdir";
-  Deno.mkdirSync(path);
+  Deno.mkdirSync(path, { recursive: true });
   Deno.mkdirSync(subPath);
   const pathInfo = Deno.statSync(path);
   assert(pathInfo.isDirectory()); // check exist first
@@ -240,6 +317,72 @@ testPerm({ write: true }, async function removeFail(): Promise<void> {
   assertEquals(err.name, "NotFound");
 });
 
+testPerm(
+  { write: true, read: true },
+  async function removeDanglingSymlinkSuccess(): Promise<void> {
+    const danglingSymlinkPath = Deno.makeTempDirSync() + "/dangling_symlink";
+    // TODO(#3832): Remove "Not Implemented" error checking when symlink creation is implemented for Windows
+    let errOnWindows;
+    try {
+      Deno.symlinkSync("unexistent_file", danglingSymlinkPath);
+    } catch (e) {
+      errOnWindows = e;
+    }
+    if (Deno.build.os === "win") {
+      assertEquals(errOnWindows.kind, Deno.ErrorKind.Other);
+      assertEquals(errOnWindows.message, "Not implemented");
+    } else {
+      const pathInfo = Deno.lstatSync(danglingSymlinkPath);
+      assert(pathInfo.isSymlink());
+      await Deno.remove(danglingSymlinkPath);
+      let err;
+      try {
+        Deno.lstatSync(danglingSymlinkPath);
+      } catch (e) {
+        err = e;
+      }
+      assertEquals(err.kind, Deno.ErrorKind.NotFound);
+      assertEquals(err.name, "NotFound");
+    }
+  }
+);
+
+testPerm(
+  { write: true, read: true },
+  async function removeValidSymlinkSuccess(): Promise<void> {
+    const encoder = new TextEncoder();
+    const data = encoder.encode("Test");
+    const tempDir = Deno.makeTempDirSync();
+    const filePath = tempDir + "/test.txt";
+    const validSymlinkPath = tempDir + "/valid_symlink";
+    Deno.writeFileSync(filePath, data, { perm: 0o666 });
+    // TODO(#3832): Remove "Not Implemented" error checking when symlink creation is implemented for Windows
+    let errOnWindows;
+    try {
+      Deno.symlinkSync(filePath, validSymlinkPath);
+    } catch (e) {
+      errOnWindows = e;
+    }
+    if (Deno.build.os === "win") {
+      assertEquals(errOnWindows.kind, Deno.ErrorKind.Other);
+      assertEquals(errOnWindows.message, "Not implemented");
+    } else {
+      const symlinkPathInfo = Deno.statSync(validSymlinkPath);
+      assert(symlinkPathInfo.isFile());
+      await Deno.remove(validSymlinkPath);
+      let err;
+      try {
+        Deno.statSync(validSymlinkPath);
+      } catch (e) {
+        err = e;
+      }
+      Deno.removeSync(filePath);
+      assertEquals(err.kind, Deno.ErrorKind.NotFound);
+      assertEquals(err.name, "NotFound");
+    }
+  }
+);
+
 testPerm({ write: false }, async function removePerm(): Promise<void> {
   let err;
   try {
@@ -251,64 +394,70 @@ testPerm({ write: false }, async function removePerm(): Promise<void> {
   assertEquals(err.name, "PermissionDenied");
 });
 
-testPerm({ write: true }, async function removeAllDirSuccess(): Promise<void> {
-  // REMOVE EMPTY DIRECTORY
-  let path = Deno.makeTempDirSync() + "/dir/subdir";
-  Deno.mkdirSync(path);
-  let pathInfo = Deno.statSync(path);
-  assert(pathInfo.isDirectory()); // check exist first
-  await Deno.remove(path, { recursive: true }); // remove
-  // We then check again after remove
-  let err;
-  try {
-    Deno.statSync(path);
-  } catch (e) {
-    err = e;
-  }
-  // Directory is gone
-  assertEquals(err.kind, Deno.ErrorKind.NotFound);
-  assertEquals(err.name, "NotFound");
-  // REMOVE NON-EMPTY DIRECTORY
-  path = Deno.makeTempDirSync() + "/dir/subdir";
-  const subPath = path + "/subsubdir";
-  Deno.mkdirSync(path);
-  Deno.mkdirSync(subPath);
-  pathInfo = Deno.statSync(path);
-  assert(pathInfo.isDirectory()); // check exist first
-  const subPathInfo = Deno.statSync(subPath);
-  assert(subPathInfo.isDirectory()); // check exist first
-  await Deno.remove(path, { recursive: true }); // remove
-  // We then check parent directory again after remove
-  try {
-    Deno.statSync(path);
-  } catch (e) {
-    err = e;
+testPerm(
+  { write: true, read: true },
+  async function removeAllDirSuccess(): Promise<void> {
+    // REMOVE EMPTY DIRECTORY
+    let path = Deno.makeTempDirSync() + "/dir/subdir";
+    Deno.mkdirSync(path, { recursive: true });
+    let pathInfo = Deno.statSync(path);
+    assert(pathInfo.isDirectory()); // check exist first
+    await Deno.remove(path, { recursive: true }); // remove
+    // We then check again after remove
+    let err;
+    try {
+      Deno.statSync(path);
+    } catch (e) {
+      err = e;
+    }
+    // Directory is gone
+    assertEquals(err.kind, Deno.ErrorKind.NotFound);
+    assertEquals(err.name, "NotFound");
+    // REMOVE NON-EMPTY DIRECTORY
+    path = Deno.makeTempDirSync() + "/dir/subdir";
+    const subPath = path + "/subsubdir";
+    Deno.mkdirSync(path, { recursive: true });
+    Deno.mkdirSync(subPath);
+    pathInfo = Deno.statSync(path);
+    assert(pathInfo.isDirectory()); // check exist first
+    const subPathInfo = Deno.statSync(subPath);
+    assert(subPathInfo.isDirectory()); // check exist first
+    await Deno.remove(path, { recursive: true }); // remove
+    // We then check parent directory again after remove
+    try {
+      Deno.statSync(path);
+    } catch (e) {
+      err = e;
+    }
+    // Directory is gone
+    assertEquals(err.kind, Deno.ErrorKind.NotFound);
+    assertEquals(err.name, "NotFound");
   }
-  // Directory is gone
-  assertEquals(err.kind, Deno.ErrorKind.NotFound);
-  assertEquals(err.name, "NotFound");
-});
+);
 
-testPerm({ write: true }, async function removeAllFileSuccess(): Promise<void> {
-  // REMOVE FILE
-  const enc = new TextEncoder();
-  const data = enc.encode("Hello");
-  const filename = Deno.makeTempDirSync() + "/test.txt";
-  Deno.writeFileSync(filename, data, { perm: 0o666 });
-  const fileInfo = Deno.statSync(filename);
-  assert(fileInfo.isFile()); // check exist first
-  await Deno.remove(filename, { recursive: true }); // remove
-  // We then check again after remove
-  let err;
-  try {
-    Deno.statSync(filename);
-  } catch (e) {
-    err = e;
+testPerm(
+  { write: true, read: true },
+  async function removeAllFileSuccess(): Promise<void> {
+    // REMOVE FILE
+    const enc = new TextEncoder();
+    const data = enc.encode("Hello");
+    const filename = Deno.makeTempDirSync() + "/test.txt";
+    Deno.writeFileSync(filename, data, { perm: 0o666 });
+    const fileInfo = Deno.statSync(filename);
+    assert(fileInfo.isFile()); // check exist first
+    await Deno.remove(filename, { recursive: true }); // remove
+    // We then check again after remove
+    let err;
+    try {
+      Deno.statSync(filename);
+    } catch (e) {
+      err = e;
+    }
+    // File is gone
+    assertEquals(err.kind, Deno.ErrorKind.NotFound);
+    assertEquals(err.name, "NotFound");
   }
-  // File is gone
-  assertEquals(err.kind, Deno.ErrorKind.NotFound);
-  assertEquals(err.name, "NotFound");
-});
+);
 
 testPerm({ write: true }, async function removeAllFail(): Promise<void> {
   // NON-EXISTENT DIRECTORY/FILE

cli/js/unit_tests.ts

@@ -39,6 +39,7 @@ import "./realpath_test.ts";
 import "./read_dir_test.ts";
 import "./read_file_test.ts";
 import "./read_link_test.ts";
+import "./remove_test.ts";
 import "./rename_test.ts";
 import "./request_test.ts";
 import "./resources_test.ts";

cli/ops/fs.rs

@@ -168,8 +168,9 @@ fn op_remove(
   let is_sync = args.promise_id.is_none();
   blocking_json(is_sync, move || {
     debug!("op_remove {}", path.display());
-    let metadata = fs::metadata(&path)?;
-    if metadata.is_file() {
+    let metadata = fs::symlink_metadata(&path)?;
+    let file_type = metadata.file_type();
+    if file_type.is_file() || file_type.is_symlink() {
       fs::remove_file(&path)?;
     } else if recursive {
       remove_dir_all(&path)?;

std/encoding/testdata/cargo.toml

@@ -38,7 +38,7 @@ libc = "0.2.49"
 log = "0.4.6"
 rand = "0.6.5"
 regex = "1.1.0"
-remove_dir_all = "0.5.1"
+remove_dir_all = "0.5.2"
 ring = "0.14.6"
 rustyline = "3.0.0"
 serde_json = "1.0.38"

std/encoding/toml_test.ts

@@ -276,7 +276,7 @@ test({
         log: "0.4.6",
         rand: "0.6.5",
         regex: "1.1.0",
-        remove_dir_all: "0.5.1",
+        remove_dir_all: "0.5.2",
         ring: "0.14.6",
         rustyline: "3.0.0",
         serde_json: "1.0.38",