-
Notifications
You must be signed in to change notification settings - Fork 904
Fix Template Literal String Conversion for ECMAScript Compliance #2065
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@dilbertKocik many thanks for this PR. The CI failures are because you have manually edited the test262.properties. The idea of this is to regenerate the file - see https://github.com/mozilla/rhino/tree/master/tests#updating-the-test262properties-file for more It will be great if you can regenerate the file according to the docu - i'm sure the ci build will pass then and we can merge your great PR. |
|
@dilbertKocik maybe you have an idea about a good place in the code to document the idea behind the new token a bit. Hope this will help others in the future to understand why we have the two add tokens. |
5d3b169 to
61e07e3
Compare
|
@dilbertKocik can you please rebase and force push again, i see some conflicts here |
|
@rbri Yeah, I see it. I rebased my branch locally but I'm seeing a large diff in the test262.properties file (approximately 500 lines). I'm trying to figure out why that's happening. All the other changes look the same as the PR. I tried regenerating the file but it looks like the same large diff. I'll try to sort this out on Monday. |
61e07e3 to
0c1c5c5
Compare
|
@rbri Can you tell me if this test update makes sense? I don't understand how so many tests could be affected. But this is what I get when I run the generator per the documentation. |
|
@dilbertKocik will have a look the ci fails because your new comments are violating the spotless rules |
|
@dilbertKocik looks like you have reverted the update of the linked test262 project as part of your commits. Therefore you get this many differences. Not sure how to sort this out - maybe the easiest way is to start a new branch from head and copy your changed files and make a new pr out of this
|
0c1c5c5 to
2734efd
Compare
|
@rbri Thanks for the help. Starting from head and cherry picking was great advice. |
gbrail
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I'd really like to see the actual "concat" logic not be duplicated between interpreted and compiled mode -- see the comments. Thanks!
| lhs = ScriptRuntime.wrapNumber(frame.sDbl[state.stackTop - 1]); | ||
| } | ||
|
|
||
| String rhsString = ScriptRuntime.toString(rhs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See below, but this is an opportunity to consolidate the "concat" logic into one call into ScriptRuntime rather than duplicating it in two places.
| return ScriptRuntime.add(val1, val2, cx); | ||
| } | ||
|
|
||
| public static Object concat(Object lhs, Object rhs) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please put this function in "ScriptRuntime" and then use it in both the interpreter code (see above) and in the bytecode? That way we only have the logic in here once. Yes, it's only three lines of code but it still means that we can put them together in the same place to change them in the future if we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good idea. Thanks for the suggestion.
|
@dilbertKocik thanks a lot, hope you provide more geat stuff like this in the future |

Summary
This commit addresses ECMAScript specification compliance issues with template literal string conversion by introducing a dedicated
STRING_CONCATtoken and operation that properly converts objects to strings usingtoString()semantics instead of the defaultToPrimitivebehavior.Problem
Template literals like
${Object(1n)}were incorrectly using the defaultToPrimitiveconversion (which callsvalueOf()first) instead of string conversion semantics (which callstoString()first). This caused Test262 compliance failures where customtoString()methods on BigInt and Symbol wrapper objects were not being invoked correctly.Before: Template literals were transformed into regular
ADDoperations, causing${Object(1n)}to return"1"instead of the expected customtoString()result.After: Template literals use dedicated
STRING_CONCAToperations that properly callScriptRuntime.toString(), ensuring ECMAScript compliance.Changes Made
New Token Type: Added
Token.STRING_CONCATto distinguish template literal string concatenation from arithmetic addition operations.IRFactory Enhancement: Modified
transformTemplateLiteral()to useSTRING_CONCATinstead ofADDtokens, ensuring template literal substitutions follow string conversion semantics.Interpreter Support: Implemented
DoStringConcatinstruction class that directly converts operands to strings usingScriptRuntime.toString()before concatenation.Optimizer Support: Added
STRING_CONCAThandling inBodyCodegenandOptRuntime.concat()method for optimized execution paths.Test Improvements: Removed 2 failing Test262 tests from the exclusion list:
built-ins/BigInt/wrapper-object-ordinary-toprimitive.jsbuilt-ins/Symbol/prototype/Symbol.toPrimitive/removed-symbol-wrapper-ordinary-toprimitive.jsTechnical Details
ToPrimitivehint usageToPrimitivecalls and type checking overheadImpact
This change brings Rhino's template literal implementation into full compliance with the ECMAScript specification while maintaining performance and backward compatibility.