Implement ES6 "temporal dead zone" for let

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
5 years ago
a month ago

People

(Reporter: jorendorff, Assigned: shu)

Tracking

(Blocks 1 bug, {addon-compat, dev-doc-complete, site-compat})

unspecified
mozilla35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox35 fixed, relnote-firefox 35+)

Details

(Whiteboard: [DocArea=JS])

Attachments

(24 attachments, 91 obsolete attachments)

4.36 KB, patch
jimb
: review+
Details | Diff | Splinter Review
37.90 KB, patch
jandem
: review+
Details | Diff | Splinter Review
15.13 KB, patch
jandem
: review+
Details | Diff | Splinter Review
2.48 KB, patch
zombie
: review+
Details | Diff | Splinter Review
3.34 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
6.12 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
965 bytes, patch
fabrice
: review+
Details | Diff | Splinter Review
890 bytes, patch
gwagner
: review+
Details | Diff | Splinter Review
1.87 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
1.09 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
1.11 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
2.07 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
1.44 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
1.78 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
1.13 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1019 bytes, patch
Gavin
: review+
Details | Diff | Splinter Review
1.70 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
1.12 KB, patch
vicamo
: review+
Details | Diff | Splinter Review
383.12 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
1.26 KB, patch
Gavin
: review+
Details | Diff | Splinter Review
2.07 KB, patch
Margaret
: review+
Details | Diff | Splinter Review
1.21 KB, patch
mak
: review+
Details | Diff | Splinter Review
264.59 KB, patch
Details | Diff | Splinter Review
207.90 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

Comment 1

5 years ago
## 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
(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
(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]
(Reporter)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
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
(Assignee)

Comment 6

5 years ago
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;
  }
}
(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.
(Assignee)

Comment 8

5 years ago
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.
Attachment #8459125 - Flags: feedback?(luke)
Attachment #8459125 - Flags: feedback?(jorendorff)
Assignee: general → nobody
(Assignee)

Updated

5 years ago
Assignee: nobody → shu
Status: NEW → ASSIGNED
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.
Attachment #8459125 - Flags: feedback?(luke) → feedback+
(Assignee)

Comment 10

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

Not it!
(Assignee)

Comment 11

5 years ago
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.
Attachment #8459125 - Attachment is obsolete: true
Attachment #8459125 - Flags: feedback?(jorendorff)
Attachment #8468128 - Flags: review?(jorendorff)
(Assignee)

Comment 12

5 years ago
Cleaned up misc debug things I left in.
Attachment #8468128 - Attachment is obsolete: true
Attachment #8468128 - Flags: review?(jorendorff)
Attachment #8468131 - Flags: review?(jorendorff)
(Assignee)

Comment 13

5 years ago
Things this bug do NOT fix:

 - Parsing consts as lets
 - Scoping for the various for statements
 - Global level lets
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 15

5 years ago
(Assignee)

Comment 16

5 years ago
Attachment #8470401 - Attachment is obsolete: true
(Assignee)

Comment 17

5 years ago
Fix LazyScript::FreeVariable logic was wrong in BytecodeEmitter. The slot logic
was completely bogus and was marking upvars as hoisted let uses incorrectly.
Attachment #8468131 - Attachment is obsolete: true
Attachment #8468131 - Flags: review?(jorendorff)
Attachment #8470403 - Flags: review?(jorendorff)
(Assignee)

Comment 18

5 years ago
Had an inverted condition in codegen for LLetCheck.
Attachment #8470402 - Attachment is obsolete: true
(Assignee)

Comment 19

5 years ago
Jim, per our IRC conversation, changing UnwindScope uses where we want to
unwind to the outermost scope to not use a pc.
Attachment #8471204 - Flags: review?(jimb)
(Assignee)

Comment 20

5 years ago
Attachment #8470404 - Attachment is obsolete: true
(Assignee)

Comment 21

5 years ago
Attachment #8470400 - Attachment is obsolete: true
(Assignee)

Comment 36

5 years ago
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.
Attachment #8471205 - Flags: review?(jdemooij)
(Assignee)

Updated

5 years ago
Attachment #8471206 - Flags: review?(jdemooij)
(Assignee)

Comment 37

5 years ago
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.
(Assignee)

Comment 38

5 years ago
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.
Attachment #8471906 - Flags: review?(gps)
(Assignee)

Comment 39

5 years ago
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.
Attachment #8471906 - Flags: review?(gps) → review?(tomica+amo)
(Assignee)

Comment 40

5 years ago
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.
Attachment #8471908 - Flags: review?(nfitzgerald)
(Assignee)

Comment 41

5 years ago
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.
Attachment #8471910 - Flags: review?(anygregor)
(Assignee)

Comment 42

5 years ago
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.
Attachment #8471918 - Flags: review?(mrbkap)
Attachment #8471918 - Flags: review?(mrbkap) → review+
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.
Attachment #8471906 - Flags: review?(tomica+amo) → review+
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 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
Attachment #8471916 - Flags: review+
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
Attachment #8471917 - Flags: review+
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.
Attachment #8471913 - Flags: review+
Attachment #8471910 - Flags: review?(anygregor) → review+
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!)
Attachment #8471911 - Flags: review+

