implement an improved random command

Fixes #2642

doc_src/random.txt

@@ -2,31 +2,40 @@
 
 \subsection random-synopsis Synopsis
 \fish{synopsis}
-random [SEED]
+random
+random SEED
+random START END
+random START STEP END
+random choice [ITEMS...]
 \endfish
 
 \subsection random-description Description
 
-`random` outputs a psuedo-random number from 0 to 32767, inclusive.
+`RANDOM` generates a pseudo-random integer from a uniform distribution. The
-Even ignoring the very narrow range of values you should not assume
+range (inclusive) is dependent on the arguments passed.
-this produces truly random values within that range. Do not use the
+No arguments indicate a range of [0; 32767].
-value for any cryptographic purposes, and take care to handle collisions:
+If one argument is specified, the internal engine will be seeded with the
-the same random number appearing more than once in a given fish instance.
+argument for future invocations of `RANDOM` and no output will be produced.
+Two arguments indicate a range of [START; END].
+Three arguments indicate a range of [START; END] with a spacing of STEP
+between possible outputs.
+`RANDOM choice` will select one random item from the succeeding arguments.
 
-If a `SEED` value is provided, it is used to seed the random number
+Note that seeding the engine will NOT give the same result across different
-generator, and no output will be produced. This can be useful for debugging
+systems.
-purposes, where it can be desirable to get the same random number sequence
-multiple times. If the random number generator is called without first
-seeding it, the current time will be used as the seed.
 
+You should not consider `RANDOM` cryptographically secure, or even
+statistically accurate.
 
 \subsection random-example Example
 
-The following code will count down from a random number to 1:
+The following code will count down from a random even number between 10 and 20 to 1:
-
 \fish
-for i in (seq (random) -1 1)
+for i in (seq (random 10 2 20) -1 1)
     echo $i
-    sleep
 end
 \endfish
+And this will open a random picture from any of the subdirectories:
+\fish 
+open (random choice **jpg)
+\endfish

src/builtin.cpp

@@ -26,12 +26,13 @@
 #include <stdlib.h>
 #include <string.h>
 #include <sys/stat.h>
-#include <time.h>
+#include <sys/types.h>
 #include <unistd.h>
 #include <wchar.h>
 #include <algorithm>
 #include <map>
 #include <memory>
+#include <random>
 #include <string>
 #include <utility>
 
