Skip to content

Standardize system error codes for File::Error #16024

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Standardize system error codes for File::Error #16024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
62f4a20
Refactor `File::Error.new_from_os_error`
straight-shoota Jun 16, 2025
e0098dd
Standardize system codes for file errors
straight-shoota Jul 23, 2025
a50d7f6
Add `WinError::ERROR_FILE_EXISTS` to `AccessDeniedError`
straight-shoota Jul 23, 2025
72fd6ce
Refactor to `.os_error?` methods
straight-shoota Jul 23, 2025
c1ee613
Remove unnecessary macro block
straight-shoota Aug 4, 2025
4bf5449
Merge branch 'master' into feat/filename-too-long-error
straight-shoota Aug 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions spec/std/file_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,41 @@ describe "File" do
end
end

long_path = "a" * 1000
describe ".info" do
it "raises for too long pathname" do
expect_raises(File::NotFoundError, /Unable to get file info: '#{long_path}': (File ?name too long|The system cannot find the path specified)/) do
File.info(long_path)
end
end

it "raises for invalid pathname" do
expect_raises(File::NotFoundError, /Unable to get file info: '': (No such file or directory|The system cannot find the path specified)/) do
File.info("")
end
end

it "raises for invalid pathname" do
expect_raises(File::NotFoundError, /Unable to get file info: '<': (No such file or directory|The filename, directory name, or volume label syntax is incorrect)/) do
File.info("<")
end
end
end

describe ".info?" do
it "returns nil for too long pathname" do
File.info?(long_path).should be_nil
end

it "returns nil for invalid pathname" do
File.info?("").should be_nil
end

it "returns nil for invalid pathname" do
File.info?("<").should be_nil
end
end

describe "File::Info" do
it "gets for this file" do
info = File.info(datapath("test_file.txt"))
Expand Down
6 changes: 6 additions & 0 deletions spec/std/process_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,12 @@ describe Process do
end
end

it "raises for long path" do
expect_raises(File::NotFoundError, "Error executing process: 'aaaaaaa") do
Process.new("a" * 1000)
end
end

it "accepts nilable string for `chdir` (#13767)" do
expect_raises(File::NotFoundError, "Error executing process: 'foobarbaz'") do
Process.new("foobarbaz", chdir: nil.as(String?))
Expand Down
8 changes: 5 additions & 3 deletions src/crystal/system/file.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,12 @@ module Crystal::System::File
when Tuple(FileDescriptor::Handle, Bool)
fd, blocking = result
return {fd, path, blocking}
when Errno::EEXIST, WinError::ERROR_FILE_EXISTS
# retry
else
raise ::File::Error.from_os_error("Error creating temporary file", result, file: path)
if ::File::AlreadyExistsError.os_error?(result)
# retry
else
raise ::File::Error.from_os_error("Error creating temporary file", result, file: path)
end
end
end

Expand Down
6 changes: 3 additions & 3 deletions src/crystal/system/unix/file.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ module Crystal::System::File
if ret == 0
::File::Info.new(stat)
else
if Errno.value.in?(Errno::ENOENT, Errno::ENOTDIR)
if ::File::NotFoundError.os_error?(Errno.value)
nil
else
raise ::File::Error.from_errno("Unable to get file info", file: path)
Expand Down Expand Up @@ -129,7 +129,7 @@ module Crystal::System::File
err = LibC.unlink(path.check_no_null_byte)
if err != -1
true
elsif !raise_on_missing && Errno.value == Errno::ENOENT
elsif !raise_on_missing && ::File::NotFoundError.os_error?(Errno.value)
false
else
raise ::File::Error.from_errno("Error deleting file", file: path)
Expand Down Expand Up @@ -158,7 +158,7 @@ module Crystal::System::File
buf = uninitialized UInt8[4096]
bytesize = LibC.readlink(path, buf, buf.size)
if bytesize == -1
if Errno.value.in?(Errno::EINVAL, Errno::ENOENT, Errno::ENOTDIR)
if ::File::NotFoundError.os_error?(Errno.value) || Errno.value == Errno::EINVAL
yield
end

Expand Down
3 changes: 1 addition & 2 deletions src/crystal/system/unix/process.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,8 +342,7 @@ struct Crystal::System::Process
end

