1163851 - Use InHandling to implement restrictions on |in| expressions in the first part of a for(;;) loop

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Following up on all the infrastructure landed in bug 1155472.  This is approximately a five-liner, modulo ripping out all the manual forparsingForInit maintenance code.

This is not a null change from a functionality point of view.  At an absolute minimum, it's expected that this should be the case:

js> for (x => x in true; false; ) break;
typein:3:4 SyntaxError: invalid for/in left-hand side:
typein:3:4 for (x => x in true; false; ) break;
typein:3:4 ....^

But it's not in the current parsingForInit system.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Pass the correct |InHandling| argument when parsing the initializer in a declaration in a for-loop head

No functionality change, but without this, later patches in the sequence would be buggy.  Also a test for something I misread in the spec grammar.  Stupid parser that makes it hard to straightforwardly implement the grammar.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Remove remaining tests of |pc->parsingForInit|

Alternative to InForInit/NotInForInit I could pass an InHandling argument, but I'd somewhat prefer to leave that for methods that correspond to *actual* grammar terms and such.  There's not a single grammar term Parser::variables corresponds to, so I'd prefer something sui generis.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Use the InHandling parameter in the parser to determine when to allow |in|

Comment 4

[Jeff Walden [:Waldo]]

Attachment: |pc->parsingForInit| is dead. Long live |InHandling|!

Comment 5

[Eric Faust [:efaust]]

Comment on attachment 8605661 [details] [diff] [review]
Remove remaining tests of |pc->parsingForInit|

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

::: js/src/frontend/Parser.cpp
@@ +3835,5 @@
>  template <typename ParseHandler>
>  typename ParseHandler::Node
>  Parser<ParseHandler>::variables(YieldHandling yieldHandling,
> +                                ParseNodeKind kind,
> +                                ForInitLocation location,

not that we should have many more callers in the future, but it would be nice to have these default in some nice way. This is more a comment about all of these enums as a whole, rather than this specific instance. Feel free to ignore, but perhaps food for thought. InForInit is very specific.

Comment 6

[Eric Faust [:efaust]]

Comment on attachment 8605662 [details] [diff] [review]
Use the InHandling parameter in the parser to determine when to allow |in|

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

Looks great!

Comment 7

[Eric Faust [:efaust]]

Comment on attachment 8605663 [details] [diff] [review]
|pc->parsingForInit| is dead.  Long live |InHandling|!

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

:)

::: js/src/frontend/Parser.cpp
@@ +4850,5 @@
>      // 'for (var v = expr1 in expr2) stmt'.
>      ParseNode* hoistedVar = nullptr;
>  
> +    // If there's an |in| keyword here, it's a for-in loop, by dint of careful
> +    // parsing of |pn1|.

I wish this were somehow assertable. It isn't, clearly, but it all seems "very cleverly constructed"

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5de5618316a8
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c0564d37591
https://hg.mozilla.org/integration/mozilla-inbound/rev/64722e4b3310
https://hg.mozilla.org/integration/mozilla-inbound/rev/77322b0deed3

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5de5618316a8
https://hg.mozilla.org/mozilla-central/rev/8c0564d37591
https://hg.mozilla.org/mozilla-central/rev/64722e4b3310
https://hg.mozilla.org/mozilla-central/rev/77322b0deed3