@@ -1742,15 +1743,21 @@ int builtin_function(parser_t &parser, io_streams_t &streams, const wcstring_lis
 
 /// The random builtin generates random numbers.
 static int builtin_random(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
-    static int seeded = 0;
+    static bool seeded = false;
-    static struct drand48_data seed_buffer;
+    static std::minstd_rand engine;
-
+    if (!seeded) {
-    int argc = builtin_count_args(argv);
+        // seed engine with 2*32 bits of random data
+        // for the 64 bits of internal state of minstd_rand
+        std::random_device rd;
+        std::seed_seq seed{rd(), rd()};
+        engine.seed(seed);
+        seeded = true;
+    }
 
     wgetopter_t w;
-
+    int argc = builtin_count_args(argv);
-    static const struct woption long_options[] = {{L"help", no_argument, 0, 'h'}, {0, 0, 0, 0}};
+    static const struct woption long_options[] = {{L"help", no_argument, NULL, 'h'},
-
+                                                  {NULL, 0, NULL, 0}};
     while (1) {
         int opt_index = 0;
 
@@ -1767,7 +1774,7 @@ static int builtin_random(parser_t &parser, io_streams_t &streams, wchar_t **arg
             }
             case 'h': {
                 builtin_print_help(parser, streams, argv[0], streams.out);
-                break;
+                return STATUS_BUILTIN_OK;
             }
             case '?': {
                 builtin_unknown_option(parser, streams, argv[0], argv[w.woptind - 1]);
@@ -1781,29 +1788,115 @@ static int builtin_random(parser_t &parser, io_streams_t &streams, wchar_t **arg
     }
 
     int arg_count = argc - w.woptind;
-    if (arg_count == 0) {
+    long long start, end;
-        long res;
+    unsigned long long step;
-        if (!seeded) {
+    bool choice = false;
-            seeded = 1;
+    if (arg_count >= 1 && !wcscmp(argv[w.woptind], L"choice")) {
-            srand48_r(time(0), &seed_buffer);
+        if (arg_count == 1) {
-        }
+            streams.err.append_format(L"%ls: nothing to choose from\n", argv[0]);
-        lrand48_r(&seed_buffer, &res);
-        streams.out.append_format(L"%ld\n", res % 32768);
-    } else if (arg_count == 1) {
-        long foo = fish_wcstol(argv[w.woptind]);
-        if (errno) {
-            streams.err.append_format(_(L"%ls: Seed value '%ls' is not a valid number\n"), argv[0],
-                                      argv[w.woptind]);
             return STATUS_BUILTIN_ERROR;
         }
-        seeded = 1;
+        choice = true;
-        srand48_r(foo, &seed_buffer);
+        start = 1;
+        step = 1;
+        end = arg_count - 1;
     } else {
-        streams.err.append_format(_(L"%ls: Expected zero or one argument, got %d\n"), argv[0],
+        bool parse_error = false;
-                                  argc - w.woptind);
+        auto parse_ll = [&](const wchar_t *str) {
-        builtin_print_help(parser, streams, argv[0], streams.err);
+            long long ll = fish_wcstoll(str);
+            if (errno) {
+                streams.err.append_format(L"%ls: %ls is not a valid integer\n", argv[0], str);
+                parse_error = true;
+            }
+            return ll;
+        };
+        auto parse_ull = [&](const wchar_t *str) {
+            unsigned long long ull = fish_wcstoull(str);
+            if (errno) {
+                streams.err.append_format(L"%ls: %ls is not a valid integer\n", argv[0], str);
+                parse_error = true;
+            }
+            return ull;
+        };
+        if (arg_count == 0) {
+            start = 0;
+            end = 32767;
+            step = 1;
+        } else if (arg_count == 1) {
+            long long seed = parse_ll(argv[w.woptind]);
+            if (parse_error) return STATUS_BUILTIN_ERROR;
+            engine.seed(static_cast<uint32_t>(seed));
+            return STATUS_BUILTIN_OK;
+        } else if (arg_count == 2) {
+            start = parse_ll(argv[w.woptind]);
+            step = 1;
+            end = parse_ll(argv[w.woptind + 1]);
+        } else if (arg_count == 3) {
+            start = parse_ll(argv[w.woptind]);
+            step = parse_ull(argv[w.woptind + 1]);
+            end = parse_ll(argv[w.woptind + 2]);
+        } else {
+            streams.err.append_format(BUILTIN_ERR_TOO_MANY_ARGUMENTS, argv[0]);
             return STATUS_BUILTIN_ERROR;
         }
+
+        if (parse_error) {
+            return STATUS_BUILTIN_ERROR;
+        } else if (start >= end) {
+            streams.err.append_format(L"%ls: END must be greater than START\n", argv[0]);
+            return STATUS_BUILTIN_ERROR;
+        } else if (step == 0) {
+            streams.err.append_format(L"%ls: STEP must be a positive integer\n", argv[0]);
+            return STATUS_BUILTIN_ERROR;
+        }
+    }
+
+    // only for negative argument
+    auto safe_abs = [](long long ll) -> unsigned long long {
+        return -static_cast<unsigned long long>(ll);
+    };
+    long long real_end;
+    if (start >= 0 || end < 0) {
+        // 0 <= start <= end
+        long long diff = end - start;
+        // 0 <= diff <= LL_MAX
+        real_end = start + static_cast<long long>(diff / step);
+    } else {
+        // start < 0 <= end
+        unsigned long long abs_start = safe_abs(start);
+        unsigned long long diff = (end + abs_start);
+        real_end = diff / step - abs_start;
+    }
+
+    if (!choice && start == real_end) {
+        streams.err.append_format(L"%ls: range contains only one possible value\n", argv[0]);
+        return STATUS_BUILTIN_ERROR;
+    }
+
+    std::uniform_int_distribution<long long> dist(start, real_end);
+    long long random = dist(engine);
+    long long result;
+    if (start >= 0) {
+        // 0 <= start <= random <= end
+        long long diff = random - start;
+        // 0 < step * diff <= end - start <= LL_MAX
+        result = start + static_cast<long long>(diff * step);
+    } else if (random < 0) {
+        // start <= random < 0
+        long long diff = random - start;
+        result = diff * step - safe_abs(start);
+    } else {
+        // start < 0 <= random
+        unsigned long long abs_start = safe_abs(start);
+        unsigned long long diff = (random + abs_start);
+        result = diff * step - abs_start;
+    }
+
+    if (choice) {
+        streams.out.append_format(L"%ls\n", argv[w.woptind + result]);
+    } else {
+        streams.out.append_format(L"%lld\n", result);
+    }
     return STATUS_BUILTIN_OK;
 }
 

src/wutil.cpp

@@ -631,11 +631,13 @@ long long fish_wcstoll(const wchar_t *str, const wchar_t **endptr, int base) {
 /// annoying to use them in a portable fashion.
 ///
 /// The caller doesn't have to zero errno. Sets errno to -1 if the int ends with something other
-/// than a digit. Leading whitespace is ignored (per the base wcstoll implementation). Trailing
+/// than a digit. Leading minus is considered invalid. Leading whitespace is ignored (per the base
-/// whitespace is also ignored.
+/// wcstoull implementation). Trailing whitespace is also ignored.
 unsigned long long fish_wcstoull(const wchar_t *str, const wchar_t **endptr, int base) {
     while (iswspace(*str)) ++str;  // skip leading whitespace
-    if (!*str) {  // this is because some implementations don't handle this sensibly
+    if (!*str ||      // this is because some implementations don't handle this sensibly
+        *str == '-')  // disallow minus as the first character to avoid questionable wrap-around
+    {
         errno = EINVAL;
         if (endptr) *endptr = str;
         return 0;

tests/random.err

@@ -0,0 +1,16 @@
+random: a is not a valid integer
+random: 18446744073709551614 is not a valid integer
+random: Too many arguments
+random: END must be greater than START
+random: 18446744073709551614 is not a valid integer
+random: 1d is not a valid integer
+random: 1c is not a valid integer
+random: END must be greater than START
+random: - is not a valid integer
+random: -1 is not a valid integer
+random: -9223372036854775807 is not a valid integer
+random: STEP must be a positive integer
+random: range contains only one possible value
+random: range contains only one possible value
+random: nothing to choose from
+random: Too many arguments

tests/random.in

@@ -0,0 +1,98 @@
+set -l max 9223372036854775807
+set -l close_max 9223372036854775806
+set -l min -9223372036854775807
+set -l close_min -9223372036854775806
+set -l diff_max 18446744073709551614
+
+# check failure cases
+random a
+random $diff_max
+random -- 1 2 3 4
+random -- 10 -10
+random -- 10 $diff_max
+random -- 1 1d
+random -- 1 1c 10
+random -- 10 10
+random -- 1 - 10
+random -- 1 -1 10
+random -- 1 $min 10
+random -- 1 0 10
+random -- 1 11 10
+random -- 0 $diff_max $max
+random choice
+random choic a b c
+
+function check_boundaries
+    if not test $argv[1] -ge $argv[2] -a $argv[1] -le $argv[3] 
+        printf "Unexpected: %s <= %s <= %s not verified\n" $argv[2] $argv[1] $argv[3] >&2
+        return 1
+    end
+end
+
+function test_range
+    return (check_boundaries (random -- $argv) $argv)
+end
+
+function check_contains
+    if not contains -- $argv[1] $argv[2..-1]
+        printf "Unexpected: %s not among possibilities" $argv[1] >&2
+        printf " %s" $argv[2..-1] >&2
+        printf "\n" >&2
+        return 1
+    end
+end
+
+function test_step
+    return (check_contains (random -- $argv) (seq -- $argv))
+end
+
+function test_choice
+    return (check_contains (random choice $argv) $argv)
+end
+
+for i in (seq 10)
+    check_boundaries (random) 0 32767
+
+    test_range 0 10
+    test_range -10 -1
+    test_range -10 10
+
+    test_range 0 $max
+    test_range $min -1
+    test_range $min $max
+
+    test_range $close_max $max
+    test_range $min $close_min
+    test_range $close_min $close_max
+
+    #OSX's `seq` uses scientific notation for large numbers, hence not usable here
+    check_contains (random -- 0 $max $max) 0 $max
+    check_contains (random -- 0 $close_max $max) 0 $close_max
+    check_contains (random -- $min $max 0) $min 0
+    check_contains (random -- $min $close_max 0) $min -1
+    check_contains (random -- $min $max $max) $min 0 $max
+    check_contains (random -- $min $diff_max $max) $min $max
+
+    test_step 0 $i 10
+    test_step -5 $i 5
+    test_step -10 $i 0
+    
+    test_choice a
+    test_choice foo bar
+    test_choice bass trout salmon zander perch carp
+end
+
+
+#check seeding
+set -l seed (random)
+random $seed
+set -l run1 (random) (random) (random) (random) (random)
+random $seed
+set -l run2 (random) (random) (random) (random) (random)
+if not test "$run1" = "$run2"
+    printf "Unexpected different sequences after seeding with %s\n" $seed
+    printf "%s " $run1
+    printf "\n"
+    printf "%s " $run2
+    printf "\n"
+end