Template literals convert substitutions too late (and look up String.prototype.concat)

[Iann29]

Hi! First, thank you for QuickJS-ng. I run the same JavaScript on QuickJS-ng
and on V8 and compare the results, and I found a small difference in
template literals.

QuickJS-ng compiles `a${x}b${y}` as "a".concat(x, "b", y). Two things
follow from that:

1. Each substitution is converted to a string only at the end, after all of
   them were evaluated. The spec converts each one with ToString right after
   evaluating it (ECMA-262, 13.2.8.6, step 4 before step 5).
2. String.prototype.concat is looked up at run time, so code that replaces
   it changes every template.

Repro (plain JS):

const d = new Date(2024, 0, 15, 12);
console.log(`${d} / ${d.setFullYear(2000)}`.slice(0, 15));

const order = [];
const obj = { toString() { order.push("toString"); return "obj"; } };
`${obj}${(order.push("next"), "!")}`;
console.log(order.join(","));

String.prototype.concat = function () { return "concat was called"; };
console.log(`a${1}b`);

	1	2	3
V8 (node 22)	Mon Jan 15 2024	toString,next	a1b
SpiderMonkey (JavaScript-C159.0a1)	Mon Jan 15 2024	toString,next	a1b
QuickJS-ng v0.17.0 and master (60984dc)	Sat Jan 15 2000	next,toString	concat was called

The fix is small and only touches untagged templates in js_parse_template:
after each substitution it emits OP_to_propkey + OP_add (ToString right
away, with a TypeError for a symbol as before), and OP_add for each later
chunk, instead of get_field2 concat + call_method. It also removes the
old XXX: should convert to string at this stage? comment, since this
answers it. Tagged templates are unchanged.

I added test_template_tostring_order to tests/test_language.js. It fails
on master and passes with the change. make test gives 0/120 errors, and
test262's template-literal and tagged-template folders still pass
(0/160 errors). As far as I can see, no test262 test covers this order; I
can propose one there too if that helps.

Happy to change anything you'd prefer done differently. Thanks again!

[bnoordhuis]

Seems fine to me. You can remove the concat atom from quickjs-atom.h, there are no other users anymore after your change.

edit: although that affects bytecode generation so you probably also have to bump BC_VERSION and run make codegen

edit2: this change affects codegen anyway, running make codegen is unavoidable.

[Iann29]

Thanks for the review! Done: I removed the now-unused concat atom, bumped BC_VERSION to 29 and regenerated the bytecode with make codegen. make test still passes (0/120 errors).