722121 - Remove CheckRedeclaration

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

CheckRedeclaration has no obvious parallel in the spec, and it confuses much more than it helps.  There are very few uses of it, all of which can be addressed without too much trouble.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Remove JSOP_DEF{VAR,CONST} uses

No functional change, just more closely paralleling the spec, and getting rid of the confusing CheckRedeclaration call that doesn't correspond to spec steps.

Note that the algorithm isn't in a published spec PDF that I can find.  <https://mail.mozilla.org/pipermail/es5-discuss/2011-January/003882.html> has a copy of it, and the most recent ES6 draft spec (at least) has it as well, but ES5.1, say, doesn't appear to.  :-\

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Remove JSOP_DEFFUN_FC

According to SemanticAnalysis.cpp and BytecodeEmitter.cpp, we can only generate JSOP_DEFFUN_FC if we have an optimizable function (not heavyweight, not in eval, etc.).  Then, we can only have it if the function has upvars, and those upvars are flattenable.  Given that JSOP_DEFFUN is only for global functions, the upvars would have to be global variables.  We couldn't guarantee flattenability for vars, because they could be overwritten by a previous or subsequent script.  We almost could for consts, but we don't.  And there would be trouble there if the function in question were called before the const's initializer were reached.  So I don't think it's possible to generate this opcode, and it should be removed.

No JS tests or jit tests hit any of these new assertions, and some would fail if we were in a situation where JSOP_DEFFUN_FC would have been generated.

Just to be clear, assuming this gets an r+, I have no plans to land it before the merge train departs for aurora.

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Remove CheckRedeclaration and its last use in JSOP_GETTER/JSOP_SETTER

There can be no conflict defining a getter or setter in an object literal, because literal syntax forbids conflicts:

http://whereswalden.com/2011/01/09/new-es5-requirement-getters-and-setters-in-object-literals-must-not-conflict-with-each-other-or-with-data-properties-even-outside-strict-mode/

There can be no worry about the definition failing due to a readonly or permanent property because such a property can't exist on an object literal at execution time.  Until the object is exposed to script, it's filled only with writable, configurable data properties.  And now that sharps are gone, the object isn't exposed until after the entire literal is evaluated, and getters and setters would have been defined.

Comment 4

[Jason Orendorff [:jorendorff]]

Comment on attachment 592465 [details] [diff] [review]
Remove JSOP_DEF{VAR,CONST} uses

This looks like the same code twice, except for THROW(). If that's the case, would you mind commoning up the code in an inline method called from two places? stubs::DefVarOrConst could just read

    if (!js::DefVarOrConstOperation(f.cx, *f.pc(), dn, f.fp()))
        THROW();

You are right, CheckRedeclaration does make this harder to follow than it should be.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 592468 [details] [diff] [review]
Remove JSOP_DEFFUN_FC

>+                MOZ_ASSERT(fn->getOp() != JSOP_DEFFUN,
>+                           "function statements at top level can never be fl[...]
>+                           "variables they might have can't be flattened");

Code like this:

    {
        let x = 42;
        function f() { return x; }
    }

would produce a flat closure, but it happens that the TreeContext here is marked as TCF_FUN_HEAVYWEIGHT--which I think is just a bug. Global contexts should never have any of the TCF_FUN_* flags. The code that does this is js::Parser::functionDef:

        /*
         * If this function is not at body level of a program or function (i.e.
         * it is a function statement that is not a direct child of a program
         * or function), then our enclosing function, if any, must be
         * heavyweight.
         */
        if (!bodyLevel && kind == Statement)
            outertc->flags |= TCF_FUN_HEAVYWEIGHT;

Removing JSOP_DEFFUN_FC is fine with me, but the logic is kind of brittle. Let's not have this code in SemanticAnalysis.cpp depend on how the parser sets some flag. Instead make SemanticAnalysis.cpp explicitly test for JSOP_DEFFUN and say "no, we can't flatten that".

Comment 6

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb8e2ee24071
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa99b3a04938
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d1c5d3a29ff

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/bb8e2ee24071
https://hg.mozilla.org/mozilla-central/rev/fa99b3a04938
https://hg.mozilla.org/mozilla-central/rev/6d1c5d3a29ff