Persona is no longer an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 722121 - Remove CheckRedeclaration
: Remove CheckRedeclaration
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla13
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
: Jason Orendorff [:jorendorff]
Depends on: 781739
  Show dependency treegraph
Reported: 2012-01-28 22:08 PST by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2012-08-09 23:00 PDT (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Remove JSOP_DEF{VAR,CONST} uses (6.63 KB, patch)
2012-01-28 22:08 PST, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Splinter Review
Remove JSOP_DEFFUN_FC (4.83 KB, patch)
2012-01-28 23:12 PST, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Splinter Review
Remove CheckRedeclaration and its last use in JSOP_GETTER/JSOP_SETTER (3.77 KB, patch)
2012-01-28 23:18 PST, Jeff Walden [:Waldo] (remove +bmo to email)
jorendorff: review+
Details | Diff | Splinter Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2012-01-28 22:08:00 PST
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] (remove +bmo to email) 2012-01-28 22:08:59 PST
Created attachment 592465 [details] [diff] [review]

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.  <> 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] (remove +bmo to email) 2012-01-28 23:12:55 PST
Created attachment 592468 [details] [diff] [review]

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] (remove +bmo to email) 2012-01-28 23:18:01 PST
Created attachment 592469 [details] [diff] [review]
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:

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] 2012-02-08 06:55:11 PST
Comment on attachment 592465 [details] [diff] [review]

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.pc(), dn, f.fp()))

You are right, CheckRedeclaration does make this harder to follow than it should be.
Comment 5 Jason Orendorff [:jorendorff] 2012-02-08 07:25:37 PST
Comment on attachment 592468 [details] [diff] [review]

>+                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".

Note You need to log in before you can comment on or make changes to this bug.