Turn SystemError.from_errno into a macro (#15874)

`SystemError.from_errno` calls `Errno.value` to retrieve the system error value from the last lib call.
But this happens in the body of the method which only executes after its arguments are evaluated (for example building the error message). This can lead to a change in `Errno.value` which `.from_errno` would then mistake for the original error.
In order to fix this, we must query `Errno.value` immediately after the lib call.

Changing `.from_errno` into a macro achieves that because it expands into a call to `Errno.value` before constructing the actual error object and its arguments.

We need to change the deprecated `from_errno` method which receives an `Errno` value as well because the macro would shadow the method.

One downside of the macro approach is that it won't work with file-private error types (#15496 (comment)). But this is probably not a very relevant use case.

Author: straight-shoota

spec/std/system_error_spec.cr

@@ -3,9 +3,41 @@ require "spec"
 describe SystemError do
   describe ".from_os_error" do
     it "Can build an error from an errno" do
-      errno = Errno.new(2)
-      error = ::IO::Error.from_os_error(message: nil, os_error: errno)
+      errno = Errno::ENOENT
+      error = ::RuntimeError.from_os_error(message: nil, os_error: errno)
       error.message.should eq("No such file or directory")
     end
   end
+
+  describe ".from_errno" do
+    it "captures `Errno.value`" do
+      Errno.value = :ENOENT
+      error = ::RuntimeError.from_errno "foobar"
+      error.os_error.should eq Errno::ENOENT
+    end
+
+    it "avoid reset from message" do
+      Errno.value = :ENOENT
+      error = ::RuntimeError.from_errno("foobar".tap { Errno.value = :EPERM })
+      error.os_error.should eq Errno::ENOENT
+    end
+  end
+
+  {% if flag?(:win32) %}
+    describe ".from_winerror" do
+      it "avoid reset from message" do
+        WinError.value = :ERROR_FILE_NOT_FOUND
+        error = ::RuntimeError.from_winerror("foobar".tap { WinError.value = :ERROR_ACCESS_DENIED })
+        error.os_error.should eq WinError::ERROR_FILE_NOT_FOUND
+      end
+    end
+
+    describe ".from_wsa_error" do
+      it "avoid reset from message" do
+        WinError.wsa_value = :ERROR_FILE_NOT_FOUND
+        error = ::RuntimeError.from_wsa_error("foobar".tap { WinError.wsa_value = :ERROR_ACCESS_DENIED })
+        error.os_error.should eq WinError::ERROR_FILE_NOT_FOUND
+      end
+    end
+  {% end %}
 end

src/system_error.cr

@@ -40,6 +40,53 @@
 module SystemError
   macro included
     extend ::SystemError::ClassMethods
+
+    # The following are macros so we can get the `errno` or `winerror` _before_
+    # evaluating `message` and `opts` that may change the `errno` or `winerror` value.
+
+    # Builds an instance of the exception from the current system error value (`Errno.value`).
+    #
+    # The system message corresponding to the OS error value amends the *message*.
+    # Additional keyword arguments are forwarded to the exception initializer `.new_from_os_error`.
+    macro from_errno(message, **opts)
+      %errno = ::Errno.value
+      ::\{{ @type }}.from_os_error(\{{ message }}, %errno, \{{ opts.double_splat }})
+    end
+
+    @[Deprecated("Use `.from_os_error` instead")]
+    macro from_errno(message = nil, errno = nil, **opts)
+      ::\{{ @type }}.from_os_error(\{{ message }}, \{{ errno }}, \{{ opts.double_splat }})
+    end
+
+    # Builds an instance of the exception from the current windows error value (`WinError.value`).
+    #
+    # The system message corresponding to the OS error value amends the *message*.
+    # Additional keyword arguments are forwarded to the exception initializer `.new_from_os_error`.
+    macro from_winerror(message, **opts)
+      %error = ::WinError.value
+      ::\{{ @type }}.from_os_error(\{{ message }}, %error, \{{ opts.double_splat }})
+    end
+
+    # Builds an instance of the exception from the current Windows Socket API error value (`WinError.wsa_value`).
+    #
+    # The system message corresponding to the OS error value amends the *message*.
+    # Additional keyword arguments are forwarded to the exception initializer.
+    macro from_wsa_error(message = nil, **opts)
+      %error = ::WinError.wsa_value
+      ::\{{ @type }}.from_os_error(\{{ message }}, %error, \{{ opts.double_splat }})
+    end
+
+    {% if flag?(:win32) %}
+      @[Deprecated("Use `.from_os_error` instead")]
+      macro from_winerror(message, winerror, **opts)
+        ::\{{ @type }}.from_os_error(\{{ message }}, \{{ winerror }}, \{{ opts.double_splat }})
+      end
+
+      @[Deprecated("Use `.from_os_error` instead")]
+      macro from_winerror(*, winerror = ::WinError.value, **opts)
+        ::\{{ @type }}.from_os_error(nil, \{{ winerror }}, \{{ opts.double_splat }})
+      end
+    {% end %}
   end
 
   # The original system error wrapped by this exception
@@ -68,19 +115,6 @@ module SystemError
       end
     end
 
-    # Builds an instance of the exception from the current system error value (`Errno.value`).
-    #
-    # The system message corresponding to the OS error value amends the *message*.
-    # Additional keyword arguments are forwarded to the exception initializer `.new_from_os_error`.
-    def from_errno(message : String, **opts)
-      from_os_error(message, Errno.value, **opts)
-    end
-
-    @[Deprecated("Use `.from_os_error` instead")]
-    def from_errno(message : String? = nil, errno : Errno? = nil, **opts)
-      from_os_error(message, errno, **opts)
-    end
-
     # Prepares the message that goes before the system error description.
     #
     # By default it returns the original message unchanged. But that could be
@@ -106,33 +140,5 @@ module SystemError
     protected def new_from_os_error(message : String?, os_error, **opts)
       self.new(message, **opts)
     end
-
-    # Builds an instance of the exception from the current windows error value (`WinError.value`).
-    #
-    # The system message corresponding to the OS error value amends the *message*.
-    # Additional keyword arguments are forwarded to the exception initializer `.new_from_os_error`.
-    def from_winerror(message : String?, **opts)
-      from_os_error(message, WinError.value, **opts)
-    end
-
-    # Builds an instance of the exception from the current Windows Socket API error value (`WinError.wsa_value`).
-    #
-    # The system message corresponding to the OS error value amends the *message*.
-    # Additional keyword arguments are forwarded to the exception initializer.
-    def from_wsa_error(message : String? = nil, **opts)
-      from_os_error(message, WinError.wsa_value, **opts)
-    end
-
-    {% if flag?(:win32) %}
-      @[Deprecated("Use `.from_os_error` instead")]
-      def from_winerror(message : String?, winerror : WinError, **opts)
-        from_os_error(message, winerror, **opts)
-      end
-
-      @[Deprecated("Use `.from_os_error` instead")]
-      def from_winerror(*, winerror : WinError = WinError.value, **opts)
-        from_os_error(message, winerror, **opts)
-      end
-    {% end %}
   end
 end