From 8f12767bc07be8b9742f3561ae51b30cbb362132 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Johannes=20M=C3=BCller?= <straightshoota@gmail.com>
Date: Sat, 14 Dec 2024 14:04:37 +0100
Subject: [PATCH] Fix stringification of abnormal `Process::Status` on Windows
 [fixup #15255] (#15267)

The change in #15255 broke `Process::Status#to_s` and `#inspect` on Windows due to the redefinition of `#normal_exit?`. Unfortunately we were missing specs for this, so it did go unnoticed.

This patch adds specs and restablishes the previous behaviour with the small improvement of treating the `exit_status` value as `UInt32` instead of `Int32`, which results in positive numbers.

```cr
# Crystal 1.14.0
Process::Status.new(LibC::STATUS_CONTROL_C_EXIT)).inspect # => "Process::Status[-1073741510]"

# master
Process::Status.new(LibC::STATUS_CONTROL_C_EXIT)).inspect # NotImplementedError: Process::Status#exit_signal

# this patch
Process::Status.new(LibC::STATUS_CONTROL_C_EXIT)).inspect # => "Process::Status[3221225786]"
```

There's still room for improvement, for example map the values to names and/or use base 16 numerals as is custom for error statuses on Windows), but I'll leave that for a follow-up.
---
 spec/std/process/status_spec.cr | 16 +++++++++++++
 src/process/status.cr           | 42 +++++++++++++++++++++------------
 2 files changed, 43 insertions(+), 15 deletions(-)

diff --git a/spec/std/process/status_spec.cr b/spec/std/process/status_spec.cr
index ea360a9a9dae..86529b2cefd4 100644
--- a/spec/std/process/status_spec.cr
+++ b/spec/std/process/status_spec.cr
@@ -236,6 +236,14 @@ describe Process::Status do
       assert_prints Process::Status.new(exit_status(255)).to_s, "255"
     end
 
+    it "on abnormal exit" do
+      {% if flag?(:win32) %}
+        assert_prints status_for(:interrupted).to_s, "3221225786"
+      {% else %}
+        assert_prints status_for(:interrupted).to_s, "INT"
+      {% end %}
+    end
+
     {% if flag?(:unix) && !flag?(:wasi) %}
       it "with exit signal" do
         assert_prints Process::Status.new(Signal::HUP.value).to_s, "HUP"
@@ -254,6 +262,14 @@ describe Process::Status do
       assert_prints Process::Status.new(exit_status(255)).inspect, "Process::Status[255]"
     end
 
+    it "on abnormal exit" do
+      {% if flag?(:win32) %}
+        assert_prints status_for(:interrupted).inspect, "Process::Status[3221225786]"
+      {% else %}
+        assert_prints status_for(:interrupted).inspect, "Process::Status[Signal::INT]"
+      {% end %}
+    end
+
     {% if flag?(:unix) && !flag?(:wasi) %}
       it "with exit signal" do
         assert_prints Process::Status.new(Signal::HUP.value).inspect, "Process::Status[Signal::HUP]"
diff --git a/src/process/status.cr b/src/process/status.cr
index 15913ce2fd5e..5fd70e5ad203 100644
--- a/src/process/status.cr
+++ b/src/process/status.cr
@@ -275,11 +275,15 @@ class Process::Status
   # `Process::Status[Signal::HUP]`.
   def inspect(io : IO) : Nil
     io << "Process::Status["
-    if normal_exit?
-      exit_code.inspect(io)
-    else
-      exit_signal.inspect(io)
-    end
+    {% if flag?(:win32) %}
+      @exit_status.to_s(io)
+    {% else %}
+      if normal_exit?
+        exit_code.inspect(io)
+      else
+        exit_signal.inspect(io)
+      end
+    {% end %}
     io << "]"
   end
 
@@ -288,11 +292,15 @@ class Process::Status
   # A normal exit status prints the numerical value (`0`, `1` etc).
   # A signal exit status prints the name of the `Signal` member (`HUP`, `INT`, etc.).
   def to_s(io : IO) : Nil
-    if normal_exit?
-      io << exit_code
-    else
-      io << exit_signal
-    end
+    {% if flag?(:win32) %}
+      @exit_status.to_s(io)
+    {% else %}
+      if normal_exit?
+        io << exit_code
+      else
+        io << exit_signal
+      end
+    {% end %}
   end
 
   # Returns a textual representation of the process status.
@@ -300,10 +308,14 @@ class Process::Status
   # A normal exit status prints the numerical value (`0`, `1` etc).
   # A signal exit status prints the name of the `Signal` member (`HUP`, `INT`, etc.).
   def to_s : String
-    if normal_exit?
-      exit_code.to_s
-    else
-      exit_signal.to_s
-    end
+    {% if flag?(:win32) %}
+      @exit_status.to_s
+    {% else %}
+      if normal_exit?
+        exit_code.to_s
+      else
+        exit_signal.to_s
+      end
+    {% end %}
   end
 end