Merge pull request #2379 from svaarala/json-replacer-setup-improvement

Improve JSON.stringify() array replacer handling

releases/releases.yaml

@@ -1374,6 +1374,7 @@ duktape_releases:
     - "Accept unlabelled function statements outside of top level for strict functions (using hoist semantics), previously they were rejected with a SyntaxError (GH-2213)"
     - "Accept unescaped U+2028 and U+2029 in string literals so that all JSON.stringify() output parses with eval() (ES2019) (GH-2235)"
     - "Don't treat U+180E as whitespace e.g. for String trim() purposes, (ES2016, requires Unicode 8.0.0 or higher) (GH-2236)"
+    - "Align JSON.stringify() Array 'replacer' setup with the latest specification (GH-2379)"
     - "Use wasm for dukweb.js compilation (including duktape.org site), fix async loading of emcc-compiled code in dukweb.html (GH-2244)"
-    - "Fix duk_opcodes.yaml metadata for TRYCATCH and CALLn (GH-2277)"
     - "Improve DUK_USE_OS_STRING for macOS, iOS, watchOS, and tvOS (GH-2288)"
+    - "Fix JSON.stringify() handling of Array 'replacer' duplicates (e.g. JSON.stringify({foo: 123}, [\"foo\", \"foo\"])); previously incorrectly serialized multiple times, now only once (GH-2379)"

src-input/duk_bi_json.c

@@ -2903,6 +2903,50 @@ void duk_bi_json_parse_helper(duk_hthread *thr,
 	DUK_ASSERT(duk_get_top(thr) == entry_top + 1);
 }
 
