Another bit of code that heavily depends on arity to rewrite.

Initial mega-switch, you know the jazz.

s/jazz/drill/, Miss Malaprop.

At some point here my care in splitting these sensibly disappeared, so here begins undistinguished grab-bags of changes until everything's transitioned.

Looks like I split out binary stuff explicitly for some reason. Okay.

And again for weird sensible-splitting. I think this one derives from my initially considering PNK_FUNCTION to be effectful and that breaking (!) functionality. No idea why functions *must* be considered non-effectful for correctness, but there it is.

You are not expected to understand this comment. (I don't, either!) Once we start having multiple kinds of parse node kind, maybe this can be made more sensible as to why. I'm not even sure I believe it, anyway -- function statements *should* have effect inside eval code (where such introduce new bindings, I think). How anything at all works right now, I dunno. (Thus was it ever.)

I guess N === 7, then. And, death to the old arity-testing code!

Updated for the splitting of PNK_DELETE into PNK_DELETE{NAME,PROP,SUPERPROP,ELEM,SUPERELEM,EXPR} in bug 1169171.

Comment on attachment 8610668 [details] [diff] [review] Handle more nodes by kind in BytecodeEmitter::checkSideEffects, 1/N Review of attachment 8610668 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.cpp @@ +1926,5 @@ > MOZ_ASSERT(pn->isArity(PN_NULLARY)); > *answer = true; > return true; > > + // Watch out for getters! Oo, good catch.

Comment on attachment 8610669 [details] [diff] [review] Handle more nodes by kind when checking for side effects, 2/N Review of attachment 8610669 [details] [diff] [review]: ----------------------------------------------------------------- Looks okay to me, but I'd like answers to the questions below. ::: js/src/frontend/BytecodeEmitter.cpp @@ +1922,5 @@ > *answer = false; > return true; > > + case PNK_BREAK: > + case PNK_CONTINUE: Why are break and continue considered effectful? @@ +2047,5 @@ > + // Every part of a loop might be effect-free, but looping infinitely *is* > + // an effect. (Language lawyer trivia: C++ says threads can be assumed > + // to exit or have side effects, C++14 [intro.multithread]p27, so a C++ > + // implementation's equivalent of the below could set |*answer = false;| > + // if all loop sub-nodes set |*answer = false|!) I'm not sure if it's in the spirit of JS to say that non-termination is a side effect. I mean, hell, it's not even considered a side effect in Haskell.

Comment on attachment 8610670 [details] [diff] [review] Check various binary operators for side effects, by node kind Review of attachment 8610670 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.cpp @@ +2025,5 @@ > + } > + return true; > + > + // Most other binary operations (parsed as lists in SpiderMonkey) may > + // perform conversions triggering side effects. How can it trigger conversions? valueOf? If so, please document it as such.

Comment on attachment 8610671 [details] [diff] [review] Handle more nodes by kind when checking for side effects, 3/N Review of attachment 8610671 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.cpp @@ +2121,5 @@ > + MOZ_ASSERT(pn->isArity(PN_TERNARY)); > + if (!checkSideEffects(pn->pn_kid1, answer)) > + return false; > + if (*answer) > + return true; Do you need manual short circuiting here and below? Worried about an extra call? The top of checkSideEffects is already if (!pn || *answer) return true;

Comment on attachment 8610674 [details] [diff] [review] Handle try/catch by kind when checking for side effects Review of attachment 8610674 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.cpp @@ +2164,5 @@ > + MOZ_ASSERT(pn->isArity(PN_TERNARY)); > + if (!checkSideEffects(pn->pn_kid1, answer)) > + return false; > + if (*answer) > + return true; Ditto question about short circuiting here as my previous review. If you're ensuring caller short-circuiting everywhere instead of callee short-circuiting, could you remove the short-circuiting logic at the top of checkSideEffects?

