fix #17467 1st call to rand is now non-skewed; allow seed == 0 (#17468)

* fix #17467 1st call to rand is now non-skewed; allow passing 0 as seed

* changelog + fallback

* document behavior for seed == 0

* address comments

* _

* fix tests, disable kdtree

* re-enable kdtree with -d:nimLegacyRandomInitRand

changelog.md

@@ -63,6 +63,10 @@
 - Nim compiler now adds ASCII unit separator `\31` before a newline for every generated
   message (potentially multiline), so tooling can tell when messages start and end.
 
+- `random.initRand(seed)` now produces non-skewed values for the 1st call to `rand()` after
+  initialization with a small (< 30000) seed. Use `-d:nimLegacyRandomInitRand` to restore
+  previous behavior for a transition time, see PR #17467.
+
 
 ## Standard library additions and changes
 - Added support for parenthesized expressions in `strformat`
@@ -119,6 +123,13 @@
 - Added `randState` template that exposes the default random number generator.
   Useful for library authors.
 
+- Added `random.initRand()` overload with no argument which uses the current time as a seed.
+
+- `random.initRand(seed)` now allows `seed == 0`.
+
+- Added `std/sysrand` module to get random numbers from a secure source
+  provided by the operating system.
+
 - Added `std/enumutils` module. Added `genEnumCaseStmt` macro that generates case statement to parse string to enum.
   Added `items` for enums with holes.
   Added `symbolName` to return the enum symbol name ignoring the human readable name.
@@ -205,9 +216,6 @@
 
 - Deprecated `any`. See https://github.com/nim-lang/RFCs/issues/281
 
-- Added `std/sysrand` module to get random numbers from a secure source
-  provided by the operating system.
-
 - Added optional `options` argument to `copyFile`, `copyFileToDir`, and
   `copyFileWithPermissions`. By default, on non-Windows OSes, symlinks are
   followed (copy files symlinks point to); on Windows, `options` argument is
@@ -225,8 +233,6 @@
 - Added `os.isAdmin` to tell whether the caller's process is a member of the
   Administrators local group (on Windows) or a root (on POSIX).
 
-- Added `random.initRand()` overload with no argument which uses the current time as a seed.
-
 - Added experimental `linenoise.readLineStatus` to get line and status (e.g. ctrl-D or ctrl-C).
 
 - Added `compilesettings.SingleValueSetting.libPath`.

lib/pure/random.nim

@@ -133,11 +133,10 @@ proc next*(r: var Rand): uint64 =
   ##   that accepts a slice
   ## * `rand proc<#rand,typedesc[T]>`_ that accepts an integer or range type
   ## * `skipRandomNumbers proc<#skipRandomNumbers,Rand>`_
-  runnableExamples:
+  runnableExamples("-r:off"):
     var r = initRand(2019)
-    doAssert r.next() == 138_744_656_611_299'u64
+    assert r.next() == 13223559681708962501'u64 # implementation defined
-    doAssert r.next() == 979_810_537_855_049_344'u64
+    assert r.next() == 7229677234260823147'u64 # ditto
-    doAssert r.next() == 3_628_232_584_225_300_704'u64
 
   let s0 = r.a0
   var s1 = r.a1
@@ -214,11 +213,9 @@ proc rand*(r: var Rand; max: Natural): int {.benign.} =
   ## * `rand proc<#rand,Rand,HSlice[T: Ordinal or float or float32 or float64,T: Ordinal or float or float32 or float64]>`_
   ##   that accepts a slice
   ## * `rand proc<#rand,typedesc[T]>`_ that accepts an integer or range type
-  runnableExamples:
+  runnableExamples("-r:off"):
     var r = initRand(123)
-    doAssert r.rand(100) == 0
+    assert r.rand(100) == 96 # implementation defined
-    doAssert r.rand(100) == 96
-    doAssert r.rand(100) == 66
 
   if max == 0: return
   while true:
@@ -241,11 +238,9 @@ proc rand*(max: int): int {.benign.} =
   ## * `rand proc<#rand,HSlice[T: Ordinal or float or float32 or float64,T: Ordinal or float or float32 or float64]>`_
   ##   that accepts a slice
   ## * `rand proc<#rand,typedesc[T]>`_ that accepts an integer or range type
-  runnableExamples:
+  runnableExamples("-r:off"):
     randomize(123)
-    doAssert rand(100) == 0
+    assert [rand(100), rand(100)] == [96, 63] # implementation defined
-    doAssert rand(100) == 96
-    doAssert rand(100) == 66
 
   rand(state, max)
 
@@ -305,10 +300,8 @@ proc rand*[T: Ordinal or SomeFloat](r: var Rand; x: HSlice[T, T]): T =
   ## * `rand proc<#rand,typedesc[T]>`_ that accepts an integer or range type
   runnableExamples:
     var r = initRand(345)
