1001090 - Implement ES6 "temporal dead zone" for let

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

ES6 distinguishes between let/const variables with the value `undefined` and those that have not been initialized at all.

    {
        let x;
        x;        // <==== ReferenceError: x is uninitialized
    }

This is a ReferenceError even in non-strict code.

Comment 1

[Jason Orendorff [:jorendorff]]

## Whaaaat? Where in the spec does it say this is a ReferenceError?

In the GetBindingValue method of declarative lexical environments <http://people.mozilla.org/~jorendorff/es6-draft-rev-22.html#sec-declarative-environment-records-getbindingvalue-n-s>:

> 3. If the binding for N in envRec is an uninitialised binding, then
>     a. If S is false, return the value undefined, otherwise throw a
>         ReferenceError exception.

Unfortunately, in the current draft, step 3.a. is incorrect; it states that the exception only happens in strict mode. According to Domenic, this is a spec bug. Step 3.a. should read simply "Throw a ReferenceError exception." Trying to confirm.

Anyway. How did we get to this method? Well, evaluating an identifier that refers to any kind of local binding produces a Reference:

- Evaluating an identifier calls ResolveBinding.
http://people.mozilla.org/~jorendorff/es6-draft-rev-22.html#sec-identifier-reference-runtime-semantics-evaluation

- ResolveBinding calls GetIdentifierReference.
http://people.mozilla.org/~jorendorff/es6-draft.html#sec-resolvebinding

- GetIdentifierReference, when an enclosing LexicalEnvironment has a binding
(which is the case if the identifier refers to a let/const binding in scope),
returns a Reference whose base value is that LexicalEnvironment.

Then, right after evaluation, GetValue is called to turn the Reference into a value. This always happens. Whenever identifiers are evaluated, except as assignment targets, GetValue is the next step.

- In the code above, we have an ExpressionStatement, so we call GetValue from:
http://people.mozilla.org/~jorendorff/es6-draft-rev-22.html#sec-expression-statement-runtime-semantics-evaluation

- The last step of GetValue calls the GetBindingValue "concrete method":
http://people.mozilla.org/~jorendorff/es6-draft-rev-22.html#sec-getvalue

Comment 2

[André Bargull [:anba]]

(In reply to Jason Orendorff [:jorendorff] from comment #1)
> Unfortunately, in the current draft, step 3.a. is incorrect; it states that
> the exception only happens in strict mode. According to Domenic, this is a
> spec bug. Step 3.a. should read simply "Throw a ReferenceError exception."
> Trying to confirm.

Filed at https://bugs.ecmascript.org/show_bug.cgi?id=2709

Comment 3

[André Bargull [:anba]]

(In reply to Jason Orendorff [:jorendorff] from comment #0)
> ES6 distinguishes between let/const variables with the value `undefined` and
> those that have not been initialized at all.
> 
>     {
>         let x;
>         x;        // <==== ReferenceError: x is uninitialized
>     }
> 
> This is a ReferenceError even in non-strict code.

And I guess the example should actually be:
---
{
    x;        // <==== ReferenceError: x is uninitialized
    let x;    
}
---

because after "let x;", "x" is initialized to "undefined". [13.2.1.4 Runtime Semantics: Evaluation]

Comment 4

[Jason Orendorff [:jorendorff]]

André is right about everything as usual.

In the bug linked in comment 2, Allen confirms that an error should be thrown in both strict and non-strict code.

Comment 5

[Shu-yu Guo [:shu]]

Similarly, for assignments to uninitialized let/const declarations, SetMutableBinding is what throws the ReferenceError in step 8.1.1.1.5 (3): https://people.mozilla.org/~jorendorff/es6-draft.html#sec-declarative-environment-records-setmutablebinding-n-v-s

Comment 6

[Shu-yu Guo [:shu]]

If I'm reading the spec right, the following throws a ReferenceError instead of binding the outer 'x'. Is this right?

{
  let x;
  {
     x; // 'x' is defined in the current declarative env, but uninitialized
     let x;
  }
}

Comment 7

[Till Schneidereit [:till]]

(In reply to Shu-yu Guo [:shu] from comment #6)
> If I'm reading the spec right, the following throws a ReferenceError instead
> of binding the outer 'x'. Is this right?

You probably already got an answer to this, but yes, that's correct. The temporal dead zone wouldn't make much sense otherwise, I think.

Comment 8

[Shu-yu Guo [:shu]]

Attachment: WIP Part 1: Frontend and interpreter parts of let temporal dead zone.

WIP of the approach so far. The frontend and interpreter parts.

The idea here is to:

 1) Introduce MagicValue(JS_UNINITIALIZED_LET), which will be used to
    initialize 'let' bindings and will throw ReferenceError on touch.

 2) Introduce a family of JSOps which check for the magic value for easy
    compilation in the JITs and as to not slowdown the existing LOCAL/ALIASEDVAR
    ops. The specific scenarios for which these new JSOps will be emitted are
    listed below.

    The new ops are:
     - JSOP_GETLET
     - JSOP_SETLET
     - JSOP_INITLET
     - JSOP_GETALIASEDLET
     - JSOP_SETALIASEDLET
     - JSOP_INITALIASEDLET

    The init variants are able to overwrite MagicValue(JS_UNINITIALIZED_LET).
    All other ops throw on touching the magic value.

    JSOP_NAME and JSOP_SETNAME, being deoptimized cases already, will always
    check for the magic value.

The assumption will be that most programs written with 'let' bindings will have
simple to compute (e.g., analyzable at parse time) dominance relation w.r.t.
their uses. So the patch only generates the LET ops when it cannot prove in a
straightforward fashion that a 'let' binding initialization dominates all its
uses. The scenarios the parser detects are:

 - A 'let' def that resolves existing lexdeps marks all of its lexdep's uses as
   hoisted uses, thus generating LET ops.

 - A free access to a 'let' in a body-level function declaration is marked as a
   hoisted let use if there are any uses of the function name before the
   observed declaration. This is due to its behavior of hoisting both its
   declaration and definition) 

 - When parsing an inner function lazily, free accesses are unknown at parse
   time to be 'let' or 'var' bindings. In this case, if any upvar use is
   hoisted at all---either the definition of the free variable in the outer
   parser context is a placeholder or the inner function is a body-level
   function declaration and has hoisted uses of its own name---we mark the
   LazyScript to convert any upvar let accesses during bytecode compilation to
   be one of the LET ops.

   This is a coarse solution. At the cost of memory (a boolean vector at the
   end of LazyScript), we could track hoisted uses on an upvar by upvar basis.

Body-level 'let's also have to now be distinguished from 'var's. This patch
puts all 'let' slots after all 'var' slots. That is, the fixed part of
execution frames have 'var' slots come first, then body-level 'let's, then
block-local 'let's.

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 8459125 [details] [diff] [review]
WIP Part 1: Frontend and interpreter parts of let temporal dead zone.

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

Seems reasonable.  Unfortunate language design decision, though, imho.  The PND_LET-flag-on-uses is a useful trick but a bit dangerous as sometimes we fiddle with the def-use links in a way that could fail to set the PND_LET flags on uses.  I'd grep around for this.  I keep hoping someone will embark on making the parser two-pass...

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1462,5 @@
> +                    // For emitting lazy scripts, we need to check if we are
> +                    // emitting a use to a hoisted let. See
> +                    // Parser<T>::addFreeVariablesFromLazyFunction.
> +                    if (bce->lazyCheckHoistedLetUse &&
> +                        IsAliasedNameLet(script, pn->pn_atom->asPropertyName()))