Comment on attachment 8610682 [details] [diff] [review] Handle functions by kind when checking for side effects Review of attachment 8610682 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.cpp @@ +2189,5 @@ > > + case PNK_FUNCTION: > + MOZ_ASSERT(pn->isArity(PN_CODE)); > + /* > + * A named function, contrary to ES3, is no longer useful, because we Nit: effectful @@ +2193,5 @@ > + * A named function, contrary to ES3, is no longer useful, because we > + * bind its name lexically (using JSOP_CALLEE) instead of creating an > + * Object instance and binding a readonly, permanent property in it > + * (the object and binding can be detected and hijacked or captured). > + * This is a bug fix to ES3; it is fixed in ES3.1 drafts. That's wack.

Comment on attachment 8610683 [details] [diff] [review] Handle more nodes by kind when checking for side effects, 6/N Review of attachment 8610683 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.cpp @@ +2189,5 @@ > *answer = true; > return true; > > + // Shorthands could trigger getters. > + case PNK_SHORTHAND: What is shorthand again?

Comment on attachment 8610685 [details] [diff] [review] Remove dead code for checking whether a parse tree node has side effects Review of attachment 8610685 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.cpp @@ -1901,5 @@ > { > JS_CHECK_RECURSION(cx, return false); > > - if (!pn || *answer) > - return true; Oh, I see you did remove the short circuiting at the top. Ignore my previous short-circuiting comments.

I think it would be good to see the feasibility of removing checkSideEffects entirely. The frontend shouldn't be doing this optimization now that we have 2 tiers of JITs, and it makes debugging possibly confusing.

(In reply to Shu-yu Guo [:shu] from comment #17) > ::: js/src/frontend/BytecodeEmitter.cpp > @@ +1922,5 @@ > > *answer = false; > > return true; > > > > + case PNK_BREAK: > > + case PNK_CONTINUE: > > Why are break and continue considered effectful? "side effect" isn't quite the right name for what this is checking. It's sort of more whether the node in question might do something that would change program semantics if you removed it. But that's not quite right, either, because in |({ x: null })| the PNK_COLON node for |x| affects semantics if it's removed, but as its effect is only upon a node that we consider "not to have side effects", that's fine. What should this be called? Beats me. I tried for a bit and came up blank, so I punted. It's hard even to *describe* it for a comment, either. :-( Given that meaning, eliminating a break/continue could cause code in a loop (more precisely, code in a statement list as loop body) to fail to exit/resume at start appropriately. And this matters even outside loops, because in very weird cases you can "break" in a named statement that's not a loop. (Or maybe "continue", I can't remember.) So these are "effectful". It's insane. ECMAScript is insane. Recognition code 927, I am a potato. > @@ +2047,5 @@ > > + // Every part of a loop might be effect-free, but looping infinitely *is* > > + // an effect. (Language lawyer trivia: C++ says threads can be assumed > > + // to exit or have side effects, C++14 [intro.multithread]p27, so a C++ > > + // implementation's equivalent of the below could set |*answer = false;| > > + // if all loop sub-nodes set |*answer = false|!) > > I'm not sure if it's in the spirit of JS to say that non-termination is a > side effect. I mean, hell, it's not even considered a side effect in Haskell. Not a "side effect", but iloops are supposed to happen because the spec says they do, and they're visible to code that tries to do something after an iloop. Therefore they're effects in the sense of this method.

Comment on attachment 8610669 [details] [diff] [review] Handle more nodes by kind when checking for side effects, 2/N See comment 25.

Comment on attachment 8610669 [details] [diff] [review] Handle more nodes by kind when checking for side effects, 2/N Review of attachment 8610669 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.cpp @@ +1922,5 @@ > *answer = false; > return true; > > + case PNK_BREAK: > + case PNK_CONTINUE: Why are break and continue considered effectful? @@ +2047,5 @@ > + // Every part of a loop might be effect-free, but looping infinitely *is* > + // an effect. (Language lawyer trivia: C++ says threads can be assumed > + // to exit or have side effects, C++14 [intro.multithread]p27, so a C++ > + // implementation's equivalent of the below could set |*answer = false;| > + // if all loop sub-nodes set |*answer = false|!) I'm not sure if it's in the spirit of JS to say that non-termination is a side effect. I mean, hell, it's not even considered a side effect in Haskell.

https://hg.mozilla.org/integration/mozilla-inbound/rev/928b82c7e3f2 https://hg.mozilla.org/integration/mozilla-inbound/rev/d3dadbf59a1a https://hg.mozilla.org/integration/mozilla-inbound/rev/a0f2b05a4a5f https://hg.mozilla.org/integration/mozilla-inbound/rev/6d74cf23229b https://hg.mozilla.org/integration/mozilla-inbound/rev/e3f6340e78a7 https://hg.mozilla.org/integration/mozilla-inbound/rev/4de3e7d764dc https://hg.mozilla.org/integration/mozilla-inbound/rev/1e35358eb840 https://hg.mozilla.org/integration/mozilla-inbound/rev/5baf4bf01aa2 https://hg.mozilla.org/integration/mozilla-inbound/rev/6d8aa39b6518 https://hg.mozilla.org/integration/mozilla-inbound/rev/2c3682b27301 https://hg.mozilla.org/integration/mozilla-inbound/rev/4461be1cd2cd https://hg.mozilla.org/integration/mozilla-inbound/rev/22ee40559aa3 https://hg.mozilla.org/integration/mozilla-inbound/rev/716036bb7ff0 https://hg.mozilla.org/integration/mozilla-inbound/rev/b69363c468e1

(In reply to Shu-yu Guo [:shu] from comment #22) > What is shorthand again? var x = 5; var obj = { x }; // as if { x: x } Added a reminder comment with a little elaboration.

https://hg.mozilla.org/mozilla-central/rev/928b82c7e3f2 https://hg.mozilla.org/mozilla-central/rev/d3dadbf59a1a https://hg.mozilla.org/mozilla-central/rev/a0f2b05a4a5f https://hg.mozilla.org/mozilla-central/rev/6d74cf23229b https://hg.mozilla.org/mozilla-central/rev/e3f6340e78a7 https://hg.mozilla.org/mozilla-central/rev/4de3e7d764dc https://hg.mozilla.org/mozilla-central/rev/1e35358eb840 https://hg.mozilla.org/mozilla-central/rev/5baf4bf01aa2 https://hg.mozilla.org/mozilla-central/rev/6d8aa39b6518 https://hg.mozilla.org/mozilla-central/rev/2c3682b27301 https://hg.mozilla.org/mozilla-central/rev/4461be1cd2cd https://hg.mozilla.org/mozilla-central/rev/22ee40559aa3 https://hg.mozilla.org/mozilla-central/rev/716036bb7ff0 https://hg.mozilla.org/mozilla-central/rev/b69363c468e1