Merge pull request #2944 from jgonggrijp/fix-sample-string

Properly convert _.sample input collection to array

Author: jgonggrijp

modules/sample.js

@@ -1,8 +1,8 @@
 import isArrayLike from './_isArrayLike.js';
-import clone from './clone.js';
 import values from './values.js';
 import getLength from './_getLength.js';
 import random from './random.js';
+import toArray from './toArray.js';
 
 // Sample **n** random values from a collection using the modern version of the
 // [Fisher-Yates shuffle](https://en.wikipedia.org/wiki/Fisher–Yates_shuffle).
@@ -13,7 +13,7 @@ export default function sample(obj, n, guard) {
     if (!isArrayLike(obj)) obj = values(obj);
     return obj[random(obj.length - 1)];
   }
-  var sample = isArrayLike(obj) ? clone(obj) : values(obj);
+  var sample = toArray(obj);
   var length = getLength(sample);
   n = Math.max(Math.min(n, length), 0);
   var last = length - 1;

test/collections.js

@@ -843,6 +843,28 @@
     var partialSample = _.sample(_.range(1000), 10);
     var partialSampleSorted = partialSample.sort();
     assert.notDeepEqual(partialSampleSorted, _.range(10), 'samples from the whole array, not just the beginning');
+    // The next few lines (up to END) are a regression test for #2927.
+    var alphabet = 'abcdefghijklmnopqrstuvwxyz';
+    var prefixLength = 5;
+    var prefix = _.toArray(alphabet.slice(0, prefixLength));
+    // We're going to take three random samples from the alphabet and count how
+    // many of them are exact prefixes of the alphabet ('abcde').
+    var verbatimPrefixes = 0;
+    _.times(3, function() {
+      var sample = _.toArray(_.sample(alphabet, prefixLength));
+      if (_.isEqual(sample, prefix)) ++verbatimPrefixes;
+    });
+    // The probability of a sample of length N being a prefix is 1/(A!/(A-N)!),
+    // with A being the length of the alphabet. That amounts to roughly 1 in
+    // 7.9e6 when N=5 and A=26. Most of the time, therefore, we should find that
+    // verbatimPrefixes=0. We will however accept the occasional hit. Only when
+    // it happens twice, does it start to look really suspicious; the
+    // probability of this happening is roughly 1 in 21e12. If you are lucky
+    // enough to witness this, you should be fine when you run the test again.
+    // However, if you can reliably make the test fail again, you can be sure
+    // that the code is not working as intended.
+    assert.ok(verbatimPrefixes < 2, 'sampling a string should not just return a prefix');
+    // END of regression test for #2927.
   });
 
   QUnit.test('toArray', function(assert) {

underscore-esm.js

@@ -1506,6 +1506,19 @@ function min(obj, iteratee, context) {
   return result;
 }
 
+// Safely create a real, live array from anything iterable.
+var reStrSymbol = /[^\ud800-\udfff]|[\ud800-\udbff][\udc00-\udfff]|[\ud800-\udfff]/g;
+function toArray(obj) {
+  if (!obj) return [];
+  if (isArray(obj)) return slice.call(obj);
+  if (isString(obj)) {
+    // Keep surrogate pair characters together.
+    return obj.match(reStrSymbol);
+  }
+  if (isArrayLike(obj)) return map(obj, identity);
+  return values(obj);
+}
+
 // Sample **n** random values from a collection using the modern version of the
 // [Fisher-Yates shuffle](https://en.wikipedia.org/wiki/Fisher–Yates_shuffle).
 // If **n** is not specified, returns a single random element.
@@ -1515,7 +1528,7 @@ function sample(obj, n, guard) {
     if (!isArrayLike(obj)) obj = values(obj);
     return obj[random(obj.length - 1)];
   }
-  var sample = isArrayLike(obj) ? clone(obj) : values(obj);
+  var sample = toArray(obj);
   var length = getLength(sample);
   n = Math.max(Math.min(n, length), 0);
   var last = length - 1;
@@ -1592,19 +1605,6 @@ var partition = group(function(result, value, pass) {
   result[pass ? 0 : 1].push(value);
 }, true);
 
-// Safely create a real, live array from anything iterable.
-var reStrSymbol = /[^\ud800-\udfff]|[\ud800-\udbff][\udc00-\udfff]|[\ud800-\udfff]/g;
-function toArray(obj) {
-  if (!obj) return [];
-  if (isArray(obj)) return slice.call(obj);
-  if (isString(obj)) {
-    // Keep surrogate pair characters together.
-    return obj.match(reStrSymbol);
-  }
-  if (isArrayLike(obj)) return map(obj, identity);
-  return values(obj);
-}
-
 // Return the number of elements in a collection.
 function size(obj) {
   if (obj == null) return 0;