private def self.raise_exception_from_errno(command, errno = Errno.value)
case errno
when Errno::EACCES, Errno::ENOENT, Errno::ENOEXEC
if ::File::NotFoundError.os_error?(errno) || ::File::AccessDeniedError.os_error?(errno) || errno == Errno::ENOEXEC
raise ::File::Error.from_os_error("Error executing process", errno, file: command)
else
raise IO::Error.from_os_error("Error executing process: '#{command}'", errno)
Expand Down
13 changes: 2 additions & 11 deletions src/crystal/system/win32/file.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,16 +97,9 @@ module Crystal::System::File
write_blocking(handle, slice, pos: @system_append ? UInt64::MAX : nil)
end

NOT_FOUND_ERRORS = {
WinError::ERROR_FILE_NOT_FOUND,
WinError::ERROR_PATH_NOT_FOUND,
WinError::ERROR_INVALID_NAME,
WinError::ERROR_DIRECTORY,
}

def self.check_not_found_error(message, path)
error = WinError.value
if NOT_FOUND_ERRORS.includes? error
if ::File::NotFoundError.os_error?(error)
nil
else
raise ::File::Error.from_os_error(message, error, file: path)
Expand Down Expand Up @@ -428,11 +421,9 @@ module Crystal::System::File
def self.readlink(path, &) : String
info = symlink_info?(path)
unless info
{% begin %}
if WinError.value.in?({{ NOT_FOUND_ERRORS.splat }}, WinError::ERROR_NOT_A_REPARSE_POINT)
if ::File::NotFoundError.os_error?(WinError.value) || WinError.value == WinError::ERROR_NOT_A_REPARSE_POINT
yield
end
{% end %}

raise ::File::Error.from_winerror("Cannot read link", file: path)
end
Expand Down
7 changes: 3 additions & 4 deletions src/crystal/system/win32/process.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,8 +296,8 @@ struct Crystal::System::Process
pointerof(startup_info), pointerof(process_info)
) == 0
error = WinError.value
case error.to_errno
when Errno::EACCES, Errno::ENOENT, Errno::ENOEXEC
case
when ::File::NotFoundError.os_error?(error) || ::File::AccessDeniedError.os_error?(error) || error == WinError::ERROR_BAD_EXE_FORMAT
raise ::File::Error.from_os_error("Error executing process", error, file: command_args)
else
raise IO::Error.from_os_error("Error executing process: '#{command_args}'", error)
Expand Down Expand Up @@ -379,8 +379,7 @@ struct Crystal::System::Process
end

private def self.raise_exception_from_errno(command, errno = Errno.value)
case errno
when Errno::EACCES, Errno::ENOENT
if ::File::NotFoundError.os_error?(errno) || ::File::AccessDeniedError.os_error?(errno)
raise ::File::Error.from_os_error("Error executing process", errno, file: command)
else
raise IO::Error.from_os_error("Error executing process: '#{command}'", errno)
Expand Down
52 changes: 47 additions & 5 deletions src/file/error.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@ class File::Error < IO::Error
end

private def self.new_from_os_error(message, os_error, **opts)
case os_error
when Errno::ENOENT, WinError::ERROR_FILE_NOT_FOUND, WinError::ERROR_PATH_NOT_FOUND
case
when File::NotFoundError.os_error?(os_error)
File::NotFoundError.new(message, **opts)
when Errno::EEXIST, WinError::ERROR_ALREADY_EXISTS
when File::AlreadyExistsError.os_error?(os_error)
File::AlreadyExistsError.new(message, **opts)
when Errno::EACCES, WinError::ERROR_ACCESS_DENIED, WinError::ERROR_PRIVILEGE_NOT_HELD
when File::AccessDeniedError.os_error?(os_error)
File::AccessDeniedError.new(message, **opts)
when Errno::ENOEXEC, WinError::ERROR_BAD_EXE_FORMAT
when File::BadExecutableError.os_error?(os_error)
File::BadExecutableError.new(message, **opts)
else
super message, os_error, **opts
Expand Down Expand Up @@ -48,13 +48,55 @@ class File::Error < IO::Error
end

