Saturate exit codes to 255 for all builtins

After commit 6dd6a57c60, 3 remaining
builtins were affected by uint8_t overflow: `exit`, `return`, and
`functions --query`.

This commit:
- Moves the overflow check from `builtin_set_query` to `builtin_run`.
- Removes a conflicting int -> uint8_t conversion in `builtin_return`.
- Adds tests for the 3 remaining affected builtins.
- Simplifies the wording for the documentation for `set --query`.
- Does not change documentation for `functions --query`, because it does
  not state the exit code in its API.
- Updates the CHANGELOG to reflect the change to all builtins.

CHANGELOG.rst

@@ -183,7 +183,7 @@ Scripting improvements
 -  ``fish -c`` now reads the remaining arguments into $argv (:issue:`2314`).
 -  The ``pwd`` command supports the long options ``--logical`` and ``--physical``, matching other implementations (:issue:`6787`).
 -  ``fish --profile`` now only starts the profile after fish's startup (including config.fish) is done. For profiling startup there is a new ``--profile-startup`` option that profiles only startup (:issue:`7648`).
-- ``set --query``'s $status will now saturate at 255 instead of wrapping around when checking more than 255 variables at once (:issue:`7698`).
+-  Builtins return a maximum exit status of 255, rather than potentially overflowing. In particular, this affects ``exit``, ``return``, ``functions --query``, and ``set --query`` (:issue:`7698`, :issue:`7702`).
 - It is no longer an error to run builtin with closed stdin. For example ``count <&-`` now prints 0, instead of failing.
 
 

doc_src/cmds/set.rst

@@ -49,7 +49,7 @@ The following options are available:
 
 - ``-e`` or ``--erase`` causes the specified shell variables to be erased
 
-- ``-q`` or ``--query`` test if the specified variable names are defined. Does not output anything, but the builtins exit status is the number of variables specified that were not defined, saturating at 255 if more than 255 variables are not defined.
+- ``-q`` or ``--query`` test if the specified variable names are defined. Does not output anything, but the builtins exit status is the number of variables specified that were not defined, or 255 if more than 255 variables are not defined.
 
 - ``-n`` or ``--names`` List only the names of all defined variables, not their value. The names are guaranteed to be sorted.
 

src/builtin.cpp

@@ -473,7 +473,15 @@ proc_status_t builtin_run(parser_t &parser, wchar_t **argv, io_streams_t &stream
         if (!ret) {
             return proc_status_t::empty();
         }
-        return proc_status_t::from_exit_code(ret.value());
+
+        // The exit code is cast to an 8-bit unsigned integer, so saturate to 255. Otherwise,
+        // multiples of 256 are reported as 0.
+        int code = ret.value();
+        if (code > 255) {
+            code = 255;
+        }
+
+        return proc_status_t::from_exit_code(code);
     }
 
     FLOGF(error, UNKNOWN_BUILTIN_ERR_MSG, argv[0]);

src/builtin_return.cpp

@@ -86,7 +86,6 @@ maybe_t<int> builtin_return(parser_t &parser, io_streams_t &streams, wchar_t **a
             builtin_print_error_trailer(parser, streams.err, cmd);
             return STATUS_INVALID_ARGS;
         }
-        retval &= 0xFF;
     }
 
     // Find the function block.

src/builtin_set.cpp

@@ -565,13 +565,6 @@ static int builtin_set_query(const wchar_t *cmd, set_cmd_opts_t &opts, int argc,
         free(dest);
     }
 
-    // The return value is cast to an 8-bit unsigned integer,
-    // so saturate the error count to 255.
-    // Otherwise 256 errors is reported as 0 errors.
-    if (retval > 255) {
-        retval = 255;
-    }
-
     return retval;
 }
 

src/proc.h

@@ -81,6 +81,8 @@ class proc_status_t {
         // Some paranoia.
         constexpr int zerocode = w_exitcode(0, 0);
         static_assert(WIFEXITED(zerocode), "Synthetic exit status not reported as exited");
+
+        assert(ret < 256);
         return proc_status_t(w_exitcode(ret, 0 /* sig */));
     }
 

tests/checks/basic.fish

@@ -110,6 +110,18 @@ end
 echo Test 5 $sta
 #CHECK: Test 5 pass
 
+
+function test_builtin_status_clamp_to_255
+    return 300
+end
+test_builtin_status_clamp_to_255
+echo $status
+#CHECK: 255
+
+$fish -c "exit 300"
+echo $status
+#CHECK: 255
+
 ####################
 # echo tests
 echo 'abc\ndef'

tests/checks/functions.fish

@@ -92,6 +92,12 @@ functions --erase ls
 type -t ls
 #CHECK: file
 
+# ==========
+# Verify that `functions --query` does not return 0 if there are 256 missing functions
+functions --query a(seq 1 256)
+echo $status
+#CHECK: 255
+
 echo "function t; echo tttt; end" | source
 functions t
 # CHECK: # Defined via `source`