888002 - Constant-folding should not affect the meaning of delete-expressions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

Function("var e = delete(false ? e : e)")

asserts js debug shell on m-c changeset 3b955f306226 without any CLI arguments at Assertion failure: ((js_CodeSpec[op].format) & 0x001f) == 2, at frontend/BytecodeEmitter.cpp

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/c39ede0eea8f
user:        Jason Orendorff
date:        Tue Jun 25 17:40:00 2013 -0500
summary:     Bug 844805, part 3 - Remove two calls to FoldConstants from the parser. r=Waldo.

Comment 2

[Jason Orendorff [:jorendorff]]

Yikes. There's a pre-existing bug here that was being masked by the constant folding I removed.

Comment 3

[Jason Orendorff [:jorendorff]]

Attachment: v1

Try running the tests without applying the rest of the patch to see how we were screwing up before by doing constant folding on delete expressions.

The patch causes us, in an expression like `delete x && y`, to apply constant-folding optimizations to `x` and `y` individually, but not to `x && y` since that isn't sound in all cases.

Skipping constant-folding on delete expressions altogether would be easier, but it seemed unwise to let users switch to a less-optimized, minimally-tested variant of the bytecode just by wrapping some code in
`delete (function () { _ }())`.

Comment 4

[Jason Orendorff [:jorendorff]]

In case that wasn't clear -- I'm fixing two bugs here:

* Constant folding shouldn't affect the meaning of a delete (or anything else);

* Constant folding shouldn't even be happening during parsing here, grrr.

What does that have to do with the assertion? Well, my changes in bug 844805 triggered this assertion because we ended up in the emitter with `delete x`, and `x` was bound to a local variable. The emitter was surprised to see that, because when the parser sees `delete x` it deoptimizes the whole scope. But when it sees `delete (1 ? x : x)`, it doesn't.

So, yeah, the emitter depends on the parser to deoptimize on `delete NAME`. I decided that's OK. The emitter isn't to blame: that constant-folding is clearly wrong.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 769684 [details] [diff] [review]
v1

Review of attachment 769684 [details] [diff] [review]:
-----------------------------------------------------------------

These syntax "requirements" for |delete| are a disaster.

This does seem the right way to do it to me, as well, wrt whether the bug is in the parser or emitter.  Of course, moving further down the road, we should be able to recognize these bogus deletion syntaxes and constant-fold them to true, but I think that has to follow getting rid of arity.  Definitely not here in any case.

Please add a test that |delete (0 || x)| doesn't fold to |delete x| while you're here.  Also that |delete(x);| is a syntax error if we're considering ES5 rules (I think), or not a syntax error if we're considering ES6 draft rules.  (Parentheses don't convert a Reference into a value, so the |x| there is still a Reference, and per ES5 it'd retain its strictness.  ES6 makes it an error *only* if the sub-expression is an Identifier.)

::: js/src/frontend/FoldConstants.cpp
@@ +232,5 @@
> +    // pn is the operand of the 'delete' keyword.
> +    SC_Delete,
> +
> +    // Any other syntactic context.
> +    SC_Other

SC_Foo naming is a bit weird, and I can't remember ever seeing it elsewhere.  ConditionContext, DeleteContext, OtherContext would be better, to me.

MOZ_{BEGIN,END}_ENUM_CLASS from TypedEnum.h would also be better, in terms of giving you SyntacticContext::Condition, SyntacticContext::Delete, etc.

@@ +390,5 @@
>      }
>  
> +    // The immediate child of a PNK_DELETE node should not be replaced
> +    // with node indicating a different syntactic form; |delete x| is not
> +    // the same as |delete (true && x)|. See bug 888002.

Might be worth adding "(as distinguished from any children of that child, e.g. |x| and |y| in |delete (x || y)|, which are replaced above)" to make clear exactly what happens: kid's not constant-folded, but its kids are.  Whenever we get to removing arity, this feels like a nice comment to have around to keep whoever writes that patch on the straight and narrow -- and as an explanation when all code constant-folding delete stuff is in a single place.

::: js/src/frontend/FoldConstants.h
@@ +7,5 @@
>  #ifndef frontend_FoldConstants_h
>  #define frontend_FoldConstants_h
>  
>  #include "jsprvtd.h"
> +#include "SyntaxParseHandler.h"

Needs a blank line before it and frontend/ in it.

::: js/src/frontend/SyntaxParseHandler.h
@@ +7,5 @@
>  #ifndef frontend_SyntaxParseHandler_h
>  #define frontend_SyntaxParseHandler_h
>  
> +#include "ParseNode.h"
> +#include "TokenStream.h"

I think these are supposed to include frontend/ now.

