Closed
Bug 1001090
Opened 11 years ago
Closed 10 years ago
Implement ES6 "temporal dead zone" for let
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla35
People
(Reporter: jorendorff, Assigned: shu)
References
(Blocks 1 open bug)
Details
(Keywords: addon-compat, dev-doc-complete, site-compat, Whiteboard: [DocArea=JS])
Attachments
(24 files, 91 obsolete files)
(deleted),
patch
|
jimb
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
zombie
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
fitzgen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
fabrice
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gwagner
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
vicamo
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Margaret
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
gkw
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
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•11 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
Comment 2•11 years ago
|
||
(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•11 years ago
|
||
(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•11 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.
Updated•10 years ago
|
Blocks: harmony:modules
Assignee | ||
Comment 5•10 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•10 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;
}
}
Comment 7•10 years ago
|
||
(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•10 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)
Updated•10 years ago
|
Assignee: general → nobody
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → shu
Status: NEW → ASSIGNED
Comment 9•10 years ago
|
||
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•10 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•10 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•10 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•10 years ago
|
||
Things this bug do NOT fix:
- Parsing consts as lets
- Scoping for the various for statements
- Global level lets
Updated•10 years ago
|
Flags: needinfo?(jwalden+bmo)
Assignee | ||
Comment 14•10 years ago
|
||
Assignee | ||
Comment 15•10 years ago
|
||
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8470401 -
Attachment is obsolete: true
Assignee | ||
Comment 17•10 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•10 years ago
|
||
Had an inverted condition in codegen for LLetCheck.
Attachment #8470402 -
Attachment is obsolete: true
Assignee | ||
Comment 19•10 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•10 years ago
|
||
Attachment #8470404 -
Attachment is obsolete: true
Assignee | ||
Comment 21•10 years ago
|
||
Attachment #8470400 -
Attachment is obsolete: true
Assignee | ||
Comment 22•10 years ago
|
||
Assignee | ||
Comment 23•10 years ago
|
||
Assignee | ||
Comment 24•10 years ago
|
||
Assignee | ||
Comment 25•10 years ago
|
||
Assignee | ||
Comment 26•10 years ago
|
||
Assignee | ||
Comment 27•10 years ago
|
||
Assignee | ||
Comment 28•10 years ago
|
||
Assignee | ||
Comment 29•10 years ago
|
||
Assignee | ||
Comment 30•10 years ago
|
||
Assignee | ||
Comment 31•10 years ago
|
||
Assignee | ||
Comment 32•10 years ago
|
||
Assignee | ||
Comment 33•10 years ago
|
||
Assignee | ||
Comment 34•10 years ago
|
||
Assignee | ||
Comment 35•10 years ago
|
||
Assignee | ||
Comment 36•10 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•10 years ago
|
Attachment #8471206 -
Flags: review?(jdemooij)
Assignee | ||
Comment 37•10 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•10 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•10 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•10 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•10 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•10 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)
Updated•10 years ago
|
Attachment #8471918 -
Flags: review?(mrbkap) → review+
Comment 43•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8471907 -
Flags: review+
Updated•10 years ago
|
Attachment #8471919 -
Flags: review+
Updated•10 years ago
|
Attachment #8471920 -
Flags: review+
Comment 44•10 years ago
|
||
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"?
Updated•10 years ago
|
Attachment #8471915 -
Flags: review+
Updated•10 years ago
|
Attachment #8471914 -
Flags: review+
Comment 45•10 years ago
|
||
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 46•10 years ago
|
||
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 47•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8471910 -
Flags: review?(anygregor) → review+
Comment 48•10 years ago
|
||
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•10 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 50•10 years ago
|
||
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+
Comment 51•10 years ago
|
||
(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•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8471908 -
Flags: review?(nfitzgerald) → review+
Assignee | ||
Comment 53•10 years ago
|
||
Assignee | ||
Comment 54•10 years ago
|
||
Assignee | ||
Comment 55•10 years ago
|
||
Assignee | ||
Comment 56•10 years ago
|
||
Assignee | ||
Comment 57•10 years ago
|
||
Assignee | ||
Comment 58•10 years ago
|
||
Assignee | ||
Comment 59•10 years ago
|
||
Assignee | ||
Comment 60•10 years ago
|
||
Assignee | ||
Comment 61•10 years ago
|
||
Assignee | ||
Comment 62•10 years ago
|
||
Assignee | ||
Comment 63•10 years ago
|
||
Assignee | ||
Comment 64•10 years ago
|
||
Assignee | ||
Comment 65•10 years ago
|
||
Assignee | ||
Comment 66•10 years ago
|
||
Assignee | ||
Comment 67•10 years ago
|
||
Assignee | ||
Comment 68•10 years ago
|
||
Assignee | ||
Comment 69•10 years ago
|
||
Assignee | ||
Comment 70•10 years ago
|
||
Assignee | ||
Comment 71•10 years ago
|
||
Assignee | ||
Comment 72•10 years ago
|
||
Assignee | ||
Comment 73•10 years ago
|
||
Assignee | ||
Comment 74•10 years ago
|
||
Assignee | ||
Comment 75•10 years ago
|
||
Assignee | ||
Comment 76•10 years ago
|
||
Assignee | ||
Comment 77•10 years ago
|
||
Assignee | ||
Comment 78•10 years ago
|
||
Assignee | ||
Comment 79•10 years ago
|
||
Assignee | ||
Comment 80•10 years ago
|
||
Assignee | ||
Comment 81•10 years ago
|
||
Assignee | ||
Comment 82•10 years ago
|
||
Assignee | ||
Comment 83•10 years ago
|
||
Assignee | ||
Comment 84•10 years ago
|
||
Assignee | ||
Comment 85•10 years ago
|
||
Assignee | ||
Comment 86•10 years ago
|
||
Assignee | ||
Comment 87•10 years ago
|
||
Assignee | ||
Comment 88•10 years ago
|
||
Assignee | ||
Comment 89•10 years ago
|
||
Assignee | ||
Comment 90•10 years ago
|
||
Assignee | ||
Comment 91•10 years ago
|
||
Assignee | ||
Comment 92•10 years ago
|
||
Assignee | ||
Comment 93•10 years ago
|
||
Assignee | ||
Comment 94•10 years ago
|
||
Assignee | ||
Comment 95•10 years ago
|
||
Assignee | ||
Comment 96•10 years ago
|
||
Assignee | ||
Comment 97•10 years ago
|
||
Assignee | ||
Comment 98•10 years ago
|
||
Assignee | ||
Comment 99•10 years ago
|
||
Assignee | ||
Comment 100•10 years ago
|
||
Assignee | ||
Comment 101•10 years ago
|
||
Assignee | ||
Comment 102•10 years ago
|
||
Assignee | ||
Comment 103•10 years ago
|
||
Assignee | ||
Comment 104•10 years ago
|
||
Assignee | ||
Comment 105•10 years ago
|
||
Attachment #8473284 -
Attachment is obsolete: true
Assignee | ||
Comment 106•10 years ago
|
||
Attachment #8473289 -
Attachment is obsolete: true
Assignee | ||
Comment 107•10 years ago
|
||
Attachment #8473290 -
Attachment is obsolete: true
Assignee | ||
Comment 108•10 years ago
|
||
Assignee | ||
Comment 109•10 years ago
|
||
Attachment #8473293 -
Attachment is obsolete: true
Assignee | ||
Comment 110•10 years ago
|
||
Attachment #8473302 -
Attachment is obsolete: true
Assignee | ||
Comment 111•10 years ago
|
||
Attachment #8473303 -
Attachment is obsolete: true
Assignee | ||
Comment 112•10 years ago
|
||
Attachment #8473310 -
Attachment is obsolete: true
Assignee | ||
Comment 113•10 years ago
|
||
Attachment #8473315 -
Attachment is obsolete: true
Assignee | ||
Comment 114•10 years ago
|
||
Attachment #8473450 -
Attachment is obsolete: true
Assignee | ||
Comment 115•10 years ago
|
||
Assignee | ||
Comment 116•10 years ago
|
||
Assignee | ||
Comment 117•10 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•10 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 119•10 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]:
-----------------------------------------------------------------
with pleasure. All hail the Temporal Dead Zone!
Attachment #8474037 -
Flags: review?(rcampbell) → review+
Comment 120•10 years ago
|
||
Landed a gaia patch to update let usage in fake_update-checker.js: https://github.com/mozilla-b2g/gaia/commit/8732cae00450205ae23e8118668d1531055eb6f1
Assignee | ||
Comment 121•10 years ago
|
||
Assignee | ||
Comment 122•10 years ago
|
||
Assignee | ||
Comment 123•10 years ago
|
||
Assignee | ||
Comment 124•10 years ago
|
||
Assignee | ||
Comment 125•10 years ago
|
||
Assignee | ||
Comment 126•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8475692 -
Flags: review?(anygregor)
Assignee | ||
Comment 127•10 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•10 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•10 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 #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 #8475688 -
Attachment is obsolete: true
Attachment #8475689 -
Attachment is obsolete: true
Attachment #8475690 -
Attachment is obsolete: true
Attachment #8475691 -
Attachment is obsolete: true
Attachment #8476190 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 130•10 years ago
|
||
Attachment #8476193 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 131•10 years ago
|
||
Please see comment 37 for instructions.
Attachment #8476194 -
Flags: review?(margaret.leibovic)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(gavin.sharp)
Assignee | ||
Updated•10 years ago
|
Attachment #8471909 -
Flags: review?(fabrice)
Assignee | ||
Updated•10 years ago
|
Attachment #8470403 -
Attachment is obsolete: true
Attachment #8470403 -
Flags: review?(jorendorff)
Assignee | ||
Comment 133•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8476199 -
Flags: review?(mak77)
Assignee | ||
Comment 134•10 years ago
|
||
Could you guys fuzz this? Applies cleanly to cbbc380f1e1c.
Attachment #8476211 -
Flags: feedback?(gary)
Attachment #8476211 -
Flags: feedback?(choller)
Updated•10 years ago
|
Attachment #8471909 -
Flags: review?(fabrice) → review+
Comment 135•10 years ago
|
||
I've landed a gaia commit to update let usage of newTimerClassID: https://github.com/mozilla-b2g/gaia/commit/6c3c251ebd037710f370cda60b336b48fb4798be
Flags: needinfo?(kgrandon)
Comment 136•10 years ago
|
||
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•10 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•10 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 139•10 years ago
|
||
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 140•10 years ago
|
||
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 141•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8475692 -
Flags: review?(vyang) → review+
Updated•10 years ago
|
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Assignee | ||
Comment 142•10 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)
Comment 143•10 years ago
|
||
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•10 years ago
|
Flags: needinfo?(shu)
Assignee | ||
Comment 144•10 years ago
|
||
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)
Comment 145•10 years ago
|
||
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•10 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 147•10 years ago
|
||
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-
Comment 148•10 years ago
|
||
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•10 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•10 years ago
|
||
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•10 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•10 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 153•10 years ago
|
||
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-
Comment 154•10 years ago
|
||
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•10 years ago
|
||
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)
Comment 156•10 years ago
|
||
(function() {
((function() {
p(y)
})());
let y
})()
Assertion failure: false (MOZ_ASSERT_UNREACHABLE: unexpected type), at jit/LIR.h
Attachment #8481047 -
Attachment is obsolete: true
Comment 157•10 years ago
|
||
(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•10 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)
Comment 159•10 years ago
|
||
{
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.
Updated•10 years ago
|
Attachment #8481102 -
Flags: feedback?(gary) → feedback-
Comment 160•10 years ago
|
||
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•10 years ago
|
||
More fuzz fixes.
Attachment #8480847 -
Attachment is obsolete: true
Attachment #8480847 -
Flags: review?(jorendorff)
Assignee | ||
Comment 162•10 years ago
|
||
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•10 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•10 years ago
|
||
Applies cleanly to m-c 372ce1f36116
Attachment #8482894 -
Attachment is obsolete: true
Attachment #8482894 -
Flags: feedback?(gary)
Attachment #8483188 -
Flags: feedback?(gary)
Comment 165•10 years ago
|
||
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-
Updated•10 years ago
|
Attachment #8481750 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8481774 -
Attachment is obsolete: true
Assignee | ||
Comment 166•10 years ago
|
||
Sigh.
Attachment #8483188 -
Attachment is obsolete: true
Attachment #8483337 -
Flags: feedback?(gary)
Assignee | ||
Comment 167•10 years ago
|
||
Attachment #8482862 -
Attachment is obsolete: true
Updated•10 years ago
|
Attachment #8476199 -
Flags: review?(mak77) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8483338 -
Flags: review?(jwalden+bmo)
Comment 168•10 years ago
|
||
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 169•10 years ago
|
||
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+
Assignee | ||
Comment 170•10 years ago
|
||
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
Flags: needinfo?(jwalden+bmo)
Comment 171•10 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox35:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 172•10 years ago
|
||
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
Updated•10 years ago
|
Keywords: addon-compat
Comment 173•10 years ago
|
||
Newsgroup post about this change:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/tezdW299Zds
Comment 174•10 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
Comment 175•10 years ago
|
||
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!
Keywords: dev-doc-needed → dev-doc-complete
Updated•10 years ago
|
Comment 176•10 years ago
|
||
(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•10 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?
Comment 178•10 years ago
|
||
(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•10 years ago
|
||
(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•10 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
Comment 181•10 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?
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•10 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:
--- → ?
Comment 183•10 years ago
|
||
(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•10 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•10 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•10 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.
Comment 187•10 years ago
|
||
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•10 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.
Comment 189•10 years ago
|
||
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•10 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.
Comment 191•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
(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
Updated•10 years ago
|
Flags: needinfo?(sledru)
Comment 195•10 years ago
|
||
OK. Updated "to Changed JavaScript 'let' semantics".
Thanks for the feedback
Flags: needinfo?(sledru)
Updated•10 years ago
|
Keywords: dev-doc-needed → dev-doc-complete
Comment 196•10 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•