Implement ES6 block-level functions

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: jorendorff, Unassigned)

Tracking

(Blocks: 2 bugs, {dev-doc-complete})

unspecified
mozilla46
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(8 attachments, 8 obsolete attachments)

6.64 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
71.29 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
76.14 KB, patch
Details | Diff | Splinter Review
11.28 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
8.05 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
6.35 KB, patch
jandem
: review+
Details | Diff | Splinter Review
13.20 KB, patch
jandem
: review+
Details | Diff | Splinter Review
35.96 KB, patch
shu
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
Keywords: dev-doc-needed
OS: Linux → All
Hardware: x86_64 → All
See Also: → bug 585536
Whiteboard: [DocArea=JS]
See Also: → bug 793975, bug 861006
(Reporter)

Comment 1

4 years ago
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
Summary: Implement ES6 B.3.3 Block-Level Function Declarations Web Legacy Compatibility Semantics → Implement ES6 block-level functions
(Reporter)

Updated

3 years ago
Duplicate of this bug: 1217837
(Reporter)

Updated

3 years ago
Duplicate of this bug: 585536
This is starting to break real sites in the wild, not just stackoverflow questions.  Do we have a planned ETA for fixing this?
Flags: needinfo?(jorendorff)

Updated

3 years ago
Attachment #8682863 - Attachment is obsolete: true
(Reporter)

Comment 8

3 years ago
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.
Attachment #8683984 - Flags: review?(jorendorff) → review+

Comment 9

3 years ago
Fixed a bug in BCE where I wasn't emitting a JSOP_POP for the Annex B assignment.
Attachment #8683985 - Attachment is obsolete: true
Attachment #8683985 - Flags: review?(jorendorff)
Attachment #8685738 - Flags: review?(jorendorff)
Posted patch Tests. (obsolete) — Splinter Review
Attachment #8685739 - Flags: review?(jorendorff)
> 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.

Updated

3 years ago
Attachment #8685738 - Attachment is obsolete: true
Attachment #8685739 - Attachment is obsolete: true
Attachment #8685738 - Flags: review?(jorendorff)
Attachment #8685739 - Flags: review?(jorendorff)
(Reporter)

Comment 13

3 years ago
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.
Attachment #8686870 - Flags: review?(jorendorff) → review+
The fix for labelled declarations was pretty involved, so additional review would be good.
Attachment #8689861 - Flags: review?(jorendorff)
Oops, was using popCopy incorrectly.

Updated

3 years ago
Attachment #8689861 - Attachment is obsolete: true
Attachment #8689861 - Flags: review?(jorendorff)
Attachment #8689883 - Flags: review?(jorendorff)

Updated

3 years ago
Attachment #8689881 - Flags: review?(jorendorff)
(Reporter)

Comment 17

3 years ago
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.
Attachment #8689881 - Flags: review?(jorendorff) → review+
Attachment #8690288 - Flags: review?(jorendorff)

Updated

3 years ago
Attachment #8689883 - Attachment is obsolete: true
Attachment #8689883 - Flags: review?(jorendorff)
(Reporter)

Comment 19

3 years ago
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.
(Reporter)

Comment 21

3 years ago
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...

Updated

3 years ago
Attachment #8690288 - Attachment is obsolete: true
Attachment #8690288 - Flags: review?(jorendorff)
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.
Attachment #8692219 - Flags: review?(jorendorff)
Attachment #8692221 - Flags: review?(jdemooij)
Carrying r=jorendorff and addressed comments + added tests.
Attachment #8692222 - Flags: review+

Updated

3 years ago
Attachment #8689881 - Attachment is obsolete: true
Comment on attachment 8692220 [details] [diff] [review]
Support JSOP_BINDVAR in Baseline.

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

Nice refactoring in VMFunctions.cpp
Attachment #8692220 - Flags: review?(jdemooij) → review+
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`.
Attachment #8692221 - Flags: review?(jdemooij) → review+
Thanks for fixing this Shu! Looks like it will fix a number of websites and get rid of a footgun.
(Reporter)

Comment 30

3 years ago
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
Attachment #8692218 - Flags: review?(jorendorff) → review+
(Reporter)

Comment 31

3 years ago
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.
Attachment #8692219 - Flags: review?(jorendorff) → review+
(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.
(Reporter)

Comment 33

3 years ago
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.
Flags: needinfo?(jorendorff)

Updated

3 years ago
Depends on: 1231568
Depends on: 1231758

Updated

3 years ago
Depends on: 1232328

Comment 37

3 years ago
https://hg.mozilla.org/mozilla-central/rev/d2bec6ed7b30 - landed yesterday
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Depends on: 1234717
Depends on: 1235477
Inbox is broken again on Nightly for me, presumably due to these patches.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Inbox is working now. Maybe I was getting cached stuff.
Depends on: 1236875
Updating target milestone to avoid confusion; this was backed out (bug 1231568) and relanded in 46.
Target Milestone: mozilla45 → mozilla46
status-firefox45: fixed → ---

Updated

3 years ago
Depends on: 1245126
Depends on: 1246827
(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.
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1304641

Updated

2 years ago
Duplicate of this bug: 1315272
Depends on: 1357070
Duplicate of this bug: 1071877
You need to log in before you can comment on or make changes to this bug.