Super-fancy refactor of Process::Status

[oprypin]

This uses a hierarchy of structs; it's a type-safe approach to the variety of POSIX exit statuses.

Based on #8381 (comment), #8381 (comment)

Usage examples

It is also backwards-compatible

I don't know how this will be received, but it doesn't matter, I had to put this out there 😂

[straight-shoota]

Sorry, but this is a no from me (per #8381 (comment)).

Establishing POSIXisms in the Process::Status API seems counterproductive to our efforts to create a platform-independent interface.
The features look nice, but that's too specific for stdlib. It would probably be a good fit for a posix/system shard, though 👍

[oprypin]

The POSIXisms are very well established as is, though.
This is quite the opposite, isolating them.

If you pretend that those other states don't exist, that's fine too.
But you can't also pretend that an external shard can fix that, because essential information will be thrown away irrecoverably before it can get to it. Unless you suggest re-implementing Process entirely for such a shard.

[straight-shoota]

I'm sure we can expose the platform-specific exit value and use that for such specific implementations.

[asterite]

Could we have a single Process::Status where info that's not available for one possible status is just simply nilable? I feel this is a bit overkill.

[oprypin]

I'm not particularly excited about a struct with 6 members to store 1 number. Which would also do nothing to prevent invalid states.

This is also the most straightforward way to implement this, even if you :nodoc: the underlying types. You can try to write out the code, there will be more of it than here.

Although my vision is that the types would be used more directly, with benefits of exhaustive case etc. Then the methods normal_exit?, signal_exit?, exit_signal can be deprecated.
People who care about these details will look at the subclasses, and people who don't will have exit_code+success? cover 99% of cases, and that will be the entirety of the top-level API.

[asterite]

with benefits of exhaustive case

Those benefits don't apply here because this is a type hierarchy and anyone could extend Process::Status and add their own type. We would need @[Sealed] for that. Or just change Process::Status to be a union of a fixed set of types.

[oprypin]

Those benefits don't apply here because this is a type hierarchy and anyone could extend Process::Status and add their own type.

Should this really be an argument? There are easier ways than that to break stdlib if one does such obviously unintended things.

---

Also maybe it's nice for when Linux adds more possible meanings to the range of exit statuses. 😂

WIFCONTINUED is available since Linux 2.6.10

[asterite]

Should this really be an argument?

I'm just saying that we can't prove exhaustiveness if the type is open for everyone to open. We can't just crash the program when we reach the else.

What are the problems with Ruby: https://ruby-doc.org/core-2.5.1/Process/Status.html ?

[oprypin]

What are the problems with Ruby: https://ruby-doc.org/core-2.5.1/Process/Status.html ?

Well, the API is cluttered and every method's return value is nilable.
Specifically what that means for Crystal is:
if s.signal_exit?; p s.exit_signal.not_nil!

It's not the worst solution, though, and we're just minor tweaks (though quite a few added methods) away from that. It just means that the logic can't move into Crystal::System::Process (which I was against anyway).

Just that people have been suggesting refactors where we don't store the exit_status.

And I found a very nice way to achieve that, while also potentially decreasing the number of methods that programmers are exposed to by default.

[asterite]

if s.signal_exit?; p s.exit_signal.not_nil!

Can we make each method return a nilable something that has all the properties if it's there, but nil if it doesn't apply?

Otherwise, it might be fine to return a union type... as long as they all quack similarly for the common cases (success?). I'm just not sure if $? handles unions well, or even abstract structs, maybe yes.

[oprypin]

Mm I guess it's about time for me to share the intended usage.

Basic:

if !status.success?
  puts "Oops, code #{status.exit_code}"  # can be a negative number for signals, which is not great
end

Advanced:

case status
when Process::Status::Exited
  puts "Exited with code #{status.exit_code}"
when Process::Status::Signaled
  puts "Terminated with signal #{status.signal}"
  puts "Core dumped" if status.core_dumped?
else # mandatory!
  puts "Something wrong, don't care!"
end

Alternate (per previous comment's suggestion, and already works, though ideally I'd remove it):

if (signaled = status.signal_exit?)
  puts "Terminated with signal #{signaled.signal}"
else
  puts "Uh probably exited normally? No idea"
end

Regarding the last part, this is what can happen frequently in the current state - people want to check all outcomes but don't realize that normal exit and signal exit are not the only possible states.
In the compiler, I'm guessing this code is broken for STOPPED and CONTINUED, it tries to obtain some signal value where there is none:

crystal/src/compiler/crystal/compiler.cr

Line 579 in f85bdb2

msg = status.normal_exit? ? "code: #{status.exit_code}" : "signal: #{status.exit_signal} (#{status.exit_signal.value})"

[oprypin]

Mm I just realized that Process never causes any state where STOPPED or CONTINUED would be possible. (does not set WUNTRACED)

Yeah now this is overkill.
My bad.