1071646 - Implement ES6 block-level functions

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Jason Orendorff [:jorendorff]]

In short:

*   Block-level functions do not cause dynamic scoping.
*   The binding is hoisted to the block scope.
*   Creation of the function and initialization of the binding is hoisted to the top of the block.
*   In non-strict mode, there's extra weirdness; see ES6 B.3.3 and B.3.4.

http://www.ecma-international.org/ecma-262/6.0/index.html#sec-block-level-function-declarations-web-legacy-compatibility-semantics

Comment 4

[Boris Zbarsky [:bzbarsky]]

This is starting to break real sites in the wild, not just stackoverflow questions.  Do we have a planned ETA for fixing this?

Comment 5

[Shu-yu Guo [:shu]]

Attachment: Rough WIP for strict-mode lexical function stmts without annex B

Comment 6

[Shu-yu Guo [:shu]]

Attachment: Light refactoring of lexical binding helpers in Parser.

Comment 7

[Shu-yu Guo [:shu]]

Attachment: Make functions block-scoped in JS and implement Annex B semantics for compatibility.

Comment 8

[Jason Orendorff [:jorendorff]]

Comment on attachment 8683984 [details] [diff] [review]
Light refactoring of lexical binding helpers in Parser.

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

All right.

ParseNode::name() should return a Handle<JSAtom*>::fromRootedLocation, so that you don't have to create a temporary Rooted in the second Parser<FullParseHandler>::bindUninitialized() method, but if you don't want to add that right now, I'll do it later.

Comment 9

[Shu-yu Guo [:shu]]

Attachment: Make functions block-scoped in JS and implement Annex B semantics for compatibility.

Fixed a bug in BCE where I wasn't emitting a JSOP_POP for the Annex B assignment.

Comment 10

[Shu-yu Guo [:shu]]

Attachment: Tests.

Comment 11

[Shu-yu Guo [:shu]]

> ParseNode::name() should return a Handle<JSAtom*>::fromRootedLocation, so
> that you don't have to create a temporary Rooted in the second
> Parser<FullParseHandler>::bindUninitialized() method, but if you don't want
> to add that right now, I'll do it later.

I'll wait to make that change.

Comment 12

[Shu-yu Guo [:shu]]

Attachment: Make functions block-scoped in JS and implement Annex B semantics for compatibility.

Also implement Annex B.3.4.

Comment 13

[Jason Orendorff [:jorendorff]]

Comment on attachment 8686870 [details] [diff] [review]
Make functions block-scoped in JS and implement Annex B semantics for compatibility.

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

r=me with comments addressed. In particular, please keep labelled functions working, and check out the weird example that I claim will break your assertion...

The core changes, in Parser.cpp, look quite nice!

Don't forget to bump Xdr.h.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +7707,5 @@
> +            // ensure that we emit the bytecode defining them before the rest
> +            // of code in the block we use a separate pass over
> +            // functions. During the main pass later the emitter will add
> +            // JSOP_NOP with source notes for the function to preserve the
> +            // original functions position when decompiling.

Kill the last sentence here, please.

