Windows: make File.delete remove symlink directories, not Dir.delete

spec/std/dir_spec.cr

@@ -143,17 +143,29 @@ describe "Dir" do
     end
   end
 
-  it "tests delete with an nonexistent path" do
-    with_tempfile("nonexistent") do |path|
-      expect_raises(File::NotFoundError, "Unable to remove directory: '#{path.inspect_unquoted}'") do
-        Dir.delete(path)
+  describe ".delete" do
+    it "raises with an nonexistent path" do
+      with_tempfile("nonexistent") do |path|
+        expect_raises(File::NotFoundError, "Unable to remove directory: '#{path.inspect_unquoted}'") do
+          Dir.delete(path)
+        end
+      end
+    end
+
+    it "raises with a path that cannot be removed" do
+      expect_raises(File::Error, "Unable to remove directory: '#{datapath.inspect_unquoted}'") do
+        Dir.delete(datapath)
       end
     end
-  end
 
-  it "tests delete with a path that cannot be removed" do
-    expect_raises(File::Error, "Unable to remove directory: '#{datapath.inspect_unquoted}'") do
-      Dir.delete(datapath)
+    it "raises with symlink directory" do
+      with_tempfile("delete-target-directory", "delete-symlink-directory") do |target_path, symlink_path|
+        Dir.mkdir(target_path)
+        File.symlink(target_path, symlink_path)
+        expect_raises(File::Error) do
+          Dir.delete(symlink_path)
+        end
+      end
     end
   end
 
@@ -351,21 +363,20 @@ describe "Dir" do
     end
 
     it "matches symlinks" do
-      link = datapath("f1_link.txt")
-      non_link = datapath("non_link.txt")
+      with_tempfile "symlinks" do |path|
+        Dir.mkdir_p(path)
 
-      File.symlink(datapath("dir", "f1.txt"), link)
-      File.symlink(datapath("dir", "nonexisting"), non_link)
+        link = Path[path, "f1_link.txt"]
+        non_link = Path[path, "non_link.txt"]
 
-      begin
-        Dir["#{datapath}/*_link.txt"].sort.should eq [
-          datapath("f1_link.txt"),
-          datapath("non_link.txt"),
+        File.symlink(datapath("dir", "f1.txt"), link)
+        File.symlink(datapath("dir", "nonexisting"), non_link)
+
+        Dir["#{path}/*_link.txt"].sort.should eq [
+          link.to_s,
+          non_link.to_s,
         ].sort
-        Dir["#{datapath}/non_link.txt"].should eq [datapath("non_link.txt")]
-      ensure
-        File.delete link
-        File.delete non_link
+        Dir["#{path}/non_link.txt"].should eq [non_link.to_s]
       end
     end
 
@@ -381,18 +392,10 @@ describe "Dir" do
         File.write(non_link, "")
         File.symlink(target, link_dir)
 
-        begin
-          Dir.glob("#{path}/glob/*/a.txt").sort.should eq [] of String
-          Dir.glob("#{path}/glob/*/a.txt", follow_symlinks: true).sort.should eq [
-            File.join(path, "glob", "dir", "a.txt"),
-          ]
-        ensure
-          # FIXME: `with_tempfile` will delete this symlink directory using
-          # `File.delete` otherwise, see #13194
-          {% if flag?(:win32) %}
-            Dir.delete(link_dir)
-          {% end %}
-        end
+        Dir.glob("#{path}/glob/*/a.txt").sort.should eq [] of String
+        Dir.glob("#{path}/glob/*/a.txt", follow_symlinks: true).sort.should eq [
+          File.join(path, "glob", "dir", "a.txt"),
+        ]
       end
     end
 

spec/std/file_spec.cr

@@ -551,6 +551,14 @@ describe "File" do
         end
       end
     end
+
+    it "deletes a symlink directory" do
+      with_tempfile("delete-target-directory", "delete-symlink-directory") do |target_path, symlink_path|
+        Dir.mkdir(target_path)
+        File.symlink(target_path, symlink_path)
+        File.delete(symlink_path)
+      end
+    end
   end
 
   describe "rename" do

spec/std/file_utils_spec.cr

@@ -239,7 +239,7 @@ describe "FileUtils" do
       end
     end
 
-    pending_win32 "doesn't follow symlinks" do
+    it "doesn't follow symlinks" do
       with_tempfile("rm_r-removed", "rm_r-linked") do |removed_path, linked_path|
         link_path = File.join(removed_path, "link")
         file_path = File.join(linked_path, "file")

src/crystal/system/win32/dir.cr

@@ -132,12 +132,24 @@ module Crystal::System::Dir
   end
 
   def self.delete(path : String, *, raise_on_missing : Bool) : Bool
-    return true if LibC._wrmdir(System.to_wstr(path)) == 0
+    win_path = System.to_wstr(path)
 
-    if !raise_on_missing && Errno.value == Errno::ENOENT
-      false
-    else
-      raise ::File::Error.from_errno("Unable to remove directory", file: path)
+    attributes = LibC.GetFileAttributesW(win_path)
+    if attributes == LibC::INVALID_FILE_ATTRIBUTES
+      File.check_not_found_error("Unable to remove directory", path)
+      raise ::File::Error.from_os_error("Unable to remove directory", Errno::ENOENT, file: path) if raise_on_missing
+      return false
+    end
+
+    # all reparse point directories should be deleted like a directory, not just
+    # symbolic links, so we don't care about the reparse tag here
+    if attributes.bits_set?(LibC::FILE_ATTRIBUTE_REPARSE_POINT) && attributes.bits_set?(LibC::FILE_ATTRIBUTE_DIRECTORY)
+      # maintain consistency with POSIX, and treat all reparse points (including
+      # symbolic links) as non-directories
+      raise ::File::Error.new("Cannot remove directory that is a reparse point: '#{path.inspect_unquoted}'", file: path)
     end
+
+    return true if LibC._wrmdir(win_path) == 0
+    raise ::File::Error.from_errno("Unable to remove directory", file: path)
   end
 end

src/crystal/system/win32/file.cr

@@ -114,7 +114,7 @@ module Crystal::System::File
     WinError::ERROR_INVALID_NAME,
   }
 