-    doAssert r.rand(1..6) == 4
+    assert r.rand(1..5) <= 5
-    doAssert r.rand(1..6) == 4
+    assert r.rand(-1.1 .. 1.2) >= -1.1
-    doAssert r.rand(1..6) == 6
-    let f = r.rand(-1.0 .. 1.0) # 0.8741183448756229
   assert x.a <= x.b
   when T is SomeFloat:
     result = rand(r, x.b - x.a) + x.a
@@ -333,9 +326,7 @@ proc rand*[T: Ordinal or SomeFloat](x: HSlice[T, T]): T =
   ## * `rand proc<#rand,typedesc[T]>`_ that accepts an integer or range type
   runnableExamples:
     randomize(345)
-    doAssert rand(1..6) == 4
+    assert rand(1..6) <= 6
-    doAssert rand(1..6) == 4
-    doAssert rand(1..6) == 6
 
   result = rand(state, x)
 
@@ -354,16 +345,12 @@ proc rand*[T: SomeInteger](t: typedesc[T]): T =
   ##   that accepts a slice
   runnableExamples:
     randomize(567)
-    doAssert rand(int8) == 55
+    if false: # implementation defined
-    doAssert rand(int8) == -42
+      assert rand(int8) == -42
-    doAssert rand(int8) == 43
+      assert rand(uint32) == 578980729'u32
-    doAssert rand(uint32) == 578980729'u32
+      assert rand(range[1..16]) == 11
-    doAssert rand(uint32) == 4052940463'u32
+  # pending csources >= 1.4.0 or fixing https://github.com/timotheecour/Nim/issues/251#issuecomment-831599772,
-    doAssert rand(uint32) == 2163872389'u32
+  # use `runnableExamples("-r:off")` instead of `if false`
-    doAssert rand(range[1..16]) == 11
-    doAssert rand(range[1..16]) == 4
-    doAssert rand(range[1..16]) == 16
-
   when T is range:
     result = rand(state, low(T)..high(T))
   else:
@@ -380,9 +367,7 @@ proc sample*[T](r: var Rand; s: set[T]): T =
   runnableExamples:
     var r = initRand(987)
     let s = {1, 3, 5, 7, 9}
-    doAssert r.sample(s) == 5
+    assert r.sample(s) in s
-    doAssert r.sample(s) == 7
-    doAssert r.sample(s) == 1
 
   assert card(s) != 0
   var i = rand(r, card(s) - 1)
@@ -406,9 +391,7 @@ proc sample*[T](s: set[T]): T =
   runnableExamples:
     randomize(987)
     let s = {1, 3, 5, 7, 9}
-    doAssert sample(s) == 5
+    assert sample(s) in s
-    doAssert sample(s) == 7
-    doAssert sample(s) == 1
 
   sample(state, s)
 
@@ -423,9 +406,7 @@ proc sample*[T](r: var Rand; a: openArray[T]): T =
   runnableExamples:
     let marbles = ["red", "blue", "green", "yellow", "purple"]
     var r = initRand(456)
-    doAssert r.sample(marbles) == "blue"
+    assert r.sample(marbles) in marbles
-    doAssert r.sample(marbles) == "yellow"
-    doAssert r.sample(marbles) == "red"
 
   result = a[r.rand(a.low..a.high)]
 
@@ -445,9 +426,7 @@ proc sample*[T](a: openArray[T]): T =
   runnableExamples:
     let marbles = ["red", "blue", "green", "yellow", "purple"]
     randomize(456)
-    doAssert sample(marbles) == "blue"
+    assert sample(marbles) in marbles
-    doAssert sample(marbles) == "yellow"
-    doAssert sample(marbles) == "red"
 
   result = a[rand(a.low..a.high)]
 
@@ -476,9 +455,7 @@ proc sample*[T, U](r: var Rand; a: openArray[T]; cdf: openArray[U]): T =
     let count = [1, 6, 8, 3, 4]
     let cdf = count.cumsummed
     var r = initRand(789)
-    doAssert r.sample(marbles, cdf) == "red"
+    assert r.sample(marbles, cdf) in marbles
-    doAssert r.sample(marbles, cdf) == "green"
-    doAssert r.sample(marbles, cdf) == "blue"
 
   assert(cdf.len == a.len) # Two basic sanity checks.
   assert(float(cdf[^1]) > 0.0)
@@ -512,9 +489,7 @@ proc sample*[T, U](a: openArray[T]; cdf: openArray[U]): T =
     let count = [1, 6, 8, 3, 4]
     let cdf = count.cumsummed
     randomize(789)
-    doAssert sample(marbles, cdf) == "red"
+    assert sample(marbles, cdf) in marbles
-    doAssert sample(marbles, cdf) == "green"
-    doAssert sample(marbles, cdf) == "blue"
 
   state.sample(a, cdf)
 
@@ -547,10 +522,10 @@ proc gauss*(mu = 0.0, sigma = 1.0): float {.since: (1, 3).} =
 proc initRand*(seed: int64): Rand =
   ## Initializes a new `Rand <#Rand>`_ state using the given seed.
   ##
-  ## `seed` must not be zero. Providing a specific seed will produce
+  ## Providing a specific seed will produce the same results for that seed each time.
-  ## the same results for that seed each time.
   ##