Comment 6

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> These syntax "requirements" for |delete| are a disaster.

At the risk of misusing bugzilla for what should be water-cooler chat...

I think this misplaces blame. The fact that the syntax of a delete operand is meaningful is just like how the syntax of an assignment lhs or inc/dec operand is meaningful. In other words, this is just a consequence of lvalues and rvalues sharing syntax--which is age-old (JS inherits it from C).

The only reason we don't have exactly the same bugs in assignment and inc/dec is that in those cases the non-Reference-producing forms are banned in the parser:

    (1 || a) = 1; // compile-time ReferenceError
    (1 ? a : b)++;  // SyntaxError

so we never get to constant-folding, much less to runtime, when a constant-folding bug would become observable.

Of course these non-lvalue expressions are every bit as meaningless when used as delete operands, and ought to be every bit as banned there. That ES doesn't ban them is the root cause of this bug.

> This does seem the right way to do it to me, as well, wrt whether the bug is
> in the parser or emitter.  Of course, moving further down the road, we
> should be able to recognize these bogus deletion syntaxes and constant-fold
> them to true [...]

I've considered a few ways to improve this. One would be: in the parser, split out ParseNodeKinds depending on the operand syntax. So we'd have:

    PNK_{DELETE,ASSIGN,$(op)ASSIGN,INC,DEC}_{NAME,PROP,ELEM}
    PNK_DESTRUCTURING_ASSIGN
    PNK_DELETE_RVALUE

This way each PNK actually corresponds to a semantically distinct thing, so the resulting emitter and constant-folding code might not need any special cases.

I dunno. It might not be worth it.

Anyway I don't think arity is going away entirely (though it'll become a function of PNK "soon"). Some AST users, including FoldConstants, want to be able to walk the tree very generically. That is, without special-casing *every* parse node kind.

> [...] Also that |delete(x);| is a syntax error if we're considering
> ES5 rules (I think), or not a syntax error if we're considering ES6 draft
> rules.  (Parentheses don't convert a Reference into a value, so the |x|
> there is still a Reference, and per ES5 it'd retain its strictness.  ES6
> makes it an error *only* if the sub-expression is an Identifier.)

Hmm. I don't think the intent is to change behavior here.

Filed https://bugs.ecmascript.org/show_bug.cgi?id=1577 to try to get that clarified.

Thanks for the review!

Comment 7

[Allen Wirfs-Brock]

The ES5 spec. language had two intents:
  1) In strict mode, "variable bindings" cannot be removed by the delete operator.
  2) It is an early syntax error for a delete operator to directly do so.

The crux this bug is how to interpret the second point and the ES5 spec didn't offer much help. The operand of a delete operator can be an arbitrarily complex expression but it is easy to statically determine whether it will evaluate to an Environment Record reference. However, ES5 wasn't explicit about identifying those cases. So, the ES6 spec. tries to exactly specify  which cases produce early errors.  For delete, it is a delete of a direct reference to an identifier:
   delete foo
or an identifier surround by any number of matched parentheses:
   delete ((((foo))))

No other expression forms, including things like:
   delete (null, foo)
or 
   delete (true ? foo : bar)
produce early errors.  They don't even evaluate to References so the ES5 rules in step 5 don't apply.  Instead, according to step 2 they should cause delete to evaluate to  true.  It would be perfectly legal to constant fold them into that.

Comment 8

[Jason Orendorff [:jorendorff]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #5)
> Please add a test that |delete (0 || x)| doesn't fold to |delete x| while
> you're here.

Done.

> Also that |delete(x);| is a syntax error if we're considering
> ES5 rules (I think), [...]

tests/ecma_5/strict/11.4.1.js covers it.

> MOZ_{BEGIN,END}_ENUM_CLASS from TypedEnum.h would also be better, in terms
> of giving you SyntacticContext::Condition, SyntacticContext::Delete, etc.

Done.

> > +    // The immediate child of a PNK_DELETE node should not be replaced
> > +    // with node indicating a different syntactic form; |delete x| is not
> > +    // the same as |delete (true && x)|. See bug 888002.
> 
> Might be worth adding "(as distinguished from any children of that child,
> e.g. |x| and |y| in |delete (x || y)|, which are replaced above)" to make
> clear exactly what happens:

Added:

    // pn is the immediate child in question. Its descendents were already
    // constant-folded above, so we're done.

> I think these are supposed to include frontend/ now.

Yep. Fixed.

Comment 9

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision c7e005de1329).

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ce515a6cc0c5

Comment 11

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This was previously landed on inbound in:

http://hg.mozilla.org/integration/mozilla-inbound/rev/ce515a6cc0c5