Implement String.raw #879

tonygermano · 2021-05-02T10:39:51Z

Fixed merge conflict and bugs from 3rd commit of #81

Bug fixes were likely due to changes in spec since the original PR.

I separated this out since it isn't actually dependent on template literals.

All test262 tests are now passing except for the four which depend on template literals themselves.

rbri · 2021-05-02T11:00:50Z

Great, another step forward

gbrail

Would it be possible to cast around for a test suite that hits more code coverage? For instance:

https://github.com/v8/v8/blob/master/test/mjsunit/es6/string-raw.js

We've used a bunch of these in the "testsrc/jstests" directory. They don't always pass due to other things that we're missing, and when that happens I put a "TODO Rhino" comment around them, but that might help, since I see a bunch of the corner cases here not being covered. (And JavaScript, it turns out, is about 90% corner cases!)

Sorry, I am getting gradually more picky since I see so much good new stuff coming ;-)

gbrail · 2021-05-03T21:03:29Z

src/org/mozilla/javascript/NativeString.java

+            /* step 8 a-i */
+            Object next;
+            if (nextIndex > Integer.MAX_VALUE) {
+                next = raw.get(Long.toString(nextIndex), raw);


Would you mind looking to see if there are any other tests, perhaps in the v8 project, that we could use to get better coverage? For example, I see that this case (of the index being greater than a 32-bit int) isn't being tested.

Can I actually just change nextIndex from a long to an int, and get rid of that step all together? It will be impossible to hit that number, because args would have to be an Object[] with Integer.MAX_VALUE+1 elements.

Hmm, I guess raw can have a length up to 2^53-1, and if that happens, it will stop doing substitutions from args and use empty strings for the remainder of the substitutions. Do we really want a test that has to iterate that many times? It will probably cause an out of memory error.

I think it would get an out of memory error because either raw would have to have a massive amount of properties, or it is sparse, and the output will repeat "undefined" for each index that doesn't have an element.

just to confirm, I ran the following in a shell

Rhino 1.7.14-SNAPSHOT 2021 05 05 js> String.raw({raw:{length:5}}) undefinedundefinedundefinedundefinedundefined js> String.raw({raw:{length:Math.pow(2,32)+1}}) java.lang.OutOfMemoryError: Java heap space at java.base/java.util.Arrays.copyOf(Arrays.java:3745) at java.base/java.lang.AbstractStringBuilder.ensureCapacityInternal(AbstractStringBuilder.java:172) at java.base/java.lang.AbstractStringBuilder.append(AbstractStringBuilder.java:538) at java.base/java.lang.StringBuilder.append(StringBuilder.java:174) at org.mozilla.javascript.NativeString.js_raw(NativeString.java:1145) at org.mozilla.javascript.NativeString.execIdCall(NativeString.java:398) at org.mozilla.javascript.IdFunctionObject.call(IdFunctionObject.java:100) at org.mozilla.javascript.optimizer.OptRuntime.call1(OptRuntime.java:45) at org.mozilla.javascript.gen._stdin__2._c_script_0(Unknown Source) at org.mozilla.javascript.gen._stdin__2.call(Unknown Source) at org.mozilla.javascript.ContextFactory.doTopCall(ContextFactory.java:415) at org.mozilla.javascript.ScriptRuntime.doTopCall(ScriptRuntime.java:3603) at org.mozilla.javascript.gen._stdin__2.call(Unknown Source) at org.mozilla.javascript.gen._stdin__2.exec(Unknown Source) at org.mozilla.javascript.tools.shell.Main.processSource(Main.java:497) at org.mozilla.javascript.tools.shell.Main.processFiles(Main.java:181) at org.mozilla.javascript.tools.shell.Main$IProxy.run(Main.java:101) at org.mozilla.javascript.Context.call(Context.java:534) at org.mozilla.javascript.ContextFactory.call(ContextFactory.java:525) at org.mozilla.javascript.tools.shell.Main.exec(Main.java:163) at org.mozilla.javascript.tools.shell.Main.main(Main.java:138) js: exception from uncaught JavaScript throw: java.lang.OutOfMemoryError: Java heap space js>

I'm find with leaving this untested, but it raises a bigger problem that we have in a lot of places -- JavaScript strings are supposed to have a maximum length of 2^53 but Java strings have a maximum length of 2^31. I don't think that we can change that all over Rhino. But if someone DOES try to create a raw string that's longer than that, then we'll likely leak a runtime exception from somewhere in Java.

I think we're better off here throwing an exception if the length is larger than the maximum integer size rather than let some other kind of exception bubble up.

So, if literalSegments > Integer.MAX_VALUE throw a RangeError? And then I could make nextIndex an int and remove this check?

Yes! I think that would prevent worse problems later.

One other question that I wasn't able to find an answer to by searching...
Is there a negative performance impact for checking if (a == b) many times when one variable is an int and another is a long? My understanding is that the int is implicitly converted to a long. Would it be better to do this to avoid the conversions later, or does it not really matter?

long rawLength = NativeArray.getLengthProperty(cx, raw); if (rawLength > Integer.MAX_VALUE) {/*throw RangeError*/} int literalSegments = (int) rawLength;

I went ahead and implemented it the way I asked about in my last question. In addition to removing the implicit conversion in the comparison, it also removed the need to explicitly cast long to int in two places further down in the code.

gbrail · 2021-05-03T21:04:13Z

src/org/mozilla/javascript/NativeString.java

+        Scriptable cooked = ScriptRuntime.toObject(cx, scope, arg0);
+        /* step 3 */
+        Object rawValue = cooked.get("raw", cooked);
+        if (rawValue == NOT_FOUND) rawValue = undefined;


FWIW if I am reading "./gradlew jacocoTestReport" correctly, the "NOT_FOUND" case here isn't actually being tested.

Updated this to use ScriptRuntime.getObjectProp instead, which does the correct ecma [[Get]] operation. It now searches up the prototype chain and returns undefined if not found in a single call. The test that covers raw being undefined is template-raw-not-object-throws.js

gbrail · 2021-05-03T21:04:57Z

src/org/mozilla/javascript/NativeString.java

+    private static CharSequence js_raw(Context cx, Scriptable scope, Object[] args) {
+        final Object undefined = Undefined.instance;
+        /* step 1-2 */
+        Object arg0 = args.length > 0 ? args[0] : undefined;


I'm not sure if the "undefined" case is being tested here.

template-not-object-throws.js tests String.raw(undefined). Is that good enough or do you want a String.raw(), too?

gbrail · 2021-05-05T18:55:19Z

Thanks, this looks good -- I agree that we don't want to have a unit test that has more than 2^32 elements anyway.

tonygermano · 2021-05-06T12:54:41Z

src/org/mozilla/javascript/NativeString.java

+        for (; ; ) {
+            /* step 8 a-i */
+            Object next;
+            next = ScriptRuntime.getObjectIndex(raw, nextIndex, cx);


This also changed from the original PR. Instead of using Scriptable.get(int Scriptable), it now uses ScriptRuntime.getObjectIndex, which searches up the prototype chain and returns undefined when not found, rather than only searching the current object, and returning Scriptable.NOT_FOUND.

I included a test to make sure the prototype searching is working correctly.

gbrail · 2021-05-07T01:10:15Z

The code looks great and thanks for the attention to detail. It looks like this fixes a bunch of test262 tests -- I can see the output here:

Could you enable those String.raw tests in test262 in this PR, or if you're really tired of iterating, in another PR? Thanks!

org.mozilla.javascript.tests.Test262SuiteTest STANDARD_OUT
Test is marked as failing but it does not: test262/test/built-ins/DataView/prototype/getBigInt64/return-abrupt-from-tonumber-byteoffset-symbol.js
Test is marked as failing but it does not: test262/test/built-ins/RegExp/prototype/Symbol.match/builtin-failure-g-set-lastindex-err.js
Test is marked as failing but it does not: test262/test/built-ins/RegExp/prototype/Symbol.match/g-init-lastindex-err.js
Test is marked as failing but it does not: test262/test/built-ins/String/raw/length.js
Test is marked as failing but it does not: test262/test/built-ins/String/raw/name.js
Test is marked as failing but it does not: test262/test/built-ins/String/raw/raw.js
Test is marked as failing but it does not: test262/test/built-ins/String/raw/return-empty-string-from-empty-array-length.js
Test is marked as failing but it does not: test262/test/built-ins/String/raw/return-empty-string-if-length-is-negative-infinity.js
Test is marked as failing but it does not: test262/test/built-ins/String/raw/return-empty-string-if-length-is-not-defined.js
Test is marked as failing but it does not: test262/test/built-ins/String/raw/return-empty-string-if-length-is-undefined.js
Test is marked as failing but it does not: test262/test/built-ins/String/raw/return-empty-string-if-length-is-zero-NaN.js
Test is marked as failing but it does not: test262/test/built-ins/String/raw/return-empty-string-if-length-is-zero-boolean.js
Test is marked as failing but it does not: test262/test/built-ins/String/raw/return-empty-string-if-length-is-zero-null.js
Test is marked as failing but it does not: test262/test/built-ins/String/raw/return-empty-string-if-length-is-zero-or-less-number.js
Test is marked as failing but it does not: test262/test/built-ins/String/raw/return-empty-string-if-length-is-zero-or-less-string.js
Test is marked as failing but it does not: test262/test/built-ins/String/raw/return-the-string-value.js
Test is marked as failing but it does not: test262/test/built-ins/String/raw/returns-abrupt-from-next-key-toString.js
Test is marked as failing but it does not: test262/test/built-ins/String/raw/returns-abrupt-from-next-key.js
Test is marked as failing but it does not: test262/test/built-ins/String/raw/returns-abrupt-from-substitution.js
Test is marked as failing but it does not: test262/test/built-ins/String/raw/substitutions-are-appended-on-same-index.js
Test is marked as failing but it does not: test262/test/built-ins/String/raw/substitutions-are-limited-to-template-raw-length.js
Test is marked as failing but it does not: test262/test/built-ins/String/raw/template-length-throws.js
Test is marked as failing but it does not: test262/test/built-ins/String/raw/template-raw-throws.js
Test is marked as failing but it does not: test262/test/language/expressions/greater-than-or-equal/bigint-and-string.js
Test is marked as failing but it does not: test262/test/language/expressions/less-than/bigint-and-string.js

tonygermano · 2021-05-07T01:18:58Z

Oops. I had enabled all of the String.raw ones in the original PR, but I must have killed it in a rebase. I'll try to get that back in in a bit. I'll throw those few extra ones that aren't related to this PR in a separate commit so we don't need to make a whole new PR for them.

tonygermano · 2021-05-07T15:53:22Z

test262 updates are now back in this PR. I think it's ready.

gbrail · 2021-05-07T23:05:03Z

Looks good -- thanks!

gbrail reviewed May 3, 2021

View reviewed changes

tonygermano force-pushed the implement-string-raw branch 2 times, most recently from f438476 to b7d6281 Compare May 6, 2021 05:12

tonygermano commented May 6, 2021

View reviewed changes

tonygermano added 3 commits May 7, 2021 00:18

spotlessApply NativeString

4e3b34a

Implement String.raw

54ceafd

cleanup a few passing test262 tests

2add1ce

tonygermano force-pushed the implement-string-raw branch from b7d6281 to 2add1ce Compare May 7, 2021 04:20

gbrail merged commit 7d9ae82 into mozilla:master May 7, 2021

tonygermano deleted the implement-string-raw branch May 11, 2021 23:54

tonygermano mentioned this pull request Jun 11, 2021

Patch for Bug 783507 ("Implement ES.next quasi-literals (Rhino)") #81

Closed

p-bakker added the feature Issues considered a new feature label Oct 13, 2021

p-bakker added this to the Release 1.7.14 milestone Oct 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement String.raw #879

Implement String.raw #879

tonygermano commented May 2, 2021 •

edited

rbri commented May 2, 2021

gbrail left a comment

gbrail May 3, 2021

tonygermano May 4, 2021

tonygermano May 4, 2021

tonygermano May 4, 2021

tonygermano May 5, 2021

gbrail May 5, 2021

tonygermano May 5, 2021 •

edited

gbrail May 5, 2021

tonygermano May 5, 2021

tonygermano May 6, 2021

gbrail May 3, 2021

tonygermano May 6, 2021

gbrail May 3, 2021

tonygermano May 6, 2021

gbrail commented May 5, 2021

tonygermano May 6, 2021 •

edited

gbrail commented May 7, 2021

tonygermano commented May 7, 2021

tonygermano commented May 7, 2021

gbrail commented May 7, 2021

Implement String.raw #879

Implement String.raw #879

Conversation

tonygermano commented May 2, 2021 • edited

rbri commented May 2, 2021

gbrail left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tonygermano May 5, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbrail commented May 5, 2021

tonygermano May 6, 2021 • edited

Choose a reason for hiding this comment

gbrail commented May 7, 2021

tonygermano commented May 7, 2021

tonygermano commented May 7, 2021

gbrail commented May 7, 2021

tonygermano commented May 2, 2021 •

edited

tonygermano May 5, 2021 •

edited

tonygermano May 6, 2021 •

edited