Merge pull request #1121 from svaarala/array-sort-callback-retval

Clarify Array.prototype.sort() compareFn behavior

src-input/duk_bi_array.c

@@ -671,25 +671,25 @@ DUK_LOCAL duk_small_int_t duk__array_sort_compare(duk_context *ctx, duk_int_t id
 	if (!duk_is_undefined(ctx, idx_fn)) {
 		duk_double_t d;
 
-		/* no need to check callable; duk_call() will do that */
+		/* No need to check callable; duk_call() will do that. */
 		duk_dup(ctx, idx_fn);    /* -> [ ... x y fn ] */
 		duk_insert(ctx, -3);     /* -> [ ... fn x y ] */
 		duk_call(ctx, 2);        /* -> [ ... res ] */
 
-		/* The specification is a bit vague what to do if the return
-		 * value is not a number.  Other implementations seem to
-		 * tolerate non-numbers but e.g. V8 won't apparently do a
-		 * ToNumber().
+		/* ES5 is a bit vague about what to do if the return value is
+		 * not a number.  ES6 provides a concrete description:
+		 * http://www.ecma-international.org/ecma-262/6.0/#sec-sortcompare.
 		 */
 
-		/* XXX: best behavior for real world compatibility? */
-
 		d = duk_to_number(ctx, -1);
 		if (d < 0.0) {
 			ret = -1;
 		} else if (d > 0.0) {
 			ret = 1;
 		} else {
+			/* Because NaN compares to false, NaN is handled here
+			 * without an explicit check above.
+			 */
 			ret = 0;
 		}
 

tests/ecmascript/test-bi-array-proto-sort-comparefn.js

@@ -0,0 +1,83 @@
+/*
+ *  Compare function behavior, specified more concretely in ES6.
+ */
+
+/*===
+true
+true
+false
+1,2,3,4,5,6,7,8,9,10
+1,2,3,4,5,6,7,8,9,10
+1,2,3,4,5,6,7,8,9,10
+1,2,3,4,4,4,5,6,6,7,8,9,10
+===*/
+
+function test() {
+    var compareFnCalled = false;
+    var valueOfCalled = false;
+    var toStringCalled = false;
+    var arr;
+
+    function compareFn(a, b) {
+        compareFnCalled = true;
+        var ret = 0;
+        if (a < b) { ret = -1; }
+        if (a > b) { ret = 1; }
+        return {
+            valueOf: function () { valueOfCalled = true; return ret; },
+            toString: function () { toStringCalled = true; return ret; }
+        };
+    }
+
+    arr = [ 10, 4, 7, 3, 1, 2, 6, 5, 9, 8 ];
+
+    // Compare callback counts and arguments are up to the implementation, but
+    // we know it will be called at least once.  So, check that it gets called
+    // and that the result value is ToNumber() coerced based on both side effects
+    // and final sort result.
+
+    arr.sort(compareFn);
+
+    print(compareFnCalled);
+    print(valueOfCalled);
+    print(toStringCalled);
+    print(arr);
+
+    // ToNumber coercion means strings representing numbers can be returned.
+    arr = [ 10, 4, 7, 3, 1, 2, 6, 5, 9, 8 ];
+    arr.sort(function (a, b) {
+        var ret = '0.0';
+        if (a < b) { ret = '-1000'; }
+        if (a > b) { ret = '0xdeadbeef'; }
+        return ret;
+    });
+    print(arr);
+
+    // Inf and -Inf should be treated like 1 and -1.
+    arr = [ 10, 4, 7, 3, 1, 2, 6, 5, 9, 8 ];
+    arr.sort(function (a, b) {
+        var ret = 0/0;
+        if (a < b) { ret = -1/0; }
+        if (a > b) { ret = 1/0; }
+        return ret;
+    });
+    print(arr);
+
+    // If NaN is returned, it is treated like a +0.  We could return Nan for
+    // every element but that would *not* guarantee a stable sort because
+    // stability is not required (and not provided by current qsort approach).
+    arr = [ 10, 4, 7, 4, 4, 3, 1, 6, 2, 6, 5, 9, 8 ];
+    arr.sort(function (a, b) {
+        var ret = 0/0;
+        if (a < b) { ret = -1; }
+        if (a > b) { ret = 1; }
+        return ret;
+    });
+    print(arr);
+}
+
+try {
+    test();
+} catch (e) {
+    print(e.stack || e);
+}