nullPointer while running test262 in non-interpreted mode #1074
Comments
p-bakker
added
bug
|
I did some initial investigation and this one isn't a simple fix. The code generator is getting into a bad state. It shouldn't be too hard to turn the NullPointerException into some exception that can be caught in JavaScript code, but anything else will take some deeper changes. |
|
Examples of JS code that throws exceptions: |
|
Would be very nice to get rid of this nullPointer, because then we can uncomment the printing of unexpected exceptions in the test262TestSuite, so things like this become more obvious |
|
@tuchida just wondering: you've been fixing a bunch of internal errors to be properly handled the last couple of days. Would this one be up your alley as well? If we'd be able to fix this one, we could enable the printing of unexpected exceptions in the Test262 TestSuite runner, as to get earlier feedback if something is amiss |
|
Does seem like this exact nullPointer is gone now! But now seeing this one, but only on language\expressions\object\accessor-name-computed-yield-expr |
|
I haven't looked at it too closely, but this fixed the nullPointer. diff --git a/rhino/src/main/java/org/mozilla/javascript/Node.java b/rhino/src/main/java/org/mozilla/javascript/Node.java
index 49a30d8..e8c2b5a 100644
--- a/rhino/src/main/java/org/mozilla/javascript/Node.java
+++ b/rhino/src/main/java/org/mozilla/javascript/Node.java
@@ -1165,11 +1165,16 @@ public class Node implements Iterable<Node> {
}
break;
case OBJECT_IDS_PROP:
+ case OBJECT_IDS_COMPUTED_PROP:
{
Object[] a = (Object[]) x.objectValue;
sb.append("[");
for (int i = 0; i < a.length; i++) {
- sb.append(a[i].toString());
+ if (a[i] != null) {
+ sb.append(a[i].toString());
+ } else {
+ sb.append("null");
+ }
if (i + 1 < a.length) sb.append(", ");
}
sb.append("]");
diff --git a/rhino/src/main/java/org/mozilla/javascript/NodeTransformer.java b/rhino/src/main/java/org/mozilla/javascript/NodeTransformer.java
index 8b73b09..a787d11 100644
--- a/rhino/src/main/java/org/mozilla/javascript/NodeTransformer.java
+++ b/rhino/src/main/java/org/mozilla/javascript/NodeTransformer.java
@@ -389,6 +389,23 @@ public class NodeTransformer {
}
break;
}
+
+ case Token.OBJECTLIT:
+ {
+ Object[] computedPropertyIds =
+ (Object[]) node.getProp(Node.OBJECT_IDS_COMPUTED_PROP);
+ if (computedPropertyIds != null) {
+ for (Object computedPropertyId : computedPropertyIds) {
+ if (computedPropertyId == null) continue;
+ transformCompilationUnit_r(
+ tree,
+ (Node) computedPropertyId,
+ node instanceof Scope ? (Scope) node : scope,
+ createScopeObjects,
+ inStrictMode);
+ }
+ }
+ }
}
transformCompilationUnit_r(
diff --git a/tests/testsrc/test262.properties b/tests/testsrc/test262.properties
index ff092f3..7c36334 100644
--- a/tests/testsrc/test262.properties
+++ b/tests/testsrc/test262.properties
@@ -4279,7 +4279,7 @@ language/expressions/new 41/59 (69.49%)
~language/expressions/new.target
-language/expressions/object 818/1081 (75.67%)
+language/expressions/object 817/1081 (75.58%)
dstr/async-gen-meth-ary-init-iter-close.js {unsupported: [async-iteration, async]}
dstr/async-gen-meth-ary-init-iter-get-err.js {unsupported: [async-iteration]}
dstr/async-gen-meth-ary-init-iter-get-err-array-prototype.js {unsupported: [async-iteration]}
@@ -5037,7 +5037,6 @@ language/expressions/object 818/1081 (75.67%)
__proto__-permitted-dup.js {unsupported: [async-iteration, async-functions]}
__proto__-permitted-dup-shorthand.js
accessor-name-computed-in.js
- accessor-name-computed-yield-expr.js non-interpreted
accessor-name-computed-yield-id.js non-strict
accessor-name-literal-numeric-binary.js {strict: [-1], non-strict: [-1]}
accessor-name-literal-numeric-exponent.js {strict: [-1], non-strict: [-1]} |
|
Great work @tuchida Are you gonna put this up as a PR? |
|
@p-bakker If you see this as a Computed Properties issue, I would like to think about it in more detail.
I am interested in this. If you need to fix for this, feel to use the diff. |
Not exactly sure what you mean by that, I'm afraid. The test that currently throws the nullPointer didn't seem to use computed properties, but computed assessor using yield as namen inside a generator function. Whether that falls under 'Computer Priperties' I'm not sure |
How can I check this? Did you make any changes to Test262SuiteTest.java when you checked #1074 (comment)? |
|
I just ran the Test262 Test Suite on the latest master, containing you other fixes. Note that this new nullPointer only happens in non-interpreted mode |
|
@p-bakker What is the goal of this issue? If it is just a problem with accessor-name-computed-yield-expr.js, the comment will fix it. However, it does not guarantee that |
|
@tuchida if I run the full Ttest262 suite, the only test that throws this nullPoiter is the With your patch applied, the nullPointer guess away and the test passed. To see the nullPointer when running the test262 testsuite, uncomment this line The goal in eliminating the nullPointer, making the test pass. If you think there are other ways to trigger the nullPointer, but that existing tests just don't hit that scenario, I'd still think it's preferred to fix it as just for the currently broken test and then make a follow-up case with a failing testcase to fix afterwards Makes sense? |
This is a followup of #988, as one of th ementioned nullPointers wasn't fixed by #1073
The text was updated successfully, but these errors were encountered: