Address PR feedback

Ian Halliday · Ian Halliday · commit 008cc686fc13 · 2016-05-09T14:00:34.000-07:00
- Added helpful comments in a couple places
 - Made EmitUseBeforeDeclaration void instead of bool
 - Remove knopName case from EmitDestructuredElement
 - Fixed missing calls to new tests in tdz2.js
diff --git a/lib/Runtime/ByteCode/ByteCodeEmitter.cpp b/lib/Runtime/ByteCode/ByteCodeEmitter.cpp
@@ -12,7 +12,7 @@ void EmitLoad(ParseNode *rhs, ByteCodeGenerator *byteCodeGenerator, FuncInfo *fu
 void EmitCall(ParseNode* pnode, Js::RegSlot rhsLocation, ByteCodeGenerator* byteCodeGenerator, FuncInfo* funcInfo, BOOL fReturnValue, BOOL fEvaluateComponents, BOOL fHasNewTarget, Js::RegSlot overrideThisLocation = Js::Constants::NoRegister);
 void EmitSuperFieldPatch(FuncInfo* funcInfo, ParseNode* pnode, ByteCodeGenerator* byteCodeGenerator);
 
-bool EmitUseBeforeDeclaration(Symbol *sym, ByteCodeGenerator *byteCodeGenerator, FuncInfo *funcInfo);
+void EmitUseBeforeDeclaration(Symbol *sym, ByteCodeGenerator *byteCodeGenerator, FuncInfo *funcInfo);
 void EmitUseBeforeDeclarationRuntimeError(ByteCodeGenerator *byteCodeGenerator, Js::RegSlot location);
 void VisitClearTmpRegs(ParseNode * pnode, ByteCodeGenerator * byteCodeGenerator, FuncInfo * funcInfo);
 
@@ -4883,6 +4883,12 @@ void ByteCodeGenerator::EmitPropStore(Js::RegSlot rhsLocation, Symbol *sym, Iden
     if (sym && sym->GetNeedDeclaration() && scope->GetFunc() == funcInfo)
     {
         EmitUseBeforeDeclarationRuntimeError(this, Js::Constants::NoRegister);
+        // Intentionally continue on to do normal EmitPropStore behavior so
+        // that the bytecode ends up well-formed for the backend.  This is
+        // in contrast to EmitPropLoad and EmitPropTypeof where they both
+        // tell EmitUseBeforeDeclarationRuntimeError to emit a LdUndef in place
+        // of their load and then they skip emitting their own bytecode.
+        // Potayto potahto.
     }
 
     if (sym == nullptr || sym->GetIsGlobal())
@@ -5991,6 +5997,9 @@ void EmitReference(ParseNode *pnode, ByteCodeGenerator *byteCodeGenerator, FuncI
             }
             else
             {
+                // EmitLoad will check for needsDeclaration and emit the Use Before Declaration error
+                // bytecode op as necessary, but EmitReference does not check this (by design). So we
+                // must manually check here.
                 EmitUseBeforeDeclaration(pnode->sxCall.pnodeTarget->sxPid.sym, byteCodeGenerator, funcInfo);
                 EmitReference(pnode->sxCall.pnodeTarget, byteCodeGenerator, funcInfo);
             }
@@ -6036,9 +6045,6 @@ void EmitDestructuredElement(ParseNode *elem, Js::RegSlot sourceLocation, ByteCo
         elem->sxVar.sym->SetNeedDeclaration(false);
         break;
 
-    case knopName:
-        break;
-
     default:
         EmitReference(elem, byteCodeGenerator, funcInfo);
     }
@@ -8862,7 +8868,7 @@ void EmitUseBeforeDeclarationRuntimeError(ByteCodeGenerator * byteCodeGenerator,
     }
 }
 
-bool EmitUseBeforeDeclaration(Symbol *sym, ByteCodeGenerator *byteCodeGenerator, FuncInfo *funcInfo)
+void EmitUseBeforeDeclaration(Symbol *sym, ByteCodeGenerator *byteCodeGenerator, FuncInfo *funcInfo)
 {
     // Don't emit static use-before-declaration error in a closure or dynamic scope case. We detect such cases with dynamic checks,
     // if necessary.
@@ -8873,10 +8879,7 @@ bool EmitUseBeforeDeclaration(Symbol *sym, ByteCodeGenerator *byteCodeGenerator,
         sym->GetScope()->GetFunc() == funcInfo)
     {
         EmitUseBeforeDeclarationRuntimeError(byteCodeGenerator, Js::Constants::NoRegister);
-        return true;
     }
-
-    return false;
 }
 
 void EmitBinary(Js::OpCode opcode, ParseNode *pnode, ByteCodeGenerator *byteCodeGenerator, FuncInfo *funcInfo)
diff --git a/test/LetConst/tdz2.js b/test/LetConst/tdz2.js
@@ -73,6 +73,7 @@ function test2() {
     }
     WScript.Echo('test2 failed to throw TDZ error in f()');
 }