-  private def self.check_not_found_error(message, path)
+  def self.check_not_found_error(message, path)
     error = WinError.value
     if NOT_FOUND_ERRORS.includes? error
       nil
@@ -242,13 +242,21 @@ module Crystal::System::File
   end
 
   def self.delete(path : String, *, raise_on_missing : Bool) : Bool
-    if LibC._wunlink(System.to_wstr(path)) == 0
-      true
-    elsif !raise_on_missing && Errno.value == Errno::ENOENT
-      false
-    else
-      raise ::File::Error.from_errno("Error deleting file", file: path)
+    win_path = System.to_wstr(path)
+
+    attributes = LibC.GetFileAttributesW(win_path)
+    if attributes == LibC::INVALID_FILE_ATTRIBUTES
+      check_not_found_error("Error deleting file", path)
+      raise ::File::Error.from_os_error("Error deleting file", Errno::ENOENT, file: path) if raise_on_missing
+      return false
     end
+
+    # all reparse point directories should be deleted like a directory, not just
+    # symbolic links, so we don't care about the reparse tag here
+    is_reparse_dir = attributes.bits_set?(LibC::FILE_ATTRIBUTE_REPARSE_POINT) && attributes.bits_set?(LibC::FILE_ATTRIBUTE_DIRECTORY)
+    result = is_reparse_dir ? LibC._wrmdir(win_path) : LibC._wunlink(win_path)
+    return true if result == 0
+    raise ::File::Error.from_errno("Error deleting file", file: path)
   end
 
   private REALPATH_SYMLINK_LIMIT = 100

src/dir.cr

@@ -292,13 +292,21 @@ class Dir
     mkdir(path, mode) unless Dir.exists?(path)
   end
 
-  # Removes the directory at the given path.
+  # Removes the directory at *path*. Raises `File::Error` on failure.
+  #
+  # On Windows, also raises `File::Error` if *path* points to a directory that
+  # is a reparse point, such as a symbolic link. Those directories can be
+  # deleted using `File.delete` instead.
   def self.delete(path : Path | String) : Nil
     Crystal::System::Dir.delete(path.to_s, raise_on_missing: true)
   end
 
-  # Removes the directory at the given path.
-  # Returns `false` if the directory does not exist.
+  # Removes the directory at *path*, or returns `false` if the directory does
+  # not exist. Raises `File::Error` on other kinds of failure.
+  #
+  # On Windows, also raises `File::Error` if *path* points to a directory that
+  # is a reparse point, such as a symbolic link. Those directories can be
+  # deleted using `File.delete?` instead.
   def self.delete?(path : Path | String) : Bool
     Crystal::System::Dir.delete(path.to_s, raise_on_missing: false)
   end

src/file.cr

@@ -360,7 +360,10 @@ class File < IO::FileDescriptor
     Crystal::System::File.chmod(path.to_s, permissions)
   end
 
-  # Deletes the file at *path*. Deleting non-existent file will raise an exception.
+  # Deletes the file at *path*. Raises `File::Error` on failure.
+  #
+  # On Windows, this also deletes reparse points, including symbolic links,
+  # regardless of whether the reparse point is a directory.
   #
   # ```
   # File.write("foo", "")
@@ -371,8 +374,11 @@ class File < IO::FileDescriptor
     Crystal::System::File.delete(path.to_s, raise_on_missing: true)
   end
 
-  # Deletes the file at *path*.
-  # Returns `false` if the file does not exist.
+  # Deletes the file at *path*, or returns `false` if the file does not exist.
+  # Raises `File::Error` on other kinds of failure.
+  #
+  # On Windows, this also deletes reparse points, including symbolic links,
+  # regardless of whether the reparse point is a directory.
   #
   # ```
   # File.write("foo", "")