Replies: 1 comment
-
Thank you for writing this up. I think aiming to support However, for the initial Rust port, idiomatic Rust is an explicit non-goal:
When we find a bug, we can do a nearly line-by-line comparison. Trying to port and simultaneously refactor is riskier than a pure port. In C++ we use |
Beta Was this translation helpful? Give feedback.
-
After the discussion in #9544 (comment), I decided to write down my thoughts on the idiomatic design of builtins' return values in a more coherent manner. I hope that by detaching this from any particular PR, it'll be easier to discuss things without constantly blocking other more productive work (it can also just be left sitting around until it's "make it pretty" time).
I think there are two related but disctinct problems at play here:
Return value of builtins' entry functions
Most builtins return exactly one status code, which is used to update
$status
. However, some builtins do not change$status
at all, thus the current model ofOption<i32>
.There is one immediate problem with this: the semantics of
Option
'sTry
implementation (the trait backing the?
operator) do not match this usage. ForOption
,None
is treated as the error/"residual" value, meaning that if a builtin calls a function returning anOption
and naively uses?
on the result, the builtin stops executing and returns "do not change$status
", which is nonsensical.The fix would be to either wrap the
Option
in a newtype, or simply create a new enum like this:This type also immediately explains the meaning of builtins' return values to readers.
Internal functions inside builtins
The story is slightly different for (helper) functions inside builtins. Here, not all status values are created equal:
STATUS_CMD_OK
(value0
) is special, there are currently 63 occurences of even just the exact stringretval != STATUS_CMD_OK
insrc/builtins/
. The Rust type that most closely matches "success, or one of a set of error values" isResult
.A naive approach would be
Result<_, i32>
, whereOk(_)
representsSTATUS_CMD_OK
andErr(status)
represents an error. This allows?
to be used for propagating errors (equivalent to the frequentif (retval != STATUS_CMD_OK) return retval;
in C++). It can also trivially be converted to an integer status value -Ok(_)
is0
,Err(n)
isn
.There is a problem with this though: because the
Err
value can be any status value, and0
is a valid status value, it is possible to returnErr(0)
, which is ambiguous. According to?
, it's an error, according to the numeric value, it's success. In order to "make illegal state unrepresentable", the type of theErr
value needs to change. Luckily, the standard library already provides a handy option here:NonZeroI32
, which can be wrapped in a newtype like so:It can then be used like so:
Because
NonZeroI32
has some special attributes, the layout ofResult<(), BuiltinError>
is actually guaranteed to be the same as a plaini32
:0
encodes theOk(())
variant, the other values encode the errors, making it an equivalent but more meaningful replacement fori32
status values in general. As such, it can also be used for top-level builtin return values, with the following revisedStatusAction
type:Beta Was this translation helpful? Give feedback.
All reactions