+DUK_LOCAL void duk__json_setup_plist_from_array(duk_hthread *thr, duk_json_enc_ctx *js_ctx, duk_idx_t idx_replacer) {
+	/* ES5.1 required enumeration, later specification versions use an
+	 * explicit index loop (and makes it clear inheritance is required).
+	 */
+
+	duk_uarridx_t repl_len;
+	duk_uarridx_t repl_idx;
+	duk_uarridx_t plist_idx = 0;
+
+	js_ctx->idx_proplist = duk_push_bare_array(thr);
+	(void) duk_push_bare_object(thr);
+
+	/* [ ... proplist found ] */
+
+	repl_len = (duk_uarridx_t) duk_get_length(thr, idx_replacer);
+	for (repl_idx = 0; repl_idx < repl_len; repl_idx++) {
+		(void) duk_get_prop_index(thr, idx_replacer, repl_idx);
+		/* Accept strings, numbers, Strings, and Numbers, and ignore
+		 * anything else.  Reject duplicates.
+		 */
+		if (duk__json_enc_allow_into_proplist(DUK_GET_TVAL_NEGIDX(thr, -1))) {
+			(void) duk_to_string(thr, -1);  /* extra coercion of strings is OK */
+			duk_dup_top(thr);  /* -> [ ... proplist found key key ] */
+			(void) duk_get_prop(thr, -3);  /* -> [ ... proplist found key found[key] ] */
+			if (duk_to_boolean(thr, -1)) {
+				duk_pop_2_unsafe(thr);
+			} else {
+				duk_pop_unsafe(thr);
+				duk_dup_top(thr);
+				duk_push_true(thr);  /* -> [ ... proplist found key key true ] */
+				(void) duk_put_prop(thr, -4);  /* -> [ ... proplist found key ] */
+				(void) duk_put_prop_index(thr, -3, plist_idx);  /* -> [ ... proplist found ] */
+				plist_idx++;
+			}
+		} else {
+			duk_pop_unsafe(thr);
+		}
+	}
+
+	duk_pop_unsafe(thr);
+
+	/* [ ... proplist ] */
+}
+
 DUK_INTERNAL
 void duk_bi_json_stringify_helper(duk_hthread *thr,
                                   duk_idx_t idx_value,
@@ -3023,41 +3067,7 @@ void duk_bi_json_stringify_helper(duk_hthread *thr,
 		if (DUK_HOBJECT_IS_CALLABLE(h)) {
 			js_ctx->h_replacer = h;
 		} else if (duk_js_isarray_hobject(h)) {
-			/* Here the specification requires correct array index enumeration
-			 * which is a bit tricky for sparse arrays (it is handled by the
-			 * enum setup code).  We now enumerate ancestors too, although the
-			 * specification is not very clear on whether that is required.
-			 */
-
-			duk_uarridx_t plist_idx = 0;
-			duk_small_uint_t enum_flags;
-
-			js_ctx->idx_proplist = duk_push_bare_array(thr);
-
-			enum_flags = DUK_ENUM_ARRAY_INDICES_ONLY |
-			             DUK_ENUM_SORT_ARRAY_INDICES;  /* expensive flag */
-			duk_enum(thr, idx_replacer, enum_flags);
-			while (duk_next(thr, -1 /*enum_index*/, 1 /*get_value*/)) {
-				/* [ ... proplist enum_obj key val ] */
-				if (duk__json_enc_allow_into_proplist(duk_get_tval(thr, -1))) {
-					/* XXX: duplicates should be eliminated here */
-					DUK_DDD(DUK_DDDPRINT("proplist enum: key=%!T, val=%!T --> accept",
-					                     (duk_tval *) duk_get_tval(thr, -2),
-					                     (duk_tval *) duk_get_tval(thr, -1)));
-					duk_to_string(thr, -1);  /* extra coercion of strings is OK */
-					duk_put_prop_index(thr, -4, plist_idx);  /* -> [ ... proplist enum_obj key ] */
-					plist_idx++;
-					duk_pop(thr);
-				} else {
-					DUK_DDD(DUK_DDDPRINT("proplist enum: key=%!T, val=%!T --> reject",
-					                     (duk_tval *) duk_get_tval(thr, -2),
-					                     (duk_tval *) duk_get_tval(thr, -1)));
-					duk_pop_2(thr);
-				}
-                        }
-                        duk_pop(thr);  /* pop enum */
-
-			/* [ ... proplist ] */
+			duk__json_setup_plist_from_array(thr, js_ctx, idx_replacer);
 		}
 	}
 

tests/ecmascript/test-bi-json-enc-proplist-ancestor.js

@@ -3,18 +3,19 @@
 0 quux
 1 foo
 2 baz
-{"quux":3,"foo":1,"baz":4}
+{"quux":3,"foo":1}
+1 foo
+0 baz
+2 baz
+{"baz":4,"foo":1}
 ===*/
 
 /* Should ancestor properties be enumerated when creating PropertyList?
  * Specification is a bit unclear on this point.  See E5.1 Section 15.12.3,
  * main algorithm, step 4.b.ii.
  *
- * We currently iterate ancestor array indexed properties too.
- *
- * Creating an array instance with a chosen internal prototype is apparently
- * not possible.  However, Array.prototype is itself an array and we can add
- * a value to it.
+ * Newer specifications use an explicit index loop (to .length - 1) and make
+ * it clear ancestors are enumerated (but not beyond .length - 1).
  */
 
 function propertyListAncestorTest() {
@@ -22,23 +23,25 @@ function propertyListAncestorTest() {
     var k;
     var plist;
 
-    // baseline
+    // Baseline.
     print(JSON.stringify(obj, [ 'quux', 'foo' ]));
 
-    // this is not very common
+    // Plist will contain "0" and "1"; it will inherit "2" but it's >= .length so it's ignored
+    // by JSON.stringify().
     Array.prototype[2] = 'baz';
-
-    // plist will contain "0" and "1", and will inherit "2"
     plist = [ 'quux', 'foo' ];
     for (var k in plist) {
         print(k, plist[k]);
     }
+    print(JSON.stringify(obj, plist));
 
-    // Now E5.1 Section 15.12.3, main algorithm, step 4.b.ii should presumably
-    // iterate indices "0" and "1" from the array and "2" from the ancestor.
-    // Note that 'length' plays no role in this.
-    //
-    // (Both Rhino and V8 fail this test.)
+    // If the gap is at < .length, it gets looked up from an ancestor.
+    Array.prototype[0] = 'baz';
+    plist = [];
+    plist[1] = 'foo';
+    for (var k in plist) {
+        print(k, plist[k]);
+    }
     print(JSON.stringify(obj, plist));
 }
 

tests/ecmascript/test-bi-json-enc-proplist-dups.js

@@ -6,8 +6,6 @@
 /* E5.1 Section 15.12.13, main algorithm, step 4.b.ii.5 requires that an
  * implementation detect duplicates in the property list and refuse to
  * serialize the same property name twice.
- *
- * Note: both V8 and Rhino fail this test (but in different ways).
  */
 
 function dupTest1() {

tests/ecmascript/test-bi-json-enc-proplist-large.js

@@ -0,0 +1,27 @@
+/*===
+{"0":0,"prop0":0,"1":1,"prop1":1,"2":2,"prop2":2,"3":3,"prop3":3,"4":4,"prop4":4,"5":5,"prop5":5,"6":6,"prop6":6,"7":7,"prop7":7,"8":8,"prop8":8,"9":9,"prop9":9,"10":10,"prop10":10,"11":11,"prop11":11,"12":12,"prop12":12,"13":13,"prop13":13,"14":14,"prop14":14,"15":15,"prop15":15,"16":16,"prop16":16,"17":17,"prop17":17,"18":18,"prop18":18,"19":19,"prop19":19,"20":20,"prop20":20,"21":21,"prop21":21,"22":22,"prop22":22,"23":23,"prop23":23,"24":24,"prop24":24,"25":25,"prop25":25,"26":26,"prop26":26,"27":27,"prop27":27,"28":28,"prop28":28,"29":29,"prop29":29,"30":30,"prop30":30,"31":31,"prop31":31,"32":32,"prop32":32,"33":33,"prop33":33,"34":34,"prop34":34,"35":35,"prop35":35,"36":36,"prop36":36,"37":37,"prop37":37,"38":38,"prop38":38,"39":39,"prop39":39,"40":40,"prop40":40,"41":41,"prop41":41,"42":42,"prop42":42,"43":43,"prop43":43,"44":44,"prop44":44,"45":45,"prop45":45,"46":46,"prop46":46,"47":47,"prop47":47,"48":48,"prop48":48,"49":49,"prop49":49,"50":50,"prop50":50,"51":51,"prop51":51,"52":52,"prop52":52,"53":53,"prop53":53,"54":54,"prop54":54,"55":55,"prop55":55,"56":56,"prop56":56,"57":57,"prop57":57,"58":58,"prop58":58,"59":59,"prop59":59,"60":60,"prop60":60,"61":61,"prop61":61,"62":62,"prop62":62,"63":63,"prop63":63,"64":64,"prop64":64,"65":65,"prop65":65,"66":66,"prop66":66,"67":67,"prop67":67,"68":68,"prop68":68,"69":69,"prop69":69,"70":70,"prop70":70,"71":71,"prop71":71,"72":72,"prop72":72,"73":73,"prop73":73,"74":74,"prop74":74,"75":75,"prop75":75,"76":76,"prop76":76,"77":77,"prop77":77,"78":78,"prop78":78,"79":79,"prop79":79,"80":80,"prop80":80,"81":81,"prop81":81,"82":82,"prop82":82,"83":83,"prop83":83,"84":84,"prop84":84,"85":85,"prop85":85,"86":86,"prop86":86,"87":87,"prop87":87,"88":88,"prop88":88,"89":89,"prop89":89,"90":90,"prop90":90,"91":91,"prop91":91,"92":92,"prop92":92,"93":93,"prop93":93,"94":94,"prop94":94,"95":95,"prop95":95,"96":96,"prop96":96,"97":97,"prop97":97,"98":98,"prop98":98,"99":99,"prop99":99}
+===*/
+
+function test() {
+    var obj = { foo: 1, bar: 2, quux: 3, baz: 4 };
+    var i;
+
+    for (i = 0; i < 100; i++) {
+        obj[i] = i;
+        obj['prop' + i] = i;
+    }
+
+    var plist = [];
+    for (i = 0; i < 1000000; i++) {
+        plist.push((i % 1000));
+        plist.push('prop' + (i % 900));
+    }
+
+    print(JSON.stringify(obj, plist));
+}
+
+try {
+    test();
+} catch (e) {
+    print(e);
+}

tests/knownissues/test-bi-json-enc-proplist-dups-1.txt

@@ -1,4 +0,0 @@
-summary: dups in proplist not handled correctly
----
-{"bar":2,"foo":1,"baz":4,"baz":4,"baz":4,"quux":3}
-{"bar":2,"1":5,"1":5,"1":5,"foo":1}