It seems like you could fuse the loop in IsAliasedNameLet with the loop in LookupAliasedName and avoid any overhead.  LookupAliasedName could just set the PND_LET flag directly.  Given that, perhaps there is no need for the lazyCheckHoistedLetUse flag (which I assume is an optimization?

::: js/src/jsscript.cpp
@@ +86,5 @@
>      JS_ASSERT(numVars <= LOCALNO_LIMIT);
>      JS_ASSERT(numBlockScoped <= LOCALNO_LIMIT);
> +    JS_ASSERT(numBodyLevelLets <= LOCALNO_LIMIT);
> +    JS_ASSERT(numVars <= LOCALNO_LIMIT - numBodyLevelLets - numBlockScoped);
> +    JS_ASSERT(UINT32_MAX - numArgs >= numVars + numBodyLevelLets + numBlockScoped);

I think these asserts can now underflow.  How about make them using uint64_t's?

::: js/src/vm/Stack-inl.h
@@ +103,5 @@
> +InterpreterFrame::setLetsToThrowOnTouch()
> +{
> +    // 'let' declaration throw ReferenceErrors if they are used before
> +    // initialization. See ES6 8.1.1.1.6.
> +    for (size_t slot = script()->nfixedvars(); slot < script()->nfixed(); slot++)

Could you have symbolic that make this ordering assumption more explicit such as JSScript::bodyLevelLetBegin/End()?  Also, even though this is all relatively cold code, it's probably still worth it to hoist the reads of these two fields since I expect the store in the body of the loop prevents their being LICM'd.

Comment 10

[Shu-yu Guo [:shu]]

(In reply to Luke Wagner [:luke] from comment #9)
> I keep hoping someone will embark on making the parser two-pass...

Not it!

Comment 11

[Shu-yu Guo [:shu]]

Attachment: WIP Part 1: Frontend and interpreter parts of let temporal dead zone.

Passes jit-tests without --tbpl. Haven't implemented the JIT parts yet.

This is the most gnarly patch imaginable; good luck.

There's a particular bit of nastiness involving legacy genexprs. For an
expression |((E for X in I))|, we create a LexicalScope for the entire
expression, then proceed to parse the for head (X in I) inside that scope. This
is *inside out* of what we do for |for (let X in I)|, which attaches the
LexicalScope to the initializer position of the for head. The reason it does
this is that we just have a single scope no matter how many 'for's are chained
in the genexpr, e.g., even |((E for X in I for Y in J))| still has a single
lexical scope. I guess we could change this, but I... really don't want to
touch this part of the code.

So, there's special casing for legacy genexprs for when to emit the initlets for
for-of/for-in as resulting from legacy genexprs.

Comment 12

[Shu-yu Guo [:shu]]

Attachment: Part 1: Frontend and interpreter parts of let temporal dead zone.

Cleaned up misc debug things I left in.

Comment 13

[Shu-yu Guo [:shu]]

Things this bug do NOT fix:

 - Parsing consts as lets
 - Scoping for the various for statements
 - Global level lets

Comment 14

[Shu-yu Guo [:shu]]

Attachment: Part 2: Compile new let opcodes in Baseline.

Comment 15

[Shu-yu Guo [:shu]]

Attachment: Part 3: Compile new let opcodes in Ion.

Comment 16

[Shu-yu Guo [:shu]]

Attachment: Part 3: Compile new let opcodes in Ion.

Comment 17

[Shu-yu Guo [:shu]]

Attachment: Part 1: Frontend and interpreter parts of let temporal dead zone.

Fix LazyScript::FreeVariable logic was wrong in BytecodeEmitter. The slot logic
was completely bogus and was marking upvars as hoisted let uses incorrectly.

Comment 18

[Shu-yu Guo [:shu]]

Attachment: Part 3: Compile new let opcodes in Ion.

Had an inverted condition in codegen for LLetCheck.

Comment 19

[Shu-yu Guo [:shu]]

Attachment: Part 3a: Fix unwinding all scopes to not use pc.

Jim, per our IRC conversation, changing UnwindScope uses where we want to
unwind to the outermost scope to not use a pc.

Comment 20

[Shu-yu Guo [:shu]]

Attachment: Part 3: Compile new let opcodes in Ion.

Comment 21

[Shu-yu Guo [:shu]]

Attachment: Part 2: Compile new let opcodes in Baseline.

Comment 22

[Shu-yu Guo [:shu]]

Attachment: Part 4a: Fix parse errors in addon-sdk/

Comment 23

[Shu-yu Guo [:shu]]

Attachment: Part 4b: Fix parse errors in browser/base/

Comment 24

[Shu-yu Guo [:shu]]

Attachment: Part 4c: Fix parse errors in browser/devtools/ and toolkit/devtools/

Comment 25

[Shu-yu Guo [:shu]]

Attachment: Part 4d: Fix parse errors in dom/browser-element/

Comment 26

[Shu-yu Guo [:shu]]

Attachment: Part 4e: Fix parse errors in dom/contacts/

Comment 27

[Shu-yu Guo [:shu]]

Attachment: Part 4f: Fix parse errors in services/crypto/

Comment 28

[Shu-yu Guo [:shu]]

Attachment: Part 4g: Fix parse errors in services/healthreport/

Comment 29

[Shu-yu Guo [:shu]]

Attachment: Part 4h: Fix parse errors in services/sync/

Comment 30

[Shu-yu Guo [:shu]]

Attachment: Part 4i: Fix parse errors in testing/marionette/

Comment 31

[Shu-yu Guo [:shu]]

Attachment: Part 4j: Fix parse errors in toolkit/components/contentprefs/

Comment 32

[Shu-yu Guo [:shu]]

Attachment: Part 4k: Fix parse errors in toolkit/components/crashes/

Comment 33

[Shu-yu Guo [:shu]]

Attachment: Part 4l: Fix parse errors in toolkit/components/passwordmgr/

Comment 34

[Shu-yu Guo [:shu]]

Attachment: Part 4m: Fix parse errors in Sqlite.jsm

Comment 35

[Shu-yu Guo [:shu]]

Attachment: Part 4n: Fix parse errors in AddonRepository.jsm

Comment 36

[Shu-yu Guo [:shu]]

Comment on attachment 8471205 [details] [diff] [review]
Part 3: Compile new let opcodes in Ion.

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

Not sure who to ask for these patches. Feel free to reassign, Jan.

Comment 37

[Shu-yu Guo [:shu]]

For all part 4 patch reviewers:

This bug is one of several bringing 'let' semantics in SpiderMonkey up to ES6 compliance. This bug changes the parsing of body-level 'let' declarations to be parsed as lets instead of as vars. As such, body-level 'let' declarations can no longer redeclare existing bindings.

These patches fix the redeclaration errors.

Comment 38

[Shu-yu Guo [:shu]]

Comment on attachment 8471906 [details] [diff] [review]
Part 4a: Fix parse errors in addon-sdk/

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

See instructions in comment 37.

Comment 39

[Shu-yu Guo [:shu]]

Comment on attachment 8471906 [details] [diff] [review]
Part 4a: Fix parse errors in addon-sdk/

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

See instructions in comment 37.

Comment 40

[Shu-yu Guo [:shu]]

Comment on attachment 8471908 [details] [diff] [review]
Part 4c: Fix parse errors in browser/devtools/ and toolkit/devtools/

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

See instructions in comment 37.

Comment 41

[Shu-yu Guo [:shu]]

Comment on attachment 8471910 [details] [diff] [review]
Part 4e: Fix parse errors in dom/contacts/

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

See instructions in comment 37.

Comment 42

[Shu-yu Guo [:shu]]

Comment on attachment 8471918 [details] [diff] [review]
Part 4l: Fix parse errors in toolkit/components/passwordmgr/

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

See instructions in comment 37.

Comment 43

[Tomislav Jovanovic :zombie]

Comment on attachment 8471906 [details] [diff] [review]
Part 4a: Fix parse errors in addon-sdk/

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

::: addon-sdk/source/lib/sdk/io/fs.js
@@ +646,5 @@
>  
>  /**
>   * Synchronous open(2).
>   */
> +function openSync(path, flags_, mode_) {

nit: this is an exported function (15 lines below), so i'd rather not change the argument definition, but change the local variable names instead.

Comment 44

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8471917 [details] [diff] [review]
Part 4k: Fix parse errors in toolkit/components/crashes/

>diff --git a/toolkit/components/crashes/CrashManager.jsm b/toolkit/components/crashes/CrashManager.jsm

>   _addSubmissionAsCrash: function (store, processType, crashType, succeeded,
>-                                   id, date) {
>-    let id = id + "-" + this.PROCESS_TYPE_SUBMISSION;
>+                                   id_, date) {
>+    let id = id_ + "-" + this.PROCESS_TYPE_SUBMISSION;
>     let process = processType + "-" + crashType + "-" +
>                   this.PROCESS_TYPE_SUBMISSION;
>     let submission_type = (
>       succeeded ? this.SUBMISSION_TYPE_SUCCEEDED : this.SUBMISSION_TYPE_FAILED);
> 
>     return store.addCrash(process, submission_type, id, date);
>   },

Why not just remove the "let"?

Comment 45

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8471916 [details] [diff] [review]
Part 4j: Fix parse errors in toolkit/components/contentprefs/

r=me with the let removed instead of renaming the parameter

Comment 46

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8471917 [details] [diff] [review]
Part 4k: Fix parse errors in toolkit/components/crashes/

r=me with the let removed instead of renaming the parameter

Comment 47

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8471913 [details] [diff] [review]
Part 4g: Fix parse errors in services/healthreport/

Re-using the parameter here is kind of gross, can you rename the parameter to "allAddons" and leave the "let addons" as-is? r=me with that.

Comment 48

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8471911 [details] [diff] [review]
Part 4f: Fix parse errors in services/crypto/

>diff --git a/services/crypto/modules/utils.js b/services/crypto/modules/utils.js

>-  stripHeaderAttributes: function(value) {
>-    let value = value || "";
>+  stripHeaderAttributes: function(value = "") {
>     let i = value.indexOf(";");
>     return value.substring(0, (i >= 0) ? i : undefined).trim().toLowerCase();
>   },

(Very poor use of the ternary operator IMO!)

Comment 49

[Jim Blandy :jimb]

Comment on attachment 8471204 [details] [diff] [review]
Part 3a: Fix unwinding all scopes to not use pc.

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

How could it be otherwise?

Comment 50

[Jan de Mooij [:jandem]]

Comment on attachment 8471206 [details] [diff] [review]
Part 2: Compile new let opcodes in Baseline.

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

Looks good, r=me with comments addressed.

::: js/src/jit/BaselineCompiler.cpp
@@ +256,5 @@
> +
> +    // Use R0 to minimize code size. If the number of locals to push is <
> +    // LOOP_UNROLL_FACTOR, then the initialization pushes are emitted directly
> +    // and inline.  Otherwise, they're emitted in a partially unrolled loop.
> +    size_t LOOP_UNROLL_FACTOR = 4;

Pre-existing nit: static const size_t

::: js/src/jit/VMFunctions.cpp
@@ +1186,5 @@
>      return &typedObj.typedProto();
>  }
>  
> +bool
> +ThrowUninitializedLet(JSContext *cx)

Maybe call it "ReportUnitializedLet" or "ThrowUnitializedLet" everywhere, since they do basically the same thing. I was initially confused by Throw vs Report.

@@ +1197,5 @@
> +        ReportUninitializedLet(cx, script, pc, GET_LOCALNO(pc));
> +    } else {
> +        MOZ_ASSERT(JSOp(*pc) == JSOP_CHECKALIASEDLET);
> +        ReportUninitializedLet(cx, script, pc, ScopeCoordinate(pc));
> +    }

If ReportUnitializedLet takes a cx/script/pc, you can just:

ReportUninitializedLet(cx, script, pc);
return false;

::: js/src/vm/Interpreter-inl.h
@@ +152,5 @@
>  }
>  
>  static inline bool
>  CheckUninitializedLet(JSContext *cx, HandleScript script, jsbytecode *pc,
>                        ScopeCoordinate sc, HandleValue val)

Can remove the |sc| argument here too (see other comment).

::: js/src/vm/Interpreter.cpp
@@ +4043,5 @@
>      ReportUninitializedLet(cx, name);
>  }
> +
> +void
> +js::ReportUninitializedLet(JSContext *cx, HandleScript script, jsbytecode *pc, ScopeCoordinate sc)

Remove the unused |sc| argument. I also think it'd be cleaner to have a single ReportUninitializedLet function to which you can pass just the script + pc, and have the function get the PropertyName depending on JSOp(*pc).

Comment 51

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #50)
> Maybe call it "ReportUnitializedLet" or "ThrowUnitializedLet" everywhere,
> since they do basically the same thing. I was initially confused by Throw vs
> Report.

Oh I just realized Throw* returns a bool and Report* does not, so using different names makes more sense then. (Alternative is to have both functions always |return false|, like js::Throw.)

Comment 52

[Jan de Mooij [:jandem]]

Comment on attachment 8471205 [details] [diff] [review]
Part 3: Compile new let opcodes in Ion.

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

r=me with comments addressed.

::: js/src/jit/CodeGenerator.cpp
@@ +8894,5 @@
> +    Label done;
> +    ValueOperand inputValue = ToValue(ins, LLetCheck::INPUT);
> +    masm.branchTestMagicValue(Assembler::Equal, inputValue, JS_UNINITIALIZED_LET, ool->entry());
> +
> +    masm.jump(&done);

Let's get rid of this jump in opt builds by moving it into the #ifdef DEBUG:

#ifdef DEBUG
    Label done;
    masm.jump(&done);
    masm.bind(ool->rejoin());
    masm.assumeUnreachable("ThrowUninitializedLet should always bail out.");
    masm.bind(&done);
#else
    masm.bind(ool->rejoin());
#endif

    return true;

Or just remove the assumeUnreachable call and "Label done" completely; it's not really necessary IMO.

@@ +8915,5 @@
> +    masm.bind(ool->rejoin());
> +#ifdef DEBUG
> +    masm.assumeUnreachable("ThrowUninitializedLet should always bail out.");
> +#endif
> +    return true;

If we make this a call instruction (see other comment), just do an inline call:

return callVM(ThrowUninitializedLetInfo, ins);

::: js/src/jit/IonBuilder.cpp
@@ +969,5 @@
> +{
> +    for (uint32_t i = 0; i < info().nlocals(); i++) {
> +        MConstant *val = MConstant::New(alloc(), (i < info().fixedLetBegin()
> +                                                  ? UndefinedValue()
> +                                                  : MagicValue(JS_UNINITIALIZED_LET)));

Pre-existing, but can you move the MConstant::New outside the loop? There's no need to add duplicate MIR instructions and then rely on GVN to coalesce them :)

@@ +9880,5 @@
> +    MDefinition *let = addLetCheck(getAliasedVar(sc));
> +    if (!let)
> +        return false;
> +
> +    jsbytecode *nextPc = pc + js_CodeSpec[JSOp(*pc)].length;

jsbytecode *nextPc = pc + JSOP_CHECKALIASEDLET_LENGTH;

@@ +9887,5 @@
> +
> +    // If we are checking for a load, push the checked let so that the load
> +    // can use it.
> +    if (JSOp(*nextPc) == JSOP_GETALIASEDVAR)
> +        pushLetCheck(let);

"push" is a bit confusing because it doesn't really push anything on the simulated expression stack. Maybe setLetCheck/getLetCheck?

@@ +10140,5 @@
>      JSObject *call = nullptr;
>      if (hasStaticScopeObject(sc, &call) && call) {
>          PropertyName *name = ScopeCoordinateName(scopeCoordinateNameCache, script(), pc);
>          bool succeeded;
>          if (!getStaticName(call, name, &succeeded))

The run-once closure case doesn't need changes? Please make sure we have tests for this, something like this in the global scope:

(function() {
    ...
})();

::: js/src/jit/LIR-Common.h
@@ +6172,5 @@
> +  public:
> +    LIR_HEADER(LetCheck)
> +
> +    explicit LLetCheck() {
> +    }

Nit: rm constructor

@@ +6178,5 @@
> +    MLetCheck *mir() {
> +        return mir_->toLetCheck();
> +    }
> +
> +    static const size_t INPUT = 0;

Nit: almost all other instructions in this file use "Input"

@@ +6181,5 @@
> +
> +    static const size_t INPUT = 0;
> +};
> +
> +class LThrowUninitializedLet : public LInstructionHelper<0, 0, 0>

s/LInstructionHelper/LCallInstructionHelper, as it always does a VM call.

@@ +6187,5 @@
> +  public:
> +    LIR_HEADER(ThrowUninitializedLet)
> +
> +    explicit LThrowUninitializedLet() {
> +    }

Nit: rm

Comment 53

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/base/content/test/

Comment 54

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/components/customizableui/test/

Comment 55

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/components/privatebrowsing/test/

Comment 56

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/components/sessionstore/test/

Comment 57

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/components/tabview/test/

Comment 58

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/components/translation/test/

Comment 59

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/devtools/canvasdebugger/test/

Comment 60

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/devtools/commandline/test/

Comment 61

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/devtools/debugger/test/ and toolkit/devtools/server/tests/

Comment 62

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/devtools/inspector/test/

Comment 63

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/devtools/markupview/test/

Comment 64

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/devtools/netmonitor/test/

Comment 65

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/devtools/profiler/test/

Comment 66

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/devtools/projecteditor/test/

Comment 67

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/devtools/responsivedesign/test/

Comment 68

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/devtools/shadereditor/test/

Comment 69

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/devtools/shared/test/

Comment 70

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/devtools/styleeditor/test/

Comment 71

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/devtools/styleinspector/test/

Comment 72

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/devtools/webaudioeditor/test/

Comment 73

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/devtools/webconsole/test/

Comment 74

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/experiments/test/

Comment 75

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in browser/modules/test/

Comment 76

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in dom/indexedDB/test/

Comment 77

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in dom/tests/mochitest/whatwg/

Comment 78

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in extensions/cookie/test/

Comment 79

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in image/test/

Comment 80

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in js/xpconnect/tests/

Comment 81

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in modules/libpref/test/

Comment 82

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in services/common/tests/

Comment 83

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in services/crypto/tests/

Comment 84

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in services/fxaccounts/tests/

Comment 85

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in services/sync/tests/

Comment 86

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in storage/test/

Comment 87

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in toolkit/components/contentprefs/tests/

Comment 88

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in toolkit/components/crashes/tests/

Comment 89

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in toolkit/components/ctypes/tests/

Comment 90

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in toolkit/components/jsdownloads/test/

Comment 91

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in toolkit/components/osfile/tests/

Comment 92

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in toolkit/components/passwordmgr/test/

Comment 93

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in toolkit/components/places/tests/ and browser/components/places/test/

Comment 94

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in toolkit/components/satchel/test/

Comment 95

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in toolkit/components/search/tests/

Comment 96

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in toolkit/components/social/test/

Comment 97

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in toolkit/modules/tests/

Comment 98

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in toolkit/mozapps/extensions/test/

Comment 99

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix parse errors in toolkit/mozapps/update/tests/

Comment 100

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in caps/tests/

Comment 101

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in dom/tests/

Comment 102

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in dom/workers/test/

Comment 103

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in dom/xbl/test/

Comment 104

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in browser/components/preferences/tests/

Comment 105

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in browser/devtools/responsivedesign/test/

Comment 106

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in browser/devtools/webaudioeditor/test/

Comment 107

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in browser/devtools/webconsole/test/

Comment 108

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in browser/devtools/webide/test/

Comment 109

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in dom/indexedDB/test/

Comment 110

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in services/sync/tests/

Comment 111

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in storage/test/

Comment 112

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in toolkit/components/places/tests/ and browser/components/places/test/

Comment 113

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in toolkit/mozapps/extensions/test/

Comment 114

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in dom/indexedDB/test/

Comment 115

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in toolkit/profile/test/

Comment 116

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in toolkit/content/tests/

Comment 117

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in browser/devtools/*/test/ and toolkit/devtools/*/tests/

Consolidate all devtools test patches into one.

Comment 118

[Shu-yu Guo [:shu]]

Comment on attachment 8474037 [details] [diff] [review]
Part 5: Fix errors in browser/devtools/*/test/ and toolkit/devtools/*/tests/

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

Rob, would you do the honor?

See comment 37 for instructions.

Comment 119

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 8474037 [details] [diff] [review]
Part 5: Fix errors in browser/devtools/*/test/ and toolkit/devtools/*/tests/

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

with pleasure. All hail the Temporal Dead Zone!

Comment 120

[Kevin Grandon :kgrandon]

Landed a gaia patch to update let usage in fake_update-checker.js: https://github.com/mozilla-b2g/gaia/commit/8732cae00450205ae23e8118668d1531055eb6f1

Comment 121

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in toolkit/mozapps/update/tests/

Comment 122

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in dom/wappush/tests/

Comment 123

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in dom/tests/

Comment 124

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in dom/system/gonk/tests/

Comment 125

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in dom/system/tests/

Comment 126

[Shu-yu Guo [:shu]]

Attachment: Part 4: Fix errors in MobileMessageDB.jsm

Comment 127

[Shu-yu Guo [:shu]]

Gavin, could I be given leeway for a mass r=me on the test fixes (all Part 5 patches)? New ones have been cropping up constantly since I started fixing these.

Comment 128

[Shu-yu Guo [:shu]]

Kevin, here's the new Gip failure. Another ID that looks like a use before def: https://tbpl.mozilla.org/php/getParsedLog.php?id=46339530&tree=Try

Comment 129

[Shu-yu Guo [:shu]]

Attachment: Part 5: Fix errors in tests throughout the tree. (r=robcee,??)

Consolidate all part 5 test patches into one for easier review. Gavin, if you
don't want to do it please assign somebody.

Comment 130

[Shu-yu Guo [:shu]]

Attachment: Part 4: Fix errors in nsBrowserGlue.js

Comment 131

[Shu-yu Guo [:shu]]

Attachment: Part 4: Fix errors in mobile/android/

Please see comment 37 for instructions.

Comment 132

[Shu-yu Guo [:shu]]

Attachment: Part 1: Implement let temporal dead zone in the frontend and interpreter.

Rebased.

Comment 133

[Shu-yu Guo [:shu]]

Attachment: Part 4: Fix errors in PlacesTransactions.jsm

Comment 134

[Shu-yu Guo [:shu]]

Attachment: Rollup of parts 1-3 for fuzzing

Could you guys fuzz this? Applies cleanly to cbbc380f1e1c.

Comment 135

[Kevin Grandon :kgrandon]

I've landed a gaia commit to update let usage of newTimerClassID: https://github.com/mozilla-b2g/gaia/commit/6c3c251ebd037710f370cda60b336b48fb4798be

Comment 136

[:Margaret Leibovic]

Comment on attachment 8476194 [details] [diff] [review]
Part 4: Fix errors in mobile/android/

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

This looks fine to me, but I'm a bit worried that there might be other declarations we're missing. Did you just use a try run to find these problems? Our test coverage for Fennec is pretty bad, so there may be more things we need to change, but we can always just fix those as they emerge. I can make a post to our mailing list when this lands to make sure people are on the lookout for errors.

Comment 137

[Shu-yu Guo [:shu]]

(In reply to :Margaret Leibovic from comment #136)
> Comment on attachment 8476194 [details] [diff] [review]
> Part 4: Fix errors in mobile/android/
> 
> Review of attachment 8476194 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine to me, but I'm a bit worried that there might be other
> declarations we're missing. Did you just use a try run to find these
> problems? Our test coverage for Fennec is pretty bad, so there may be more
> things we need to change, but we can always just fix those as they emerge. I
> can make a post to our mailing list when this lands to make sure people are
> on the lookout for errors.

I fixed everything that came up to make TBPL green. I'm afraid the only way to do it is to fix more problems as they come up. :(

Comment 138

[Shu-yu Guo [:shu]]

(In reply to Shu-yu Guo [:shu] from comment #137)
> (In reply to :Margaret Leibovic from comment #136)
> > Comment on attachment 8476194 [details] [diff] [review]
> > Part 4: Fix errors in mobile/android/
> > 
> > Review of attachment 8476194 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks fine to me, but I'm a bit worried that there might be other
> > declarations we're missing. Did you just use a try run to find these
> > problems? Our test coverage for Fennec is pretty bad, so there may be more
> > things we need to change, but we can always just fix those as they emerge. I
> > can make a post to our mailing list when this lands to make sure people are
> > on the lookout for errors.
> 
> I fixed everything that came up to make TBPL green. I'm afraid the only way
> to do it is to fix more problems as they come up. :(

I suppose it bears to mention that the errors I fixed were static errors, and I ran the JS shell in syntax checking mode for all chrome JS/JSM code.

Comment 139

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8476190 [details] [diff] [review]
Part 5: Fix errors in tests throughout the tree. (r=robcee,??)

>diff --git a/browser/components/preferences/tests/browser_chunk_permissions.js b/browser/components/preferences/tests/browser_chunk_permissions.js

>     run: function() {
>       let testSite1 = getSiteItem(TEST_URI_1.host);
>-      ok(!testSite2, "test site 1 was not removed from sites list");
>+      ok(testSite1, "test site 1 was not removed from sites list");
>       let testSite2 = getSiteItem(TEST_URI_2.host);
>       ok(!testSite2, "test site 2 was pre-removed from sites list");
>       let testSite3 = getSiteItem(TEST_URI_3.host);
>-      ok(!testSite2, "test site 3 was not removed from sites list");
>+      ok(testSite3, "test site 3 was not removed from sites list");

This looks like a good test fix, but I'm curious how this got exposed by this patch?

>diff --git a/browser/devtools/debugger/test/browser_dbg_optimized-out-vars.js b/browser/devtools/debugger/test/browser_dbg_optimized-out-vars.js

>     let panel, debuggee, gDebugger, sources;
> 
>-    let [, debuggee, panel] = yield initDebugger(TAB_URL);
>+    [, debuggee, panel] = yield initDebugger(TAB_URL);

Should remove these from the line above rather than removing the let. Same comment applies to browser_dbg_paused-keybindings.js.

>diff --git a/browser/devtools/debugger/test/browser_dbg_variables-view-data.js b/browser/devtools/debugger/test/browser_dbg_variables-view-data.js

>-  let __proto__ = someProp6.get("__proto__");
>+  __proto__ = someProp6.get("__proto__");

Is this doing the right thing? Should this use a different variable name?

>diff --git a/browser/devtools/webaudioeditor/test/browser_webaudio-actor-connect-param.js b/browser/devtools/webaudioeditor/test/browser_webaudio-actor-connect-param.js
>diff --git a/browser/devtools/webaudioeditor/test/browser_webaudio-actor-destroy-node.js b/browser/devtools/webaudioeditor/test/browser_webaudio-actor-destroy-node.js

>-  let [_, _, created] = yield Promise.all([
>+  let [_, __, created] = yield Promise.all([

These should just be omitted since they aren't used, right? (applies to both files)

>diff --git a/content/base/test/test_ipc_messagemanager_blob.html b/content/base/test/test_ipc_messagemanager_blob.html

>+    SimpleTest.requestCompleteLog();
> 
>+    dump("XXXshu HERE\n");

Presumably not meant for checkin!

>diff --git a/storage/test/unit/test_storage_connection.js b/storage/test/unit/test_storage_connection.js

> add_task(function test_clone_no_optional_param_async()
> {
>   "use strict";
>+  let result;
>   do_print("Testing async cloning");
>   let adb1 = yield openAsyncDatabase(getTestDB(), null);
>   do_check_true(adb1 instanceof Ci.mozIStorageAsyncConnection);
> 
>   do_print("Cloning database");
>   do_check_true(Components.isSuccessCode(result));

This line above (and the line you're adding) should just be removed. Components.isSuccessCode(result) is always true and there's nothing to assign to result here.

>   stmt.params.name = "yoric";
>-  let result = yield executeAsync(stmt);
>+  result = yield executeAsync(stmt);

... so you'll need to leave this as-is.

>diff --git a/toolkit/components/places/tests/expiration/test_removeAllPages.js b/toolkit/components/places/tests/expiration/test_removeAllPages.js

>   // Add some bookmarked page with visit and annotations.
>+  let now = Date.now() * 1000;
>   for (let i = 0; i < 5; i++) {

I think this might be a bug - there is a top-level declared "now" that is Date.now(), and now you're changing the use below to Date.now() * 1000. Simple fix is to just remove this addition, but I suppose this could be cleaned up further to be less confusing.

>     let pageURI = uri("http://item_anno." + i + ".mozilla.org/");
>     // This visit will be expired.
>     yield promiseAddVisits({ uri: pageURI, visitDate: now++ });

>diff --git a/toolkit/devtools/server/tests/mochitest/test_styles-applied.html b/toolkit/devtools/server/tests/mochitest/test_styles-applied.html
>diff --git a/toolkit/devtools/server/tests/mochitest/test_styles-svg.html b/toolkit/devtools/server/tests/mochitest/test_styles-svg.html

>-  let node = node;
>+  let node;

This line should just be removed from both of these files (node is later only defined as a parameter in an arrow function).

>diff --git a/toolkit/mozapps/extensions/test/browser/browser_cancelCompatCheck.js b/toolkit/mozapps/extensions/test/browser/browser_cancelCompatCheck.js

This file should just declare a1-10 at once rather than sprinkling them through the file out of order based on first-use.

r=me with those changes

Comment 140

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 8476193 [details] [diff] [review]
Part 4: Fix errors in nsBrowserGlue.js

># HG changeset patch
># User Shu-yu Guo <[e-mail]>
>
>Bug 1001090 - Part 4: Fix errors in nsBrowserGlue.js
>
>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js
>index edfca67..e5a0931 100644
>--- a/browser/components/nsBrowserGlue.js
>+++ b/browser/components/nsBrowserGlue.js
>@@ -2226,19 +2226,19 @@ let DefaultBrowserCheck = {
>     let label = bundle.getString("setDefaultBrowserNotNow.label");
>     notNowItem.setAttribute("label", label);
>     let accesskey = bundle.getString("setDefaultBrowserNotNow.accesskey");
>     notNowItem.setAttribute("accesskey", accesskey);
>     popup.appendChild(notNowItem);
> 
>     let neverItem = doc.createElement("menuitem");
>     neverItem.id = "defaultBrowserNever";
>-    let label = bundle.getString("setDefaultBrowserNever.label");
>+    label = bundle.getString("setDefaultBrowserNever.label");
>     neverItem.setAttribute("label", label);
>-    let accesskey = bundle.getString("setDefaultBrowserNever.accesskey");
>+    accesskey = bundle.getString("setDefaultBrowserNever.accesskey");
>     neverItem.setAttribute("accesskey", accesskey);
>     popup.appendChild(neverItem);
> 
>     popup.addEventListener("command", this);
> 
>     let popupset = doc.getElementById("mainPopupSet");
>     popupset.appendChild(popup);
>   },

Comment 141

[Gregor Wagner [:gwagner]]

Comment on attachment 8475692 [details] [diff] [review]
Part 4: Fix errors in MobileMessageDB.jsm

Fine with me but Vicamo is the owner here.

Comment 142

[Shu-yu Guo [:shu]]

Attachment: Part 1: Implement let temporal dead zone in the frontend and interpreter.

Bug 1001090 - Part 2a: Compile new let opcodes in Baseline. (r=jandem)

Bug 1001090 - Part 2b: Fix unwinding all scopes to not use pc. (r=jimb)

Bug 1001090 - Part 3: Compile new let opcodes in Ion. (r=jandem)


Rebased for fuzzing against f9bfe115fee5.

Comment 143

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8479387 [details] [diff] [review]
Part 1: Implement let temporal dead zone in the frontend and interpreter.

(function() {
    let a
    function a
})()

$ ./js-dbg-opt-64-dm-nsprBuild-linux-0753f7b93ab7-1001090-c142-8edb86d71dc2 --no-threads --no-baseline --no-ion 88.js
Assertion failure: aIndex < mLength, at ../../dist/include/mozilla/Vector.h:396

===

(function() {
    let x
    var r = /()/
    function x
})()

$ ./js-dbg-opt-64-dm-nsprBuild-linux-0753f7b93ab7-1001090-c142-8edb86d71dc2 --no-threads --no-baseline --no-ion 121.js
Assertion failure: vars_[oldDecl->pn_u.name.cookie.slot()] == oldDecl, at /home/fuzz2lin/trees/mozilla-central/js/src/frontend/Parser.cpp:307

===

(function() {
    let arguments
})()

$ ./js-dbg-opt-64-dm-nsprBuild-linux-0753f7b93ab7-1001090-c142-8edb86d71dc2 --no-threads --no-baseline --no-ion 420.js
Assertion failure: !IsUninitializedLet((activation.regs()).fp()->unaliasedLocal(i)), at /home/fuzz2lin/trees/mozilla-central/js/src/vm/Interpreter.cpp:3020

===

(function() {
    with(x);
    let x
})()

$ ./js-dbg-opt-64-dm-nsprBuild-linux-0753f7b93ab7-1001090-c142-8edb86d71dc2 --no-threads --ion-offthread-compile=off --ion-eager 422.js
Assertion failure: !val.isMagic(), at /home/fuzz2lin/trees/mozilla-central/js/src/jit/BaselineIC.cpp:1185

Comment 144

[Shu-yu Guo [:shu]]

Attachment: Fuzzing rollup v2

Fixed round 1 of errors; let's go for round 2.

Comment 145

[Christian Holler (:decoder)]

I'm getting a compile error:

make[3]: Entering directory `/srv/repos/mozilla-central/js/src/debug64/mfbt/tests'
c++ -o TestMaybe.o -c  -I../../dist/system_wrappers -include /srv/repos/mozilla-central/config/gcc_hidden_dso_handle.h -DIMPL_MFBT -DAB_CD= -DNO_NSPR_10_SUPPORT -I/srv/repos/mozilla-central/mfbt/tests -I.  -I../../dist/include         -I../../dist/include/testing  -fPIC   -DMOZILLA_CLIENT -include ../../js/src/js-confdefs.h -MD -MP -MF .deps/TestMaybe.o.pp  -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Werror=int-to-pointer-cast -Wtype-limits -Wempty-body -Werror=conversion-null -Wsign-compare -Wno-invalid-offsetof -Wcast-align -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe  -DDEBUG -DTRACING -g -freorder-blocks -O3  -fno-omit-frame-pointer       /srv/repos/mozilla-central/mfbt/tests/TestMaybe.cpp
/srv/repos/mozilla-central/mfbt/tests/TestMaybe.cpp: In function ‘bool TestBasicFeatures()’:
/srv/repos/mozilla-central/mfbt/tests/TestMaybe.cpp:242:65: error: template argument 2 is invalid
make[3]: *** [TestMaybe.o] Error 1



This is with rev f9bfe115fee5 + the patch in comment 144.

Comment 146

[Shu-yu Guo [:shu]]

(In reply to Christian Holler (:decoder) from comment #145)
> I'm getting a compile error:
> 
> make[3]: Entering directory
> `/srv/repos/mozilla-central/js/src/debug64/mfbt/tests'
> c++ -o TestMaybe.o -c  -I../../dist/system_wrappers -include
> /srv/repos/mozilla-central/config/gcc_hidden_dso_handle.h -DIMPL_MFBT
> -DAB_CD= -DNO_NSPR_10_SUPPORT -I/srv/repos/mozilla-central/mfbt/tests -I. 
> -I../../dist/include         -I../../dist/include/testing  -fPIC  
> -DMOZILLA_CLIENT -include ../../js/src/js-confdefs.h -MD -MP -MF
> .deps/TestMaybe.o.pp  -Wall -Wpointer-arith -Woverloaded-virtual
> -Werror=return-type -Werror=int-to-pointer-cast -Wtype-limits -Wempty-body
> -Werror=conversion-null -Wsign-compare -Wno-invalid-offsetof -Wcast-align
> -fno-rtti -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe 
> -DDEBUG -DTRACING -g -freorder-blocks -O3  -fno-omit-frame-pointer      
> /srv/repos/mozilla-central/mfbt/tests/TestMaybe.cpp
> /srv/repos/mozilla-central/mfbt/tests/TestMaybe.cpp: In function ‘bool
> TestBasicFeatures()’:
> /srv/repos/mozilla-central/mfbt/tests/TestMaybe.cpp:242:65: error: template
> argument 2 is invalid
> make[3]: *** [TestMaybe.o] Error 1
> 
> 
> 
> This is with rev f9bfe115fee5 + the patch in comment 144.

That shouldn't be me. I'll take a look tomorrow. Maybe try with clang?

Comment 147

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8480180 [details] [diff] [review]
Fuzzing rollup v2

function g(s) {
    L = s.length;
    for (var i = 0; i < L; i++) {
        a = s.charAt()
    }
}
function h(f, inputs) {
    results = [];
    for (var j = 0; j < 99; ++j) {
        for (var k = 0; k < 99; ++k) {
            try {
                results.push(f())
            } catch (e) {}
        }
    }
    print(g(uneval(results)))
}
m = (function(x, y) {});
h(m, [])
try {
    print(x);
    let x = s()
} catch (e) {}

$ ./js-dbg-opt-64-dm-nsprBuild-linux-85135c5c6ba8-1001090-c144-f3b00ce2b15b --fuzzing-safe --ion-offthread-compile=off w1577-cj-in.js
undefined

$ ./js-dbg-opt-64-dm-nsprBuild-linux-85135c5c6ba8-1001090-c144-f3b00ce2b15b --fuzzing-safe --ion-offthread-compile=off --no-baseline w1577-cj-in.js
undefined
undefined

(without the patch, it seems to give me 2x undefined either way)

Comment 148

[Mike Conley (:mconley) (:⚙️)]

Hey :shu,

We've filed bug 1054357 to fix our usage of let under comm-central, but it'd really help if we knew how best to find the problems we need to solve. Did you use some sort of script or static analysis tool to find the places where you needed to change things under mozilla-central?

-Mike

Comment 149

[Shu-yu Guo [:shu]]

Attachment: Part 1: Implement let temporal dead zone in the frontend and interpreter.

Incorporated fixes from the fuzzers

Comment 150

[Shu-yu Guo [:shu]]

Attachment: Fuzzing rollup v3

Round 3.

Christian, I don't get any compile errors, so I don't know what's going on with
your build.

Comment 151

[Shu-yu Guo [:shu]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #148)
> Hey :shu,
> 
> We've filed bug 1054357 to fix our usage of let under comm-central, but it'd
> really help if we knew how best to find the problems we need to solve. Did
> you use some sort of script or static analysis tool to find the places where
> you needed to change things under mozilla-central?
> 
> -Mike

Hey Mike,

Thanks for being proactive about this! I don't have an automatic script, but here are manual steps to find the static redeclaration errors (as I said in the dev-platform email, that's about 95%+ of the errors). For dynamic errors, I'm afraid those are only discoverable at runtime.

1. Check out https://github.com/syg/gecko-dev/tree/es6-let-dz
2. Build a JS shell per normal
3. Build Firefox as well; you don't have wait for it to finish, just long enough for it to have postprocessed all the chrome JS code. It's hard to statically check the raw chrome JS since many have cpp-like macros in them.
4. Find all the .js and .jsm code in the build dir, i.e. |find . -regex ".+\.jsm?"|
5. For each file $f from step 4, run |path/to/js -c $f| using the shell you built in step 2.
6. Grep for "redeclaration" errors from step 5 and fix those files.

Hope this helps; ping me on IRC if you run into issues.

Comment 152

[Shu-yu Guo [:shu]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #148)
> Hey :shu,
> 
> We've filed bug 1054357 to fix our usage of let under comm-central, but it'd
> really help if we knew how best to find the problems we need to solve. Did
> you use some sort of script or static analysis tool to find the places where
> you needed to change things under mozilla-central?
> 
> -Mike

To what extent does comm-central share code with mozilla-central? I've fixed everything I found in mozilla-central already, as part 5 of this bug.

Comment 153

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8480848 [details] [diff] [review]
Fuzzing rollup v3

function g(s) {
    for (var i = 0; i < s.length; i++) {
        s.charAt()
    }
}
function h(f, inputs) {
    results = []
    for (var j = 0; j < 99; ++j) {
        for (var k = 0; k < 99; ++k) {
            try {
                results.push(f())
            } catch (e) {}
        }
    }
    g(uneval(results))
}
try {
    x()
} catch (e) {}
m = function(y) {
    return y;
};
h(m, []);
try {
    print(b)
    let b = "";
} catch (e) {}

$ ./js-dbgDisabled-opt-64-dm-nsprBuild-linux-85135c5c6ba8-1001090-c150-1c911a71df46 --fuzzing-safe --ion-offthread-compile=off w233-cj-in.js
ReferenceError: x is not defined

$ ./js-dbgDisabled-opt-64-dm-nsprBuild-linux-85135c5c6ba8-1001090-c150-1c911a71df46 --fuzzing-safe --ion-offthread-compile=off --no-baseline w233-cj-in.js
undefined

Full configuration command with needed environment variables is:
AR=ar sh /home/fuzz2lin/trees/mozilla-central/js/src/configure --disable-debug --enable-optimize --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests

Comment 154

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: debug and opt stacks

evalcx("\
    for(x = 0; x < 9; x++) {\
        let y = y.s()\
    }\
", newGlobal())

Additionally, this crashes opt [@ ObjectType] and asserts debug at Assertion failure: data.s.payload.why == why, at dist/include/js/Value.h

Comment 155

[Shu-yu Guo [:shu]]

Attachment: Fuzzing rollup v4

Round 4. Applies cleanly to m-c d697d649c765

Comment 156

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: assertion stack (top 18 lines)

(function() {
    ((function() {
        p(y)
    })());
    let y
})()

Assertion failure: false (MOZ_ASSERT_UNREACHABLE: unexpected type), at jit/LIR.h

Comment 157

[Mike Conley (:mconley) (:⚙️)]

(In reply to Shu-yu Guo [:shu] from comment #152)
> (In reply to Mike Conley (:mconley) - Needinfo me! from comment #148)
> > Hey :shu,
> > 
> > We've filed bug 1054357 to fix our usage of let under comm-central, but it'd
> > really help if we knew how best to find the problems we need to solve. Did
> > you use some sort of script or static analysis tool to find the places where
> > you needed to change things under mozilla-central?
> > 
> > -Mike
> 
> To what extent does comm-central share code with mozilla-central? I've fixed
> everything I found in mozilla-central already, as part 5 of this bug.

comm-central relies on mozilla-central as a dependency, but there is a non-trivial amount of code under there that will also be affected by this (the Thunderbird, Instantbird and SeaMonkey codebases are under there). I see that you've found a bunch of stuff to fix under mozilla-central, and that's fantastic - how did you find it? Was it an automated process, or was it tool assisted? How can we apply the same technique to comm-central?

Comment 158

[Shu-yu Guo [:shu]]

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #157)
> (In reply to Shu-yu Guo [:shu] from comment #152)
> > (In reply to Mike Conley (:mconley) - Needinfo me! from comment #148)
> > > Hey :shu,
> > > 
> > > We've filed bug 1054357 to fix our usage of let under comm-central, but it'd
> > > really help if we knew how best to find the problems we need to solve. Did
> > > you use some sort of script or static analysis tool to find the places where
> > > you needed to change things under mozilla-central?
> > > 
> > > -Mike
> > 
> > To what extent does comm-central share code with mozilla-central? I've fixed
> > everything I found in mozilla-central already, as part 5 of this bug.
> 
> comm-central relies on mozilla-central as a dependency, but there is a
> non-trivial amount of code under there that will also be affected by this
> (the Thunderbird, Instantbird and SeaMonkey codebases are under there). I
> see that you've found a bunch of stuff to fix under mozilla-central, and
> that's fantastic - how did you find it? Was it an automated process, or was
> it tool assisted? How can we apply the same technique to comm-central?

See comment 151

Comment 159

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack for second assertion

{
    let x = function() {} ? x() : function() {}
}

$ ./js-dbg-opt-64-dm-nsprBuild-linux-85135c5c6ba8-1001090-c155-0c3b1213ccbf --no-threads --ion-offthread-compile=off --ion-eager w3889-reduced.js

Assertion failure: v.isUndefined(), at jsstr.cpp

This comment and comment 156 are for fuzzing rollup v4 in comment 155.

Comment 160

[Christian Holler (:decoder)]

Comment on attachment 8481102 [details] [diff] [review]
Fuzzing rollup v4

I only saw some of the issues that gkw already reported here, so feedback+ from my side.

Comment 161

[Shu-yu Guo [:shu]]

Attachment: Part 1: Implement let temporal dead zone in the frontend and interpreter.

More fuzz fixes.

Comment 162

[Shu-yu Guo [:shu]]

Attachment: Fuzzing rollup v5

Bug 1001090 - Part 1: Implement let temporal dead zone in the frontend and interpreter.

Bug 1001090 - Part 2a: Compile new let opcodes in Baseline. (r=jandem)

Bug 1001090 - Part 2b: Fix unwinding all scopes to not use pc. (r=jimb)

Bug 1001090 - Part 3: Compile new let opcodes in Ion. (r=jandem)

Comment 163

[Shu-yu Guo [:shu]]

(In reply to Shu-yu Guo [:shu] from comment #162)
> Created attachment 8482894 [details] [diff] [review]
> Fuzzing rollup v5
> 
> Bug 1001090 - Part 1: Implement let temporal dead zone in the frontend and
> interpreter.
> 
> Bug 1001090 - Part 2a: Compile new let opcodes in Baseline. (r=jandem)
> 
> Bug 1001090 - Part 2b: Fix unwinding all scopes to not use pc. (r=jimb)
> 
> Bug 1001090 - Part 3: Compile new let opcodes in Ion. (r=jandem)

Still applies on top of m-c d697d649c765

Comment 164

[Shu-yu Guo [:shu]]

Attachment: Rollup v5

Applies cleanly to m-c 372ce1f36116

Comment 165

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8483188 [details] [diff] [review]
Rollup v5

(function() {
    let x = (function() { t(x) })()
})()

$ ./js-dbgDisabled-opt-64-dm-nsprBuild-linux-372ce1f36116-1001090-c164-c2d48dfd6a07 --no-threads --ion-eager testcase.js

Assertion failure: false (MOZ_ASSERT_UNREACHABLE: unexpected type), at jit/LIR.h:577

Comment 166

[Shu-yu Guo [:shu]]

Attachment: Fuzzing rollup v6

Sigh.

Comment 167

[Shu-yu Guo [:shu]]

Attachment: Part 1: Implement let temporal dead zone in the frontend and interpreter.

Comment 168

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 8483337 [details] [diff] [review]
Fuzzing rollup v6

Let's quickly land this. I didn't find anything more after a few hours' of fuzzing - will be sure to file more bugs should anymore pop out later.

Comment 169

[Jeff Walden [:Waldo]]

Comment on attachment 8483338 [details] [diff] [review]
Part 1: Implement let temporal dead zone in the frontend and interpreter.

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

Add a test that checks semantics of this:

  x = fThrowing(); // should throw fThrowing's thing, not the TDZ ReferenceError
  let x;

::: js/public/Value.h
@@ +243,5 @@
>      JS_ION_ERROR,                /* error while running Ion code */
>      JS_ION_BAILOUT,              /* missing recover instruction result */
>      JS_OPTIMIZED_OUT,            /* optimized out slot */
> +    JS_UNINITIALIZED_LET,        /* uninitialized let bindings that produce ReferenceError on
> +                                  * touch. */

I'd name this JS_UNINITIALIZED_LEXICAL or something, so as to sweep up const in the future.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +1076,5 @@
> +    return true;
> +}
> +
> +static bool
> +EmitUnaliasedVarOp(ExclusiveContext *cx, JSOp op, uint32_t slot, bool checkUninitializedLet,

enum for checkUninitializedLet would be nice.

@@ +1165,5 @@
> +                // Check if the free variable from a lazy script was marked as
> +                // a possible hoisted use and is a let. If so, mark it as such
> +                // so we emit a dead zone check.
> +                if (bce->emitterMode == BytecodeEmitter::LazyFunction) {
> +                    LazyScript::FreeVariable *freeVariables = bce->lazyScript->freeVariables();

Hoist this (null if not emitterMode == LazyFunction), and then we can continue if null and not indent as much.

@@ +1166,5 @@
> +                // a possible hoisted use and is a let. If so, mark it as such
> +                // so we emit a dead zone check.
> +                if (bce->emitterMode == BytecodeEmitter::LazyFunction) {
> +                    LazyScript::FreeVariable *freeVariables = bce->lazyScript->freeVariables();
> +                    uint32_t letBegin = script->bindings.numArgs() + script->bindings.numVars();

Wasn't there a helper method that returned args + vars that would read better here?

letBegin is loop-invariant, hoist please.

@@ +1167,5 @@
> +                // so we emit a dead zone check.
> +                if (bce->emitterMode == BytecodeEmitter::LazyFunction) {
> +                    LazyScript::FreeVariable *freeVariables = bce->lazyScript->freeVariables();
> +                    uint32_t letBegin = script->bindings.numArgs() + script->bindings.numVars();
> +                    for (uint32_t i = 0; i < bce->lazyScript->numFreeVariables(); i++) {

numFreeVariables is hoistable too.

@@ +2519,5 @@
>      SET_JUMP_OFFSET(bce->code(off), bce->offset() - off);
>  }
>  
>  static bool
> +PushConstantValues(ExclusiveContext *cx, BytecodeEmitter *bce, JSOp op, unsigned n)

PushInitialValue, and make it MOZ_ASSERT(op == JSOP_UNDEFINED || op == JSOP_UNINITIALIZED);.

@@ +3593,5 @@
>              if (pn2->isKind(PNK_ARRAY) || pn2->isKind(PNK_OBJECT)) {
> +                // If the emit option is DefineVars, emit variable binding
> +                // ops, but not destructuring ops.  The parser (see
> +                // Parser::variables) has ensured that our caller will be the
> +                // PNK_FOR/PNK_FORIN/PNK_FOROF case in EmitTree, and that case

EmitTree (we don't have to worry about this being a random variable declaration, because |var [x];| and such isn't legal syntax)

@@ +3596,5 @@
> +                // Parser::variables) has ensured that our caller will be the
> +                // PNK_FOR/PNK_FORIN/PNK_FOROF case in EmitTree, and that case
> +                // will emit the destructuring code only after emitting an
> +                // enumerating opcode and a branch that tests whether the
> +                // enumeration ended.

Thus each iteration's assignment is responsible for initializing, and we don't need to do anything here.

@@ +3605,2 @@
>                  JS_ASSERT(pn->pn_count == 1);
> +                if (emitOption == DefineVars) {

&& !pn->isKind(PNK_LET)

@@ +3613,5 @@
> +                    MOZ_ASSERT_IF(emitOption == InitializeVars, pn->pn_xflags & PNX_POPVAR);
> +                    if (Emit1(cx, bce, JSOP_UNDEFINED) < 0)
> +                        return false;
> +                    if (!EmitInitializeDestructuringDecls(cx, bce, pn->getOp(), pn2))
> +                        return false;

So as I read this, it really really looks like this would fail:

   var x = 5;
   var [x];
   assertEq(x, 5);

Except that |var [x];| isn't legal syntax.  Let's double-check this (and |let [x];| for good measure) are both syntax errors, so that we don't have to worry about JSOP_UNDEFINED overwriting a previously-valid value, in a case where it shouldn't have.  (It could in two cases: overwriting a loop variable each time around, or in the |try {} catch (e) {} try {} catch (e) {}| case due to slot reuse.)

@@ +4609,5 @@
> +    // we have PNK_LET without a lexical scope, because those expressions are
> +    // parsed with single lexical scope for the entire comprehension. In this
> +    // case we must initialize the lets to not trigger dead zone checks via
> +    // InitializeVars.
> +    if (pn && !*letDecl) {

pn ain't null here.

@@ +4667,5 @@
>      }
>  
>      // Enter the block before the loop body, after evaluating the obj.
> +    // Initialize let bindings with undefined when entering, as the name
> +    // assigned to is a plain assignment.

Let's add a separate test for

  var x = "foobar";
  { for (let x of x) assertEq(x.length, 1, "second x refers to outer x"); }

that ensures that the RHS x refers to the var x.  If it referred to the let x, it would be a TDZ fail.  And we don't want someone making the RHS x refer to the let x without ensuring TDZ correctness.

@@ +4816,3 @@
>      StmtInfoBCE letStmt(cx);
>      if (letDecl) {
> +        if (!EnterBlockScope(cx, bce, &letStmt, pn1->pn_objbox, JSOP_UNDEFINED, 0))

Add the same separate test as above, except for for-in instead of for-of.

::: js/src/frontend/Parser.cpp
@@ +2802,5 @@
> +    // js::frontend::CompileScript will adjust the slot again to include
> +    // script->nfixed and body-level lets.
> +    //
> +    // For body-level lets, the index is bogus at this point and is adjusted
> +    // when creating Bindings. See ParseContext::generateFunctionBindings.

...and AppendPackedBindings.

::: js/src/frontend/Parser.h
@@ +40,5 @@
> +
> +    explicit StmtInfoPC(ExclusiveContext *cx)
> +      : StmtInfoBase(cx),
> +        innerBlockScopeDepth(0),
> +        firstDominatingLetInCase(0)

If this field's only valid for switches, it seems better to leave it uninitialized such that valgrind and friends will complain if it's used.  Either that, or you depend on its being zero in the other cases and should say *that*, not that it's not "valid".

@@ +574,5 @@
>      bool functionArgsAndBody(Node pn, HandleFunction fun,
>                               FunctionType type, FunctionSyntaxKind kind,
>                               GeneratorKind generatorKind,
> +                             Directives inheritedDirectives, Directives *newDirectives,
> +                             bool bodyLevelHoistedUse);

Can this be an enum somehow?  (Same for the other new bool params in this file.)  This function takes enough arguments as is, without throwing in an unreadable true/false at call sites.

::: js/src/jit-test/tests/basic/bug1001090-1.js
@@ +1,3 @@
> +(function() {
> +    let arguments
> +})()

(() => { let arguments; })(); too, please, so we test that kind of function.

::: js/src/jit-test/tests/basic/testFunctionStatementAliasLocals.js
@@ -15,5 @@
>  assertEq(typeof f2(true, 3), "function");
>  assertEq(f2(false, 3), 3);
> -
> -function f3(b) {
> -    let (w = 3) {

What changed with this test to make it invalid?

::: js/src/jit-test/tests/basic/testLet.js
@@ +49,5 @@
> +    var caught = false;
> +    try {
> +        (new Function(str))();
> +    } catch(e) {
> +        assertEq(String(e).indexOf('ReferenceError') == 0, true);

e instanceof ReferenceError, please.

::: js/src/jit/BaselineFrame.cpp
@@ +91,5 @@
>          // Mark operand stack.
>          MarkLocals(this, trc, nfixed, numValueSlots());
>  
> +        // Clear non-magic dead locals. Magic values such as
> +        // JS_UNINITIALIZED_LET need to be left as is for correctness.

Instead of "for correctness", say something like "because we don't want the GC silently deciding to initialize lets for us (causing subsequent uses of those lets to not TDZ-fail)".  Also mention the two-ish tests that specifically exercise this code.

::: js/src/js.msg
@@ +108,5 @@
>  MSG_DEF(JSMSG_BAD_PROTOTYPE,           1, JSEXN_TYPEERR, "'prototype' property of {0} is not an object")
>  MSG_DEF(JSMSG_IN_NOT_OBJECT,           1, JSEXN_TYPEERR, "invalid 'in' operand {0}")
>  MSG_DEF(JSMSG_TOO_MANY_CON_SPREADARGS, 0, JSEXN_RANGEERR, "too many constructor arguments")
>  MSG_DEF(JSMSG_TOO_MANY_FUN_SPREADARGS, 0, JSEXN_RANGEERR, "too many function arguments")
> +MSG_DEF(JSMSG_UNINITIALIZED_LET,       1, JSEXN_REFERENCEERR, "can't access let declaration `{0}' before initialization")

UNINITIALIZED_LEXICAL again.  Message should probably stay as-is for now.

::: js/src/jsscript.cpp
@@ +458,4 @@
>      size_t numFreeVariables = lazy->numFreeVariables();
>      for (size_t i = 0; i < numFreeVariables; i++) {
>          if (mode == XDR_ENCODE)
> +            atom = freeVariables[i].atom();

Seems like you need to encode/decode the is-this-a-hoisted-use thing here.  Stripping it off like this seems like sadtimes.

@@ +3393,5 @@
> +    // We rely on the fact that atoms are always tenured.
> +    FreeVariable *freeVariables = this->freeVariables();
> +    for (size_t i = 0; i < numFreeVariables(); i++) {
> +        JSAtom *atom = freeVariables[i].atom();
> +        MarkStringUnbarriered(trc, &atom, "lazyScriptFreeVariable");

Maybe add an assert that atoms are tenured or something, to answer the should-be-barriered questions of some readers.  If MSUb doesn't itself already.

::: js/src/jsscript.h
@@ +138,5 @@
>  
>    public:
> +    // A "binding" is a formal, 'var' (also a stand in for body-level 'let'
> +    // declarations), or 'const' declaration. A function's lexical scope is
> +    // composed of these three kinds of bindings.

If you're touching this, could you make it not use the "formal" term?  Makes me ask "formal what?" when I read it.

Either that, or remove the first sentence of this entirely.  It fairly clearly duplicates the enum initializer names.

::: js/src/vm/Debugger.h
@@ +510,5 @@
>       * true }.
> +     *
> +     * If *vp is a magic JS_UNINITIALIZED_LET value signifying an unaccessible
> +     * uninitialized binding, this produces a plain object of the form
> +     * { uninitialized: true }.

Talk to jimb about what Debugger docs to change to document this.

::: js/src/vm/Interpreter-inl.h
@@ +125,5 @@
> +IsUninitializedLetSlot(HandleObject obj, HandleShape shape)
> +{
> +    if (obj->is<DynamicWithObject>())
> +        return false;
> +    if (!shape || IsImplicitDenseOrTypedArrayElement(shape) || !shape->hasSlot())

Could you add a MOZ_ASSERT_IF(shape, obj->nativeContainsPure(shape)) here, to clarify that the provided arguments cohere in being from a single lookup whose results remain valid?

Given that this is only consulted, and *should* only be consulted, for name lookups vetted by the caller to be actual property names, I think we should further assert !IsImplicitDenseOrTypedArrayElement(shape).

Slotful properties backed by propertyops will, I think, pass this test.  Tack on !hasDefaultGetter() and !hasDefaultSet

::: js/src/vm/ScopeObject-inl.h
@@ +48,5 @@
> +inline void
> +CallObject::setAliasedLetsToThrowOnTouch(JSScript *script)
> +{
> +    uint32_t aliasedLetStart = script->bindings.aliasedBodyLevelLetStart();
> +    uint32_t aliasedLetEnd = numFixedSlots();

A separate method akin to aliasedBodyLevelLetStart() that retuns this returns this value would be nice.

::: js/src/vm/Stack-inl.h
@@ +88,5 @@
>      prevpc_ = prevpc;
>      prevsp_ = prevsp;
>  
>      initVarsToUndefined();
> +    setLetsToThrowOnTouch();

These two methods are always called together.  Conceptually, they initialize all the locals.  So they should be implemented in a single initializeLocals() method.

This also has the minor benefit, if done readably, of obviously initializing each local exactly once.  As it is now, first we initialize all of them to undefined, then we go back and overwrite all the lets with JS_UNINITIALIZED_LET.  I don't have much faith in compilers detecting this and optimizing to a single write-pass for this.  Also it seems much clearer to initialize the one portion of them, then to initialize the other portion, each clearly exclusive of the other.

@@ +98,5 @@
>      SetValueRangeToUndefined(slots(), script()->nfixed());
>  }
>  
> +inline void
> +InterpreterFrame::setLetsToThrowOnTouch()

General comment, probably made elsewhere in this patch but repeated here: don't refer to these as "lets".  const is identical to let in ES6 terms, just that it's write-once, so any mention of "let" will become obsolete when we fix const to be let-like.

@@ +101,5 @@
> +inline void
> +InterpreterFrame::setLetsToThrowOnTouch()
> +{
> +    // 'let' declaration throw ReferenceErrors if they are used before
> +    // initialization. See ES6 8.1.1.1.6.

Lexical bindings throw...

Also mention 13.1.11, 9.2.13, 13.6.3.4, 13.6.4.6, 13.6.4.8, 13.14.5, 15.1.8, 15.2.0.15 in some way such that it's clear the callers, by not calling InitializeBinding, trigger this operation.  Or something like that.

@@ +103,5 @@
> +{
> +    // 'let' declaration throw ReferenceErrors if they are used before
> +    // initialization. See ES6 8.1.1.1.6.
> +    for (size_t slot = script()->fixedLetBegin(), end = script()->fixedLetEnd(); slot < end; slot++)
> +        slots()[slot].setMagic(JS_UNINITIALIZED_LET);

Unifying the two init methods into one will probably make this fall out, but please hoist slots() into a local variable outside the loop.

::: js/src/vm/Stack.cpp
@@ +408,5 @@
>          // Mark operand stack.
>          markValues(trc, nfixed, sp - slots());
>  
> +        // Clear non-magic dead locals. Magic values such as
> +        // JS_UNINITIALIZED_LET need to be left as is for correctness.

Probably worth adding the same sort of comment as in Baseline here.

@@ +413,2 @@
>          while (nfixed > nlivefixed)
> +            unaliasedLocal(--nfixed, DONT_CHECK_ALIASING).setMagic(JS_UNINITIALIZED_LET);

So this isn't consistent with Baseline's thing.  These two should do the same thing between them.

Erm.  So if these slots are garbage because the block objects got unwound enough that these can't be referred to, we should be able to init to either uninit/undefined.  Probably do uninit for slightly more niceness or so.

@@ +496,5 @@
>      InterpreterFrame *fp = reinterpret_cast<InterpreterFrame *>(buffer + 2 * sizeof(Value));
>      fp->mark_ = mark;
>      fp->initExecuteFrame(cx, script, evalInFrame, thisv, *scopeChain, type);
>      fp->initVarsToUndefined();
> +    fp->setLetsToThrowOnTouch();

This is the only call of initExecuteFrame.  So, similarly to how initCallFrame itself performs these two calls' effects, initExecuteFrame should as well.

Comment 170

[Shu-yu Guo [:shu]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7027efe7fae3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/03242a11d044
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8114e77c253e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/31714af41f2c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/687318d464a5
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fada58fb0996

Comment 171

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

This landed on m-c:

https://hg.mozilla.org/mozilla-central/rev/7027efe7fae3
https://hg.mozilla.org/mozilla-central/rev/03242a11d044
https://hg.mozilla.org/mozilla-central/rev/8114e77c253e
https://hg.mozilla.org/mozilla-central/rev/31714af41f2c
https://hg.mozilla.org/mozilla-central/rev/687318d464a5
https://hg.mozilla.org/mozilla-central/rev/fada58fb0996

Comment 172

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7027efe7fae3
https://hg.mozilla.org/mozilla-central/rev/03242a11d044
https://hg.mozilla.org/mozilla-central/rev/8114e77c253e
https://hg.mozilla.org/mozilla-central/rev/31714af41f2c
https://hg.mozilla.org/mozilla-central/rev/687318d464a5
https://hg.mozilla.org/mozilla-central/rev/fada58fb0996
https://hg.mozilla.org/mozilla-central/rev/4e8c6c5c0961

Comment 173

[Ed Morley [:emorley]]

Newsgroup post about this change:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/tezdW299Zds

Comment 174

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/2049d12583a7252dfb07c635cdd563b319a6c46f
Bug 1001090 - Part 4: Fix errors in chrome code. (r=zombie,gavin,fitzgen,dcamp,bgrins,fabrice,gwagner,margaret,mrbkap,mak,njn,vicamo)

Conflicts:
	lib/sdk/io/fs.js

Comment 175

[Florian Scholz (Open Web Docs)]

Release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/35#JavaScript

Reference pages:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/const
Especially
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let#Temporal_dead_zone_and_errors_with_let

As more and more people seem to be affected by this change, I have updated the docs for let and const. Also, on the let page I've put the non-standard extensions into a separate section, marked with a do not use banner.

A review is very much appreciated and needed if we want to point people to the correct docs when stumbling across this. Thanks!

Comment 176

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Shu-yu Guo [:shu] from comment #39)
> Comment on attachment 8471906 [details] [diff] [review]
> Part 4a: Fix parse errors in addon-sdk/
> 
> Review of attachment 8471906 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> See instructions in comment 37.

For future reference this commit did not resolve all of these issues in the `addon-sdk/`, and we have a red tree now, https://tbpl.mozilla.org/?tree=Jetpack

So now I have to put my work aside and comb thru fixing this bug.

Comment 177

[Shu-yu Guo [:shu]]

(In reply to Erik Vold [:erikvold] [:ztatic] (needinfo me if you have a question please) from comment #176)
> (In reply to Shu-yu Guo [:shu] from comment #39)
> > Comment on attachment 8471906 [details] [diff] [review]
> > Part 4a: Fix parse errors in addon-sdk/
> > 
> > Review of attachment 8471906 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > See instructions in comment 37.
> 
> For future reference this commit did not resolve all of these issues in the
> `addon-sdk/`, and we have a red tree now,
> https://tbpl.mozilla.org/?tree=Jetpack
> 
> So now I have to put my work aside and comb thru fixing this bug.

I fixed what broke mozilla-central -- why doesn't JP run on there?

Comment 178

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Shu-yu Guo [:shu] from comment #177)
> (In reply to Erik Vold [:erikvold] [:ztatic] (needinfo me if you have a
> question please) from comment #176)
> > (In reply to Shu-yu Guo [:shu] from comment #39)
> > > Comment on attachment 8471906 [details] [diff] [review]
> > > Part 4a: Fix parse errors in addon-sdk/
> > > 
> > > Review of attachment 8471906 [details] [diff] [review]:
> > > -----------------------------------------------------------------
> > > 
> > > See instructions in comment 37.
> > 
> > For future reference this commit did not resolve all of these issues in the
> > `addon-sdk/`, and we have a red tree now,
> > https://tbpl.mozilla.org/?tree=Jetpack
> > 
> > So now I have to put my work aside and comb thru fixing this bug.
> 
> I fixed what broke mozilla-central -- why doesn't JP run on there?

bug 1020473

Comment 179

[Rick Waldron [:rwaldron]]

(In reply to Shu-yu Guo [:shu] from comment #13)
> Things this bug do NOT fix:
> 
>  - Parsing consts as lets
>  - Scoping for the various for statements
>  - Global level lets

Are there bugs (or a bug) filed for these?

Comment 180

[Shu-yu Guo [:shu]]

(In reply to Rick Waldron [:rwaldron] from comment #179)
> (In reply to Shu-yu Guo [:shu] from comment #13)
> > Things this bug do NOT fix:
> > 
> >  - Parsing consts as lets
> >  - Scoping for the various for statements
> >  - Global level lets
> 
> Are there bugs (or a bug) filed for these?

Yes!

Consts as lets: bug 611388
Scoping for 'for' statements: bug 449811, bug 854037, bug 1069480 (possibly others, but those are what I could find right now)
Global lets: bug 589199

Comment 181

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Rick Waldron [:rwaldron] from comment #179)
> (In reply to Shu-yu Guo [:shu] from comment #13)
> > Things this bug do NOT fix:
> > 
> >  - Parsing consts as lets
> >  - Scoping for the various for statements
> >  - Global level lets
> 
> Are there bugs (or a bug) filed for these?

All would be blocking bug 694100 somewhere. Pro tip: use the dependency tree link, it's a lot easier than mousing over and waiting for tooltips (which I used to do) :)

Here are the bugs for fresh let bindings per c-style-for and ES6-style-for loops iteration, respectively:

https://bugzilla.mozilla.org/show_bug.cgi?id=854037
https://bugzilla.mozilla.org/show_bug.cgi?id=449811

Here is const: https://bugzilla.mozilla.org/show_bug.cgi?id=611388

Here is top level let: https://bugzilla.mozilla.org/show_bug.cgi?id=589199

Comment 182

[Florian Bender]

Release Note Request (optional, but appreciated)
[Why is this notable]: moar standardz
[Suggested wording]: Updated let & const implementation to ECMAScript 6 semantics
[Links (documentation, blog post, etc)]:

Comment 183

[Jeff Walden [:Waldo]]

(In reply to Florian Bender from comment #182)
> [Suggested wording]: Updated let & const implementation to ECMAScript 6
> semantics

Absolutely not this.  We moved *closer* to these semantics (but, only for let right now!).  But we did not update "to" those semantics, and we are still not completely compatible with ES6 on this point yet.

I'm out of time right now to come up with better wording, but I'll try to come up with something later today or maybe tomorrow morning.

Comment 184

[Marty]

This bug appears to have broken the Customize your Web addon. The developer has suspended all work so it's no longer useable, is there any way to restore it?

Comment 185

[Shu-yu Guo [:shu]]

(In reply to Marty from comment #184)
> This bug appears to have broken the Customize your Web addon. The developer
> has suspended all work so it's no longer useable, is there any way to
> restore it?

Ah, that's a rough situation. Are you up for perhaps fixing the addon yourself? The errors are usually trivial to fix as they're most likely redeclaration errors.

Comment 186

[Marty]

(In reply to Shu-yu Guo [:shu] from comment #185)
> Ah, that's a rough situation. Are you up for perhaps fixing the addon
> yourself? The errors are usually trivial to fix as they're most likely
> redeclaration errors.

It's no problem fixing the addon myself once I know what changes to make and where to make them. I've been trying to get the gist of what they might be but if you could point me in the right direction that would be a great help.

Comment 187

[Jeff Walden [:Waldo]]

Okay, here's better wording:

[Suggested wording]: Redeclaring existing variables or arguments within functions, using |let|, without doing so in a nested scope, is now a syntax error.  Use of a variable declared using |let| within a function, before its declaration is reached and evaluated, is now a runtime error.  Both changes have been made to conform to ES6 |let| semantics.

I have a feeling I might be glossing over some bit of semantic nitpickery in these one-sentence descriptions, but I can't see anything right now.

I'm still not quite happy with it on a wordsmithing level.  The mention of changing to be ES6 is a bit after-the-fact, when it might be better worked into each sentence directly.  Maybe two different change notes would be a better way to put it.  Not sure.  In any event, this wording is at least not semantically wrong any more.

Comment 188

[Shu-yu Guo [:shu]]

(In reply to Marty from comment #186)
> (In reply to Shu-yu Guo [:shu] from comment #185)
> > Ah, that's a rough situation. Are you up for perhaps fixing the addon
> > yourself? The errors are usually trivial to fix as they're most likely
> > redeclaration errors.
> 
> It's no problem fixing the addon myself once I know what changes to make and
> where to make them. I've been trying to get the gist of what they might be
> but if you could point me in the right direction that would be a great help.

I don't know much about addon development, so I'm afraid I won't be much help with specific steps. If you know how to show the errors that the addon is throwing, it should point you directly to the line of the redeclaration error. You can then fix those by renaming the conflicting binding or reusing the existing binding.

Comment 189

[Jeff Walden [:Waldo]]

I'd suggest:

1. Unzip the XPI file.
2. Unzip any .jar files within it.
3. For each error message you're hitting:
   a. Search all files for the file with that name.
   b. For each error, adjust declarations in the ways this bug's patches did.
4. Zip things back up into the same .jar/XPI structure as before, and install and use that addon.

That should be sufficient to fix an addon as you're given it.  Ideally you could find the original source code to the addon and modify that, rather than have to do this finicky zipping/unzipping thing.  But if that's not possible, that's how it goes.

Comment 190

[Shu-yu Guo [:shu]]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #187)
> Okay, here's better wording:
> 
> [Suggested wording]: Redeclaring existing variables or arguments within
> functions, using |let|, without doing so in a nested scope, is now a syntax
> error.  Use of a variable declared using |let| within a function, before its
> declaration is reached and evaluated, is now a runtime error.  Both changes
> have been made to conform to ES6 |let| semantics.
> 
> I have a feeling I might be glossing over some bit of semantic nitpickery in
> these one-sentence descriptions, but I can't see anything right now.
> 
> I'm still not quite happy with it on a wordsmithing level.  The mention of
> changing to be ES6 is a bit after-the-fact, when it might be better worked
> into each sentence directly.  Maybe two different change notes would be a
> better way to put it.  Not sure.  In any event, this wording is at least not
> semantically wrong any more.

Responding to call-to-bikeshed:

In conformance with ES6 |let| semantics, the following situations
now throw errors. Redeclaring existing variables or arguments
using |let| within the same scope in function bodies is now a
syntax error. Using a variable declared using |let| in function
bodies before the declaration is reached and evaluated is now a
runtime error.

Comment 191

[:Gavin Sharp [email: [e-mail]]]

Both of those are too specific for the release notes audience. Something like "Changed JavaScript 'let' and 'const' semantics [learn more link to MDN]" is more appropriate.

Comment 192

[Florian Scholz (Open Web Docs)]

We often point to MDN in release notes. We just need to make sure MDN is accurate, too.

I have used the wording in comment 190 for https://developer.mozilla.org/en-US/Firefox/Releases/35#JavaScript

Please review the let and const reference pages (mentioned in comment 175) as well to assure quality. Thanks! :)

Comment 193

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes with Gavin's wording "Changed JavaScript 'let' and 'const' semantics" and I used https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let#Temporal_dead_zone_and_errors_with_let as link.

Comment 194

[Jeff Walden [:Waldo]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #193)
> Added to the release notes with Gavin's wording "Changed JavaScript 'let'
> and 'const' semantics" and I used
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/
> let#Temporal_dead_zone_and_errors_with_let as link.

...wait.  We didn't change the semantics of |const|.  efaust is doing that in bug 611388, and it's not complete yet.  Could you remove const from there?  (And, uh, sorry for not keeping up with bugmail to see this earlier.  :-( )

Comment 195

[Sylvestre Ledru [:Sylvestre]]

OK. Updated "to Changed JavaScript 'let' semantics".
Thanks for the feedback

Comment 196

[Axel Grude [:realRaven]]

Here is a side effect (or maybe intended), I seem to have had this code in my Addon for ages:

obj.function() {
  {
    var msg='xxx';
    alert (msg);
  }
  ...
  let msg;
}
Of course I know that this was (always) wrong, I just didn't see it until running Fx35 beta. I would have EXPECTED this to throw an error way back (I have "use strict" in the module) because of var scope elevation. So it is a BIG surprise it is only caught now through this bugfix.

The other problem with this is that in the main window scope I do not get the specific error (with line number) but only "obj is undefined". I only found it because I reference the same from my options dialog. Also, the error is unspecific if I open a new Fx window. It would be nice if the specific error (line number, pointing to redeclaration) could be logged instead. Otherwise it can make it really hard to fix. And I would expect A LOT of addons breaking because of this - please somehow announce this warning all Add-on authors to regression test with the beta.

I have already removed most of my var declarations, but I will eliminate them completely now.