::: js/src/frontend/NameFunctions.cpp
@@ +785,5 @@
>              break;
>  
> +          case PNK_ANNEXB_BODY:
> +            MOZ_ASSERT(cur->isArity(PN_BINARY));
> +            if (cur->pn_left) {

How can this be null? FullParseHandler::setFunctionAssignmentForAnnexB initializes it.

::: js/src/frontend/ParseNode.cpp
@@ +286,3 @@
>          MOZ_ASSERT(pn->isArity(PN_BINARY));
> +        if (pn->pn_left)
> +            stack->push(pn->pn_left);

There is a comment above that says "Binary nodes with two non-null children."

There is also now a second case immediately below this one that contains the if-statement you added here. You can use that for PNK_ANNEXB_BODY. Or, if you want to merge them all, great, but change the "non-null" comment.

(...But again, how can pn_left be null here anyway?)

::: js/src/frontend/ParseNode.h
@@ +272,4 @@
>   *                          pn_scopecoord: hops and var index for function
>   *                          pn_dflags: PND_* definition/use flags (see below)
>   *                          pn_blockid: block id number
> + * PNK_ANNEXB_BODY binary   pn_left: PNK_ARGSBODY or PNK_LEXICALSCOPE

Mmm. I would have put this outside the PNK_FUNCTION. The child of PNK_FUNCTION should be code that runs when the function is called.

I think you may have done it this way because my way required fiddly workarounds in too many places, but now that you're done, it looks there is going to be some of that either way, right? setFunctionAssignmentForAnnexB, for example: yuck.

I can live with it like this, I guess.

@@ +273,5 @@
>   *                          pn_dflags: PND_* definition/use flags (see below)
>   *                          pn_blockid: block id number
> + * PNK_ANNEXB_BODY binary   pn_left: PNK_ARGSBODY or PNK_LEXICALSCOPE
> + *                          pn_right: assignment use for annex B semantics for
> + *                            block-scoped function

delete "use" here, I think - it's a PNK_ASSIGN, not a name node with isUsed()

@@ +786,5 @@
>          return !isOp(JSOP_LAMBDA) && !isOp(JSOP_LAMBDA_ARROW) && !isOp(JSOP_DEFFUN);
>      }
>  
> +    bool functionIsAnnexB() const {
> +        return functionIsHoisted() && pn_body && pn_body->isKind(PNK_ANNEXB_BODY);

Why is functionIsHoisted() a useful thing to check here? If it's correct either way, please drop `functionIsHoisted() &&`.

::: js/src/frontend/Parser.cpp
@@ +2275,5 @@
> +                    // |with|, because B.3.3 explicitly says to set the binding in
> +                    // the nearest VariableEnvironment.
> +                    //varNode->pn_dflags &= ~PND_DEOPTIMIZED;
> +
> +                    annexDef = varNode->resolve();

Can we assert that varNode is a definition, and therefore this resolve() is a no-op? This is the branch where there's no existing binding to use, so varNode should be promoted to a definition under data.bind(), in ParseContext<FPH>::define(), I think.

@@ +3048,5 @@
>          return null();
>  
> +    // Synthesizing a block for Annex B.3.4 implies that we must've also
> +    // synthesized an assignment for Annex B.3.3.
> +    MOZ_ASSERT_IF(synthesizedBlockForAnnexB, assignmentForAnnexB);

I don't think that's right. What happens in this case:

    function f() {
        let x = 1;
        {
            if (1)
                function x() {}
        }
        return x; // should return 1
    }

There should be a synthesized block but no synthesized assignment, because of the "would not produce any Early Errors" check in B.3.3.1 step 1.a.ii. (Useless code, because there's no way to access the conditionally-declared function... but still)

@@ +3683,5 @@
> +            /*
> +             * Make sure to indicate the need to deoptimize the script's arguments
> +             * object. Mark the function as if it contained a debugger statement,
> +             * which will deoptimize arguments as much as possible.
> +             */

Nit: re-word-wrap the indented comment, optionally change it to //-style.

@@ +4569,5 @@
> +    pn->pn_blockid = pc->blockid();
> +    if (!prepareAndBindInitializedLexicalWithNode(funName, /* isConst = */ false, pn, pos()))
> +        return false;
> +
> +    return true;

Nit: tail-call here.

::: js/src/tests/ecma_5/extensions/strict-function-statements.js
@@ +47,3 @@
>           true);
>  assertEq(testLenientAndStrict("x: function f() { }",
> +                              parseRaisesException(SyntaxError),

I think the pre-existing behavior is correct here, per B.3.2.

::: js/src/tests/ecma_6/LexicalEnvironment/block-scoped-functions-annex-b-property.js
@@ +7,5 @@
> +  function x() { return "fun-x"; }
> +}
> +
> +// Annex B is supposed to be like an assignment. Should not blow away the
> +// existing setter-less getter.

B.3.3.2 step 1.b.i.3 has us bailing out if envRec.CanDeclareGlobalFunction(F) returns false. There are similar cases in function scopes (the "would not produce any Early Errors" language) and in eval scopes. Please test these cases.

(In *this* test, the reason we don't bail out is that that step happens before we reach the Object.defineProperty() call. Subtle enough for a comment maybe?)

::: js/src/tests/ecma_6/LexicalEnvironment/block-scoped-functions-annex-b-with.js
@@ +1,5 @@
> +var o = { f: "string-f" };
> +with (o) {
> +  function f() {
> +    return "fun-f";
> +  }

Please check before entering the with-statement that global.f already exists. It should have been created during GlobalDeclarationInstantiation, with {value: undefined, writable: true, enumerable: true, configurable: false}.

Or in a separate test or whatever.

::: js/src/tests/js1_5/Regress/regress-326453.js
@@ +13,5 @@
>  
>  printBugNumber(BUGNUMBER);
>  printStatus (summary);
>  
> +function f() { with({}) { function g() { }; printStatus(); } }

This one you could just delete...

::: js/src/tests/js1_5/extensions/regress-245795.js
@@ +12,5 @@
>  printBugNumber(BUGNUMBER);
>  printStatus (summary);
>  
> +if (typeof uneval == 'undefined')
> +  reportCompare(0, 0);

This wouldn't end the test; it would just keep running.

We always have uneval() though, so this is just dead code.

@@ -16,5 @@
>  {
> -  function a()
> -  {
> -    b = function() {};
> -  }

Hmm. Why remove the outer block here? We can't handle the direct eval case inside a block yet?

::: js/src/tests/js1_8_5/reflect-parse/declarations.js
@@ +3,5 @@
>  
>  // Bug 632056: constant-folding
>  program([exprStmt(ident("f")),
>           ifStmt(lit(1),
> +                blockStmt([funDecl(ident("f"), [], blockStmt([]))]),

Lame! if the ECMA AST doesn't have a Block, the Reflect.parse output shouldn't have a Block. But we can let it slide in this case.

It looks like apart from this one, we don't have reflect-parse tests for block-level functions. Please add a few.

Comment 14

[Shu-yu Guo [:shu]]

Attachment: Support labelled function declarations in sloppy mode per Annex B.3.2.

The fix for labelled declarations was pretty involved, so additional review would be good.

Comment 15

[Shu-yu Guo [:shu]]

Attachment: Support labelled function declarations in sloppy mode per Annex B.3.2.

Oops, was using popCopy incorrectly.

Comment 16

[Shu-yu Guo [:shu]]

Attachment: Introduce JSOP_BINDVAR to support Annex B.3.3.3.

Comment 17

[Jason Orendorff [:jorendorff]]

Comment on attachment 8689881 [details] [diff] [review]
Support labelled function declarations in sloppy mode per Annex B.3.2.

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

Needs tests.
* SyntaxError in strict mode, with reasonable error message
* works in various contexts:
    * at toplevel
    * at body level in a function
    * in a block at toplevel
    * in a block in a function
    * with multiple labels
    * at body level in a sloppy direct eval
* function is still hoisted
* bizarro B.3.3 double binding still happens
* bizarro B.3.3 assignment still happens
* and see below

::: js/src/frontend/BytecodeEmitter.cpp
@@ +5979,5 @@
> +    if (!sc->strict() && blockScopedFunction) {
> +        StmtInfoBCE* stmt = innermostStmt();
> +        while (stmt && stmt->type == StmtType::LABEL)
> +            stmt = stmt->enclosing;
> +        blockScopedFunction = !!stmt;

Oh, I would totally have missed this if it was me writing this patch. What's the symptom if we screw this up? Test for it?

::: js/src/frontend/Parser.cpp
@@ +2230,5 @@
>          MOZ_ASSERT(assignmentForAnnexBOut);
>          *assignmentForAnnexBOut = nullptr;
>  
> +        // In sloppy mode, Annex B.3.2 allows labeled function
> +        // declarations. Otherwise it is a parse error.

OK, as discussed on IRC, the spec specially bans this (in strict and sloppy mode) where the LabelledStatement is the immediate child of if, else, while, do, for, or with. Please add tests and fix the error message.

@@ +4595,5 @@
>  Parser<FullParseHandler>::bindLexicalFunctionName(HandlePropertyName funName,
>                                                    ParseNode* pn)
>  {
> +    // In sloppy mode temporarily unlink all label statements for ES6 Annex
> +    // B.3.2. This is so terrible.

Yeah - let's not do this.

I'd rather see a boolean parameter added to prepareAndBindInitializedLexicalWithNode()
to have it do this walking-up, finding the enclosing block, without actually modifying the statement stack.

Or the innermostNonLabelStmt() thing we talked about, your call.

Comment 18

[Shu-yu Guo [:shu]]

Attachment: Introduce JSOP_BINDVAR to support Annex B.3.3.3.

Comment 19

[Jason Orendorff [:jorendorff]]

Comment on attachment 8690288 [details] [diff] [review]
Introduce JSOP_BINDVAR to support Annex B.3.3.3.

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

::: js/src/vm/Interpreter.cpp
@@ +2056,5 @@
> +    ReservedRooted<JSObject*> scopeChain(&rootObject0, &REGS.fp()->varObj());
> +    ReservedRooted<PropertyName*> name(&rootName0, script->getName(REGS.pc));
> +    ReservedRooted<JSObject*> scope(&rootObject1);
> +    MOZ_ASSERT(LookupNameWithGlobalDefault(cx, name, scopeChain, &scope));
> +    MOZ_ASSERT(scopeChain == scope);

This assertion is fine for the non-eval case, but I think in the eval case, the function binding is configurable. Supposed to be, at least, whether we implement that I dunno... But if so, the script can just delete it before we get here.

Comment 20

[Shu-yu Guo [:shu]]

Attachment: Make functions block-scoped in JS and implement Annex B semantics for compatibility.

Updated version with PNK_VAR synthesis.

Comment 21

[Jason Orendorff [:jorendorff]]

Comment on attachment 8690298 [details] [diff] [review]
Make functions block-scoped in JS and implement Annex B semantics for compatibility.

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

::: js/src/frontend/Parser.cpp
@@ +2240,5 @@
> +            if (!pc->sc->strict()) {
> +                // Under non-strict mode, try Annex B.3.3 semantics. If making
> +                // an additional 'var' binding of the same name does not throw
> +                // an early error, do so. This 'var' binding would be assigned
> +                // the function object in situ, e.g., when its declaration is

I think this is an i.e. and not an e.g.? But really, just strike "in situ, e.g.," as the rest works fine without it.

@@ +2272,5 @@
> +                    if (!data.bind(funName, this))
> +                        return false;
> +
> +                    MOZ_ASSERT(varNode->isDefn());
> +                    annexDef = static_cast<Definition*>(varNode);

If you wanted, you could add
    bool Definition::test(const ParseNode* p) { return p->isDefn(); }
and here use `varNode->as<Definition>()` instead of a static_cast (and you can drop the assertion, because `as<T>()` asserts.

If you did that, there are a bunch of places still doing C-style casts `(Definition*)` you could change...

Comment 22

[Shu-yu Guo [:shu]]

Attachment: Introduce JSOP_BINDVAR to support Annex B.3.3.3.

Comment 23

[Shu-yu Guo [:shu]]

Attachment: Cast ParseNode to Definition using as<T>.

Note that there is an reinterpret_cast<Definition*> that I did not convert,
because at that point the pn *isn't* a Defn() yet, but will become one later. I
did not feel like refactoring that logic in this patch.

Comment 24

[Shu-yu Guo [:shu]]

Attachment: Support JSOP_BINDVAR in Baseline.

Comment 25

[Shu-yu Guo [:shu]]

Attachment: Support JSOP_BINDVAR in Ion.

Comment 26

[Shu-yu Guo [:shu]]

Attachment: Support labelled function declarations in sloppy mode per Annex B.3.2.

Carrying r=jorendorff and addressed comments + added tests.

Comment 27

[Jan de Mooij [:jandem]]

Comment on attachment 8692220 [details] [diff] [review]
Support JSOP_BINDVAR in Baseline.

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

Nice refactoring in VMFunctions.cpp

Comment 28

[Jan de Mooij [:jandem]]

Comment on attachment 8692221 [details] [diff] [review]
Support JSOP_BINDVAR in Ion.

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

::: js/src/jit/MIR.h
@@ +10688,5 @@
>          return pc_;
>      }
>  };
>  
> +class MCallBindVar

This instruction should be pretty uncommon right? Else we might want to implement congruentTo and maybe getAliasSet, and call setMovable() in the constructor.

@@ +10692,5 @@
> +class MCallBindVar
> +  : public MUnaryInstruction,
> +    public SingleObjectPolicy::Data
> +{
> +    MCallBindVar(MDefinition* scopeChain)

Nit: make this `explicit`.

Comment 29

[Jan de Mooij [:jandem]]

Thanks for fixing this Shu! Looks like it will fix a number of websites and get rid of a footgun.

Comment 30

[Jason Orendorff [:jorendorff]]

Comment on attachment 8692218 [details] [diff] [review]
Introduce JSOP_BINDVAR to support Annex B.3.3.3.

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

that was easy

Comment 31

[Jason Orendorff [:jorendorff]]

Comment on attachment 8692219 [details] [diff] [review]
Cast ParseNode to Definition using as<T>.

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

Thanks.

::: js/src/frontend/Parser.cpp
@@ +348,5 @@
>  {
>      Definition* oldDecl = decls_.lookupFirst(atom);
>  
>      pn->setDefn(true);
> +    Definition* newDecl = &pn->template as<Definition>();

I'm surprised this is necessary, since it's a specialization, but ok.

Comment 32

[Jeff Walden [:Waldo]]

(In reply to Jason Orendorff [:jorendorff] from comment #31)
> > +    Definition* newDecl = &pn->template as<Definition>();
> 
> I'm surprised this is necessary, since it's a specialization, but ok.

The problem is with a little more context, we see:

 template <typename ParseHandler>
 void
 ParseContext<ParseHandler>::updateDecl(TokenStream& ts, JSAtom* atom, Node pn)
 {
     Definition* oldDecl = decls_.lookupFirst(atom);
 
     pn->setDefn(true);
-    Definition* newDecl = (Definition*)pn;
+    Definition* newDecl = &pn->template as<Definition>();

|pn| is of type Node, and the Node type is dependent upon the type passed for ParseHandler.  So when parsing we don't know if Node::as is a field or a template function.  And therefore we don't know if the < before Definition is a template bracket or a less-than.  C++ arbitrarily decided that without |template|, a field is assumed, so |template| is necessary here to opt out of that.

Comment 33

[Jason Orendorff [:jorendorff]]

For some reason after reading that code about six times I still thought it was a specialization to template<> void ParseContext<FullParseHandler>::updateDecl(...).

Maybe because this template wouldn't instantiate with ParseHandler=SyntaxParseHandler anyway. I dunno. Sorry.

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/119258fe0dd6
https://hg.mozilla.org/integration/mozilla-inbound/rev/73c94ff300b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b4fe5acd50c
https://hg.mozilla.org/integration/mozilla-inbound/rev/a44b841c33fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/52ad4194c0b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/23e4475338c5
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffd21df83fee

Comment 35

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/119258fe0dd6
https://hg.mozilla.org/mozilla-central/rev/73c94ff300b2
https://hg.mozilla.org/mozilla-central/rev/5b4fe5acd50c
https://hg.mozilla.org/mozilla-central/rev/a44b841c33fb
https://hg.mozilla.org/mozilla-central/rev/52ad4194c0b3
https://hg.mozilla.org/mozilla-central/rev/23e4475338c5
https://hg.mozilla.org/mozilla-central/rev/ffd21df83fee

Comment 36

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2bec6ed7b30

Comment 37

[Guilherme Lima]

https://hg.mozilla.org/mozilla-central/rev/d2bec6ed7b30 - landed yesterday

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd6226b5ea82
https://hg.mozilla.org/integration/mozilla-inbound/rev/001519eae1c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/204aeab8d5f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/2335624d41d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/633c7b5eeb2a
https://hg.mozilla.org/integration/mozilla-inbound/rev/6da48aaf41b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/b3066c2d2d23

Comment 39

[Shu-yu Guo [:shu]]

Relanded in light of https://github.com/tc39/ecma262/issues/162#issuecomment-165885491

I tested a local build, and I was able to log in to Inbox and get a list of emails.

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cd6226b5ea82
https://hg.mozilla.org/mozilla-central/rev/001519eae1c8
https://hg.mozilla.org/mozilla-central/rev/204aeab8d5f8
https://hg.mozilla.org/mozilla-central/rev/2335624d41d7
https://hg.mozilla.org/mozilla-central/rev/633c7b5eeb2a
https://hg.mozilla.org/mozilla-central/rev/6da48aaf41b3
https://hg.mozilla.org/mozilla-central/rev/b3066c2d2d23

Comment 41

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39d9a1b177b7

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/39d9a1b177b7

Comment 43

[James Willcox (:snorp) ([e-mail]) (he/him)]

Inbox is broken again on Nightly for me, presumably due to these patches.

Comment 44

[Tom S [:evilpie]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #43)
> Inbox is broken again on Nightly for me, presumably due to these patches.

In https://github.com/tc39/ecma262/issues/162#issuecomment-165885491 a Google dev claims that Inbox should be compatible with the new semantics. Either we or they made some mistake.

Comment 45

[Shu-yu Guo [:shu]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #43)
> Inbox is broken again on Nightly for me, presumably due to these patches.

In what way is it broken? I can load Inbox and click on messages on the 12-28 Nightly, which is after these patches were relanded.

Comment 46

[James Willcox (:snorp) ([e-mail]) (he/him)]

Inbox is working now. Maybe I was getting cached stuff.

Comment 47

[Jan de Mooij [:jandem]]

Updating target milestone to avoid confusion; this was backed out (bug 1231568) and relanded in 46.

Comment 48

[Florian Scholz (Open Web Docs)]

(Sorry, meant to have a look at this while memory of everyone here was fresh)

This ships soon and has been added to https://developer.mozilla.org/en-US/Firefox/Releases/46#JavaScript

There is a section about this in the reference https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Functions#Block-level_functions but the length of this bug, the spec changes, the compat implications, the strict vs. sloppy mode behavior, and the non-standard things we used to do, are all making this quite difficult to digest. If anyone has some insights here, MDN would welcome your contribution.