-  ## The resulting state is independent of the default RNG's state.
+  ## The resulting state is independent of the default RNG's state. When `seed == 0`,
+  ## we internally set the seed to an implementation defined non-zero value.
   ##
   ## **See also:**
   ## * `initRand proc<#initRand>`_ that uses the current time
@@ -560,20 +535,22 @@ proc initRand*(seed: int64): Rand =
     from std/times import getTime, toUnix, nanosecond
 
     var r1 = initRand(123)
-
     let now = getTime()
     var r2 = initRand(now.toUnix * 1_000_000_000 + now.nanosecond)
-
+  const seedFallback0 = int32.high # arbitrary
-  doAssert seed != 0 # 0 causes `rand(int)` to always return 0 for example.
+  let seed = if seed != 0: seed else: seedFallback0 # because 0 is a fixed point
   result.a0 = Ui(seed shr 16)
   result.a1 = Ui(seed and 0xffff)
+  when not defined(nimLegacyRandomInitRand):
+    # calling `discard next(result)` (even a few times) would still produce
+    # skewed numbers for the 1st call to `rand()`.
+    skipRandomNumbers(result)
   discard next(result)
 
 proc randomize*(seed: int64) {.benign.} =
   ## Initializes the default random number generator with the given seed.
   ##
-  ## `seed` must not be zero. Providing a specific seed will produce
+  ## Providing a specific seed will produce the same results for that seed each time.
-  ## the same results for that seed each time.
   ##
   ## **See also:**
   ## * `initRand proc<#initRand,int64>`_ that initializes a Rand state
@@ -600,7 +577,8 @@ proc shuffle*[T](r: var Rand; x: var openArray[T]) =
     var cards = ["Ace", "King", "Queen", "Jack", "Ten"]
     var r = initRand(678)
     r.shuffle(cards)
-    doAssert cards == ["King", "Ace", "Queen", "Ten", "Jack"]
+    import std/algorithm
+    assert cards.sorted == @["Ace", "Jack", "King", "Queen", "Ten"]
 
   for i in countdown(x.high, 1):
     let j = r.rand(i)
@@ -620,7 +598,8 @@ proc shuffle*[T](x: var openArray[T]) =
     var cards = ["Ace", "King", "Queen", "Jack", "Ten"]
     randomize(678)
     shuffle(cards)
-    doAssert cards == ["King", "Ace", "Queen", "Ten", "Jack"]
+    import std/algorithm
+    assert cards.sorted == @["Ace", "Jack", "King", "Queen", "Ten"]
 
   shuffle(state, x)
 

testament/important_packages.nim

@@ -79,7 +79,7 @@ pkg "itertools", "nim doc src/itertools.nim"
 pkg "iterutils"
 pkg "jstin"
 pkg "karax", "nim c -r tests/tester.nim"
-pkg "kdtree", "nimble test", "https://github.com/jblindsay/kdtree"
+pkg "kdtree", "nimble test -d:nimLegacyRandomInitRand", "https://github.com/jblindsay/kdtree"
 pkg "loopfusion"
 pkg "macroutils"
 pkg "manu"

tests/stdlib/trandom.nim

@@ -1,5 +1,5 @@
 discard """
-  joinable: false
+  joinable: false # to avoid messing with global rand state
   targets: "c js"
 """
 
@@ -37,11 +37,14 @@ main()
 
 block:
   when not defined(js):
-    doAssert almostEqual(rand(12.5), 4.012897747078944)
+    doAssert almostEqual(rand(12.5), 7.355175342026979)
-    doAssert almostEqual(rand(2233.3322), 879.702755321298)
+    doAssert almostEqual(rand(2233.3322), 499.342386778917)
 
   type DiceRoll = range[0..6]
-  doAssert rand(DiceRoll).int == 4
+  when not defined(js):
+    doAssert rand(DiceRoll).int == 3
+  else:
+    doAssert rand(DiceRoll).int == 6
 
 var rs: RunningStat
 for j in 1..5:
@@ -164,3 +167,23 @@ block: # random sample
       let stdDev = sqrt(n * p * (1.0 - p))
       # NOTE: like unnormalized int CDF test, P(wholeTestFails) =~ 0.01.
       doAssert abs(float(histo[values[i]]) - expected) <= 3.0 * stdDev
+
+block:
+  # 0 is a valid seed
+  var r = initRand(0)
+  doAssert r.rand(1.0) != r.rand(1.0)
+  r = initRand(10)
+  doAssert r.rand(1.0) != r.rand(1.0)
+  # changing the seed changes the sequence
+  var r1 = initRand(123)
+  var r2 = initRand(124)
+  doAssert r1.rand(1.0) != r2.rand(1.0)
+
+block: # bug #17467
+  let n = 1000
+  for i in -n .. n:
+    var r = initRand(i)
+    let x = r.rand(1.0)
+    doAssert x > 1e-4, $(x, i)
+      # This used to fail for each i in 0..<26844, i.e. the 1st produced value
+      # was predictable and < 1e-4, skewing distributions.