class File::NotFoundError < File::Error
# :nodoc:
# See https://github.com/crystal-lang/crystal/issues/15905#issuecomment-2975820840
def self.os_error?(error)
error.in?(
Errno::ENAMETOOLONG,
Errno::ENOENT,
Errno::ENOTDIR,
WinError::ERROR_BAD_NETPATH,
WinError::ERROR_BAD_NET_NAME,
WinError::ERROR_BAD_PATHNAME,
WinError::ERROR_DIRECTORY,
WinError::ERROR_FILE_NOT_FOUND,
WinError::ERROR_FILENAME_EXCED_RANGE,
WinError::ERROR_INVALID_DRIVE,
WinError::ERROR_INVALID_NAME,
WinError::ERROR_PATH_NOT_FOUND,
WinError::WSAENAMETOOLONG,
)
end
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this. We shall keep the errno / winerror checks buried under Crystal::System.

  • it's leaking system specifics out of crystal/system;
  • it creates an indirection when we want to know which errno or winerror will lead to an exception;
  • it's pushing target-specifics constants to every target, so POSIX targets have to check errno against all the winerror that will never match (the opposite for windows).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the explicit exceptions. I just don't like the #os_error? methods.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Said differently: each system implementation decides what exception to raise, it shouldn't have to ask each exception "hey, should I raise you?"

Copy link
Member Author

@straight-shoota straight-shoota Aug 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither Errno nor WinError nor File::Error are in Crystal::System. So it feels like this doesn't need to belong there either.

We could consider moving these methods into Crystal::System. That breaks the direct connection to the error types. But I suppose it's not essential. I'm fine either way.

I suppose we could exclude checks against error codes that are impossible on the target system.
This would only mean putting the WinErrror codes behind {% if flag?(:win32) %} because some Windows API functions use errno, so on Windows we need all.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question is not "should I raise you?" but "I got this error, does it fit the error class you describe?". And then proceed with either raising that error (which File::Error.from_os_error handles) or reacting to that error condition in a specific way.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to eliminate the cross-type checks unless the argument is indeed a union between the system error code types, maybe one of these would work:

class File::Error
  def self.os_error?(error : Errno | WinError | WasiError)
    false
  end
end

class File::AlreadyExistsError < File::Error
  def self.os_error?(error : Errno)
    error == Errno::EEXIST
  end

  def self.os_error?(error : WinError)
    error.in?(
      WinError::ERROR_ALREADY_EXISTS,
      WinError::ERROR_FILE_EXISTS,
    )
  end
end
enum Errno
  def os_error_for?(ex : File::AlreadyExistsError.class)
    self == EEXIST
  end

  def os_error_for?(ex : File::Error.class)
    false
  end
end

enum WinError
  def os_error_for?(ex : File::AlreadyExistsError.class)
    self.in?(
      ERROR_ALREADY_EXISTS,
      ERROR_FILE_EXISTS,
    )
  end

  def os_error_for?(ex : File::Error.class)
    false
  end
end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm trying my best, but the more I look into the PR the less I understand how replacing a few errno checks after a syscall with an out-of-scope method call is supposed to be better. For example 🫤

Copy link
Member Author

@straight-shoota straight-shoota Aug 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main problem is that the current checks are incomplete. #15905 lists which error codes should usually be interpreted as "file not found". Currently, we're missing out on some of them in practically every instance. For example, the referenced example is missing ENOTDIR and ENAMETOOLONG.
On Windows we're generally even missing more than on Unix because Windows has more different codes.

The first step to fix that would be to add all the missing error codes in all the places where they're missing.
In system/win32 we're currently checking for error codes relating to File::NotFoundError in four different places. Each time we'd need to compare against ten different error codes, as identified in #15902.

That's a lot of duplication checking exactly the same set of values all over the place. And the individual error check conditions would be big and ugly to read. So IMO giving this common behaviour a name and extracting them into a shared helper is a good idea.

How exactly we implement that is up for debate.

end

class File::AlreadyExistsError < File::Error
# :nodoc:
def self.os_error?(error)
error.in?(
Errno::EEXIST,
WinError::ERROR_ALREADY_EXISTS,
WinError::ERROR_FILE_EXISTS,
)
end
end

class File::AccessDeniedError < File::Error
# :nodoc:
def self.os_error?(error)
error.in?(
Errno::EACCES,
WinError::ERROR_ACCESS_DENIED,
WinError::ERROR_PRIVILEGE_NOT_HELD,
)
end
end

class File::BadExecutableError < File::Error
# :nodoc:
def self.os_error?(error)
error.in?(
Errno::ENOEXEC,
WinError::ERROR_BAD_EXE_FORMAT,
)
end
end
Loading