+test2();
 
 function test3() {
     // this used to assert before #505 was fixed; arguments expression
@@ -94,6 +95,7 @@ function test3() {
     }
     WScript.Echo('fail');
 }
+test3();
 
 function test4() {
     // this case come from jsfunfuzz and is the smallest I could

Original file line number	Diff line number	Diff line change
`@@ -12,7 +12,7 @@ void EmitLoad(ParseNode rhs, ByteCodeGenerator byteCodeGenerator, FuncInfo *fu`
`12`	`12`	`void EmitCall(ParseNode* pnode, Js::RegSlot rhsLocation, ByteCodeGenerator* byteCodeGenerator, FuncInfo* funcInfo, BOOL fReturnValue, BOOL fEvaluateComponents, BOOL fHasNewTarget, Js::RegSlot overrideThisLocation = Js::Constants::NoRegister);`
`13`	`13`	`void EmitSuperFieldPatch(FuncInfo* funcInfo, ParseNode* pnode, ByteCodeGenerator* byteCodeGenerator);`
`14`	`14`
`15`		`-bool EmitUseBeforeDeclaration(Symbol sym, ByteCodeGenerator byteCodeGenerator, FuncInfo *funcInfo);`
	`15`	`+void EmitUseBeforeDeclaration(Symbol sym, ByteCodeGenerator byteCodeGenerator, FuncInfo *funcInfo);`
`16`	`16`	`void EmitUseBeforeDeclarationRuntimeError(ByteCodeGenerator *byteCodeGenerator, Js::RegSlot location);`
`17`	`17`	`void VisitClearTmpRegs(ParseNode * pnode, ByteCodeGenerator * byteCodeGenerator, FuncInfo * funcInfo);`
`18`	`18`
`@@ -4883,6 +4883,12 @@ void ByteCodeGenerator::EmitPropStore(Js::RegSlot rhsLocation, Symbol *sym, Iden`
`4883`	`4883`	`if (sym && sym->GetNeedDeclaration() && scope->GetFunc() == funcInfo)`
`4884`	`4884`	`{`
`4885`	`4885`	`EmitUseBeforeDeclarationRuntimeError(this, Js::Constants::NoRegister);`
	`4886`	`+ // Intentionally continue on to do normal EmitPropStore behavior so`
	`4887`	`+ // that the bytecode ends up well-formed for the backend. This is`
	`4888`	`+ // in contrast to EmitPropLoad and EmitPropTypeof where they both`
	`4889`	`+ // tell EmitUseBeforeDeclarationRuntimeError to emit a LdUndef in place`
	`4890`	`+ // of their load and then they skip emitting their own bytecode.`
	`4891`	`+ // Potayto potahto.`
`4886`	`4892`	`}`
`4887`	`4893`
`4888`	`4894`	`if (sym == nullptr \|\| sym->GetIsGlobal())`
`@@ -5991,6 +5997,9 @@ void EmitReference(ParseNode pnode, ByteCodeGenerator byteCodeGenerator, FuncI`
`5991`	`5997`	`}`
`5992`	`5998`	`else`
`5993`	`5999`	`{`
	`6000`	`+ // EmitLoad will check for needsDeclaration and emit the Use Before Declaration error`
	`6001`	`+ // bytecode op as necessary, but EmitReference does not check this (by design). So we`
	`6002`	`+ // must manually check here.`
`5994`	`6003`	`EmitUseBeforeDeclaration(pnode->sxCall.pnodeTarget->sxPid.sym, byteCodeGenerator, funcInfo);`
`5995`	`6004`	`EmitReference(pnode->sxCall.pnodeTarget, byteCodeGenerator, funcInfo);`
`5996`	`6005`	`}`
`@@ -6036,9 +6045,6 @@ void EmitDestructuredElement(ParseNode *elem, Js::RegSlot sourceLocation, ByteCo`
`6036`	`6045`	`elem->sxVar.sym->SetNeedDeclaration(false);`
`6037`	`6046`	`break;`
`6038`	`6047`
`6039`		`- case knopName:`
`6040`		`- break;`
`6041`		`-`
`6042`	`6048`	`default:`
`6043`	`6049`	`EmitReference(elem, byteCodeGenerator, funcInfo);`
`6044`	`6050`	`}`
`@@ -8862,7 +8868,7 @@ void EmitUseBeforeDeclarationRuntimeError(ByteCodeGenerator * byteCodeGenerator,`
`8862`	`8868`	`}`
`8863`	`8869`	`}`
`8864`	`8870`
`8865`		`-bool EmitUseBeforeDeclaration(Symbol sym, ByteCodeGenerator byteCodeGenerator, FuncInfo *funcInfo)`
	`8871`	`+void EmitUseBeforeDeclaration(Symbol sym, ByteCodeGenerator byteCodeGenerator, FuncInfo *funcInfo)`
`8866`	`8872`	`{`
`8867`	`8873`	`// Don't emit static use-before-declaration error in a closure or dynamic scope case. We detect such cases with dynamic checks,`
`8868`	`8874`	`// if necessary.`
`@@ -8873,10 +8879,7 @@ bool EmitUseBeforeDeclaration(Symbol sym, ByteCodeGenerator byteCodeGenerator,`
`8873`	`8879`	`sym->GetScope()->GetFunc() == funcInfo)`
`8874`	`8880`	`{`
`8875`	`8881`	`EmitUseBeforeDeclarationRuntimeError(byteCodeGenerator, Js::Constants::NoRegister);`
`8876`		`- return true;`
`8877`	`8882`	`}`
`8878`		`-`
`8879`		`- return false;`
`8880`	`8883`	`}`
`8881`	`8884`
`8882`	`8885`	`void EmitBinary(Js::OpCode opcode, ParseNode pnode, ByteCodeGenerator byteCodeGenerator, FuncInfo *funcInfo)`
Original file line number	Diff line number	Diff line change
`@@ -73,6 +73,7 @@ function test2() {`
`73`	`73`	`}`
`74`	`74`	`WScript.Echo('test2 failed to throw TDZ error in f()');`
`75`	`75`	`}`
	`76`	`+test2();`
`76`	`77`
`77`	`78`	`function test3() {`
`78`	`79`	`// this used to assert before #505 was fixed; arguments expression`
`@@ -94,6 +95,7 @@ function test3() {`
`94`	`95`	`}`
`95`	`96`	`WScript.Echo('fail');`
`96`	`97`	`}`
	`98`	`+test3();`
`97`	`99`
`98`	`100`	`function test4() {`
`99`	`101`	`// this case come from jsfunfuzz and is the smallest I could`