Closed Bug 590772 Opened 10 years ago Closed 10 years ago

"Assertion failure: unexpected statement type, at ../jsreflect.cpp:1774"

Categories

(Core :: JavaScript Engine, defect, critical)

x86
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: jruderman, Assigned: dherman)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

Reflect.parse("for (var x = 3 in []) { }")

Assertion failure: unexpected statement type, at ../jsreflect.cpp:1774
Blocks: 590820
Attached patch for-in loops with initializers (obsolete) — Splinter Review
Fix attached, with test cases.

Dave
Assignee: general → dherman
Comment on attachment 469745 [details] [diff] [review]
for-in loops with initializers

>diff --git a/js/src/tests/js1_8_5/extensions/reflect-parse.js b/js/src/tests/js1_8_5/extensions/reflect-parse.js
>+function assertSyntaxError(src) {
>+    try {
>+        Reflect.parse(src);
>+    } catch (expected) {
>+        return;
>+    }
>+    throw new Error("expected syntax error for " + uneval(src));
>+}

In the catch block, can you check what kind of object |expected| is?  Hopefully, expected is an instance of SyntaxError - and that should be the requirement for passing this test.
> In the catch block, can you check what kind of object |expected| is?

Oops, yeah, meant to do that.

Dave
Attachment #469842 - Flags: review?(cdleary)
Comment on attachment 469842 [details] [diff] [review]
assertSyntaxError conditionally catches only SyntaxError

I haven't looked at TOK_SEQ before this bug, but your assertions look good! The test coverage was also on the mark -- a superset of the things I tried to use to break this patch. Always a good sign.

Only nit is that I would flip the conditional around to use an early-assert if the ListNode count is not equal to 2, but it's up to you.

Golly gee this stuff makes me want to refactor the parse node interface.
Attachment #469842 - Flags: review?(cdleary) → review+
Thanks for the suggestions. I also added the fixes I forgot for bug 590766. You can't, AFAIK, create a recursive binding in the head of a let-expression or let-statement, so I don't have tests for that. I added tests that refer to names bound in the same let-head, although they aren't actually references to that (SpiderMonkey's let is like Scheme's let, not let*). I also added a couple test cases of lets that fail because of rebinding the same name. Since that throws a TypeError (weird) I generalized assertSyntaxError to assertError in the test suite.

Will push in a sec.

Dave
http://hg.mozilla.org/tracemonkey/rev/fb5b64e975c9
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/fb5b64e975c9
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Automatically extracted testcase for this bug was committed:

https://hg.mozilla.org/mozilla-central/rev/efaf8960a929
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.