Comment 49

5 years ago
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?
Attachment #8471204 - Flags: review?(jimb) → review+
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).
Attachment #8471206 - Flags: review?(jdemooij) → review+
(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 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
Attachment #8471205 - Flags: review?(jdemooij) → review+
Attachment #8471908 - Flags: review?(nfitzgerald) → review+
(Assignee)

Comment 79

5 years ago
(Assignee)

Comment 86

5 years ago
(Assignee)

Comment 100

5 years ago
(Assignee)

Comment 101

5 years ago
(Assignee)

Comment 102

5 years ago
(Assignee)

Comment 103

5 years ago
(Assignee)

Comment 105

5 years ago
Attachment #8473284 - Attachment is obsolete: true
(Assignee)

Comment 106

5 years ago
Attachment #8473289 - Attachment is obsolete: true
(Assignee)

Comment 107

5 years ago
Attachment #8473290 - Attachment is obsolete: true
(Assignee)

Comment 109

5 years ago
Attachment #8473293 - Attachment is obsolete: true
(Assignee)

Comment 110

5 years ago
Attachment #8473302 - Attachment is obsolete: true
(Assignee)

Comment 111

5 years ago
Attachment #8473303 - Attachment is obsolete: true
(Assignee)

Comment 113

5 years ago
Attachment #8473315 - Attachment is obsolete: true
(Assignee)

Comment 114

5 years ago
Attachment #8473450 - Attachment is obsolete: true
(Assignee)

Comment 117

5 years ago
Consolidate all devtools test patches into one.
Attachment #8473276 - Attachment is obsolete: true
Attachment #8473277 - Attachment is obsolete: true
Attachment #8473278 - Attachment is obsolete: true
Attachment #8473279 - Attachment is obsolete: true
Attachment #8473280 - Attachment is obsolete: true
Attachment #8473281 - Attachment is obsolete: true
Attachment #8473282 - Attachment is obsolete: true
Attachment #8473283 - Attachment is obsolete: true
Attachment #8473285 - Attachment is obsolete: true
Attachment #8473286 - Attachment is obsolete: true
Attachment #8473287 - Attachment is obsolete: true
Attachment #8473288 - Attachment is obsolete: true
Attachment #8473446 - Attachment is obsolete: true
Attachment #8473447 - Attachment is obsolete: true
Attachment #8473448 - Attachment is obsolete: true
Attachment #8473449 - Attachment is obsolete: true
(Assignee)

Comment 118

5 years ago
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.
Attachment #8474037 - Flags: review?(rcampbell)
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!
Attachment #8474037 - Flags: review?(rcampbell) → review+
Landed a gaia patch to update let usage in fake_update-checker.js: https://github.com/mozilla-b2g/gaia/commit/8732cae00450205ae23e8118668d1531055eb6f1
(Assignee)

Comment 122

5 years ago
(Assignee)

Comment 123

5 years ago
(Assignee)

Comment 125

5 years ago
(Assignee)

Updated

5 years ago
Attachment #8475692 - Flags: review?(anygregor)
(Assignee)

Comment 127

5 years ago
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.
Flags: needinfo?(gavin.sharp)
(Assignee)

Comment 128

5 years ago
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
Flags: needinfo?(kgrandon)
(Assignee)

Comment 129

5 years ago
Consolidate all part 5 test patches into one for easier review. Gavin, if you
don't want to do it please assign somebody.
Attachment #8475688 - Attachment is obsolete: true
Attachment #8475689 - Attachment is obsolete: true
Attachment #8475690 - Attachment is obsolete: true
Attachment #8475691 - Attachment is obsolete: true
Attachment #8473269 - Attachment is obsolete: true
Attachment #8473270 - Attachment is obsolete: true
Attachment #8473272 - Attachment is obsolete: true
Attachment #8473273 - Attachment is obsolete: true
Attachment #8473274 - Attachment is obsolete: true
Attachment #8473275 - Attachment is obsolete: true
Attachment #8473291 - Attachment is obsolete: true
Attachment #8473292 - Attachment is obsolete: true
Attachment #8473294 - Attachment is obsolete: true
Attachment #8473295 - Attachment is obsolete: true
Attachment #8473296 - Attachment is obsolete: true
Attachment #8473297 - Attachment is obsolete: true
Attachment #8473298 - Attachment is obsolete: true
Attachment #8473299 - Attachment is obsolete: true
Attachment #8473300 - Attachment is obsolete: true
Attachment #8473301 - Attachment is obsolete: true
Attachment #8473304 - Attachment is obsolete: true
Attachment #8473305 - Attachment is obsolete: true
Attachment #8473306 - Attachment is obsolete: true
Attachment #8473307 - Attachment is obsolete: true
Attachment #8473308 - Attachment is obsolete: true
Attachment #8473309 - Attachment is obsolete: true
Attachment #8473311 - Attachment is obsolete: true
Attachment #8473312 - Attachment is obsolete: true
Attachment #8473313 - Attachment is obsolete: true
Attachment #8473314 - Attachment is obsolete: true
Attachment #8473316 - Attachment is obsolete: true
Attachment #8473441 - Attachment is obsolete: true
Attachment #8473442 - Attachment is obsolete: true
Attachment #8473443 - Attachment is obsolete: true
Attachment #8473444 - Attachment is obsolete: true
Attachment #8473445 - Attachment is obsolete: true
Attachment #8473452 - Attachment is obsolete: true
Attachment #8473453 - Attachment is obsolete: true
Attachment #8473455 - Attachment is obsolete: true
Attachment #8473456 - Attachment is obsolete: true
Attachment #8473980 - Attachment is obsolete: true
Attachment #8474035 - Attachment is obsolete: true
Attachment #8474036 - Attachment is obsolete: true
Attachment #8474037 - Attachment is obsolete: true
Attachment #8475687 - Attachment is obsolete: true
Attachment #8476190 - Flags: review?(gavin.sharp)
(Assignee)

Comment 130

5 years ago
Attachment #8476193 - Flags: review?(gavin.sharp)
(Assignee)

Comment 131

5 years ago
Please see comment 37 for instructions.
Attachment #8476194 - Flags: review?(margaret.leibovic)
(Assignee)

Updated

5 years ago
Flags: needinfo?(gavin.sharp)
(Assignee)

Updated

5 years ago
Attachment #8471909 - Flags: review?(fabrice)
(Assignee)

Comment 132

5 years ago
Rebased.
Attachment #8476196 - Flags: review?(jorendorff)
(Assignee)

Updated

5 years ago
Attachment #8470403 - Attachment is obsolete: true
Attachment #8470403 - Flags: review?(jorendorff)
(Assignee)

Updated

5 years ago
Attachment #8476199 - Flags: review?(mak77)
(Assignee)

Comment 134

5 years ago
Could you guys fuzz this? Applies cleanly to cbbc380f1e1c.
Attachment #8476211 - Flags: feedback?(gary)
Attachment #8476211 - Flags: feedback?(choller)
Attachment #8471909 - Flags: review?(fabrice) → review+
I've landed a gaia commit to update let usage of newTimerClassID: https://github.com/mozilla-b2g/gaia/commit/6c3c251ebd037710f370cda60b336b48fb4798be
Flags: needinfo?(kgrandon)
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.
Attachment #8476194 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Comment 137

5 years ago
(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. :(
(Assignee)

Comment 138

5 years ago
(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 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
Attachment #8476190 - Flags: review?(gavin.sharp) → review+
Comment on attachment 8476193 [details] [diff] [review]
Part 4: Fix errors in nsBrowserGlue.js

># HG changeset patch
># User Shu-yu Guo <shu@rfrn.org>
>
>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);
>   },
Attachment #8476193 - Flags: review?(gavin.sharp) → review+
Comment on attachment 8475692 [details] [diff] [review]
Part 4: Fix errors in MobileMessageDB.jsm

Fine with me but Vicamo is the owner here.
Attachment #8475692 - Flags: review?(anygregor) → review?(vyang)
Attachment #8475692 - Flags: review?(vyang) → review+
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
(Assignee)

Comment 142

5 years ago
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.
Attachment #8476211 - Attachment is obsolete: true
Attachment #8476211 - Flags: feedback?(gary)
Attachment #8476211 - Flags: feedback?(choller)
Attachment #8479387 - Flags: feedback?(gary)
Attachment #8479387 - Flags: feedback?(choller)
Depends on: 1059078
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
Attachment #8479387 - Flags: feedback?(gary) → feedback-
Flags: needinfo?(shu)
(Assignee)

Updated

5 years ago
Flags: needinfo?(shu)
(Assignee)

Comment 144

5 years ago
Posted patch Fuzzing rollup v2 (obsolete) — Splinter Review
Fixed round 1 of errors; let's go for round 2.
Attachment #8479387 - Attachment is obsolete: true
Attachment #8479387 - Flags: feedback?(choller)
Attachment #8480180 - Flags: feedback?(gary)
Attachment #8480180 - Flags: feedback?(choller)
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.
Flags: needinfo?(shu)
(Assignee)

Comment 146

5 years ago
(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 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)
Attachment #8480180 - Flags: feedback?(gary) → feedback-
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
(Assignee)

Comment 149

5 years ago
Incorporated fixes from the fuzzers
Attachment #8476196 - Attachment is obsolete: true
Attachment #8476196 - Flags: review?(jorendorff)
Attachment #8480847 - Flags: review?(jorendorff)
(Assignee)

Comment 150

5 years ago
Posted patch Fuzzing rollup v3 (obsolete) — Splinter Review
Round 3.

Christian, I don't get any compile errors, so I don't know what's going on with
your build.
Attachment #8480180 - Attachment is obsolete: true
Attachment #8480180 - Flags: feedback?(choller)
Attachment #8480848 - Flags: feedback?(gary)
Attachment #8480848 - Flags: feedback?(choller)
(Assignee)

Comment 151

5 years ago
(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.
Flags: needinfo?(shu)
(Assignee)

Comment 152

5 years ago
(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 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
Attachment #8480848 - Flags: feedback?(gary) → feedback-
Posted file debug and opt stacks (obsolete) —
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
(Assignee)

Comment 155

5 years ago
Posted patch Fuzzing rollup v4 (obsolete) — Splinter Review
Round 4. Applies cleanly to m-c d697d649c765
Attachment #8480848 - Attachment is obsolete: true
Attachment #8480848 - Flags: feedback?(choller)
Attachment #8481102 - Flags: feedback?(gary)
Attachment #8481102 - Flags: feedback?(choller)
Depends on: 1060353
Posted file assertion stack (top 18 lines) (obsolete) —
(function() {
    ((function() {
        p(y)
    })());
    let y
})()

Assertion failure: false (MOZ_ASSERT_UNREACHABLE: unexpected type), at jit/LIR.h
Attachment #8481047 - Attachment is obsolete: true
(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?
Flags: needinfo?(shu)
(Assignee)

Comment 158

5 years ago
(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
Flags: needinfo?(shu)
Posted file stack for second assertion (obsolete) —
{
    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.
Attachment #8481102 - Flags: feedback?(gary) → feedback-
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.
Attachment #8481102 - Flags: feedback?(choller) → feedback+
(Assignee)

Comment 161

5 years ago
More fuzz fixes.
Attachment #8480847 - Attachment is obsolete: true
Attachment #8480847 - Flags: review?(jorendorff)
(Assignee)

Comment 162

5 years ago
Posted patch Fuzzing rollup v5 (obsolete) — Splinter Review
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)
Attachment #8481102 - Attachment is obsolete: true
Attachment #8482894 - Flags: feedback?(gary)
(Assignee)

Comment 163

5 years ago
(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
(Assignee)

Comment 164

5 years ago
Posted patch Rollup v5 (obsolete) — Splinter Review
Applies cleanly to m-c 372ce1f36116
Attachment #8482894 - Attachment is obsolete: true
Attachment #8482894 - Flags: feedback?(gary)
Attachment #8483188 - Flags: feedback?(gary)
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
Attachment #8483188 - Flags: feedback?(gary) → feedback-
Attachment #8481750 - Attachment is obsolete: true
Attachment #8481774 - Attachment is obsolete: true
(Assignee)

Comment 166

5 years ago
Sigh.
Attachment #8483188 - Attachment is obsolete: true
Attachment #8483337 - Flags: feedback?(gary)
Attachment #8476199 - Flags: review?(mak77) → review+
(Assignee)

Updated

5 years ago
Attachment #8483338 - Flags: review?(jwalden+bmo)
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.
Attachment #8483337 - Flags: feedback?(gary) → feedback+
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.
Attachment #8483338 - Flags: review?(jwalden+bmo) → review+
Depends on: 1067805
Depends on: 1067850

Updated

5 years ago
Depends on: 1068176
Depends on: 1067969
Depends on: fix-let

Updated

5 years ago
Depends on: 1068508
Depends on: 1068668
Depends on: 1068756
Depends on: 1068790
Depends on: 1068075
Blocks: 1069049
Depends on: 1069190
Depends on: 1068621

Comment 174

5 years ago
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
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!
Depends on: 1070462
Depends on: 1070465
(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.
(Assignee)

Comment 177

5 years ago
(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?
(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
(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?
(Assignee)

Comment 180

5 years ago
(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
(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
Depends on: 1073702

Comment 182

5 years ago
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)]:
relnote-firefox: --- → ?
(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

5 years ago
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?
(Assignee)

Comment 185

5 years ago
(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

5 years ago
(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.
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.
(Assignee)

Comment 188

5 years ago
(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.
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.
(Assignee)

Comment 190

5 years ago
(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.
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.
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! :)
Depends on: 1076026
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.
Depends on: 1077949
Depends on: 1084194
(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.  :-( )
Summary: Implement ES6 "temporal dead zone" for let and const → Implement ES6 "temporal dead zone" for let
Flags: needinfo?(sledru)
OK. Updated "to Changed JavaScript 'let' semantics".
Thanks for the feedback
Flags: needinfo?(sledru)
Depends on: 1089761
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.
Depends on: 1108345
Depends on: 1111293

Updated

4 years ago
Depends on: 1153900

Updated

4 years ago
Depends on: 1154472
Depends on: 1155474
You need to log in before you can comment on or make changes to this bug.