Fixes async function parameter redeclaration cases

The parameters of an async function were disconnected from the body in
terms of redeclaration semantics since async functions are implemented
using a parser syntax tree transformation where the body is placed
inside an inner generator function expression.  So the parameters became
related to the body as the parameters of the parent function, available
for capture, but not interacting with local bindings of the same name.

This is fixed by placing PidRefs to the parameter symbols into the
parameter scope of the inner generator function.  These refs are seen
through the normal binding mechanism and interact as though they were
local parameter symbols, but they point at the outer function's
parameter symbols so that the actual binding for bytecode generation
correctly sees them as coming from the outer function.

This required touching up the bytecode generator in the case of a local
function declaration whose name matches a parameter name due to
assumptions made that local function declaration symbols will always be
local to the enclosing function.  This is no longer true in the async
function case.

lib/Parser/parse.cpp

@@ -6756,6 +6756,16 @@ void Parser::TransformAsyncFncDeclAST(ParseNodePtr *pnodeBody, bool fLambda)
     ppnodeExprScopeSave = m_ppnodeExprScope;
     m_ppnodeExprScope = nullptr;
 
+    // Push the formal parameter symbols again for the inner generator to get proper
+    // redeclaration semantics (error for let/const locals, merge for var locals)
+    Scope* paramScope = pnodeFncSave->sxFnc.pnodeScopes->sxBlock.scope;
+    paramScope->ForEachSymbol([this](Symbol* paramSym)
+    {
+        Symbol* sym = paramSym->GetPid()->GetTopRef()->GetSym();
+        PidRefStack* ref = PushPidRef(paramSym->GetPid());
+        ref->SetSym(sym);
+    });
+
     pnodeInnerBlock = StartParseBlock<true>(PnodeBlockType::Function, ScopeType_FunctionBody);
     *m_ppnodeScope = pnodeInnerBlock;
     pnodeFncGenerator->sxFnc.pnodeBodyScope = pnodeInnerBlock;

lib/Runtime/ByteCode/ByteCodeEmitter.cpp

@@ -1159,7 +1159,15 @@ void EmitAssignmentToFuncName(ParseNode *pnodeFnc, ByteCodeGenerator *byteCodeGe
                 }
             }
 
-            byteCodeGenerator->EmitLocalPropInit(pnodeFnc->location, sym, funcInfoParent);
+            if (sym->GetScope()->GetFunc() != byteCodeGenerator->TopFuncInfo())
+            {
+                byteCodeGenerator->EmitPropStore(pnodeFnc->location, sym, nullptr, funcInfoParent);
+            }
+            else
+            {
+                byteCodeGenerator->EmitLocalPropInit(pnodeFnc->location, sym, funcInfoParent);
+            }
+
             Symbol * fncScopeSym = sym->GetFuncScopeVarSym();
 
             if (fncScopeSym)
@@ -1253,11 +1261,8 @@ Js::RegSlot ByteCodeGenerator::DefineOneFunction(ParseNode *pnodeFnc, FuncInfo *
                 // Note that having a function with the same name as a let/const declaration
                 // is a redeclaration error, but we're pushing the fix for this out since it's
                 // a bit involved.
-                //
-                // TODO: Once the redeclaration error is in place, this boolean can be replaced
-                // by funcSymbol->GetIsBlockVar().
                 Assert(funcInfoParent->GetBodyScope() != nullptr && funcSymbol->GetScope() != nullptr);
-                bool isFunctionDeclarationInBlock = funcSymbol->GetScope() != funcInfoParent->GetBodyScope();
+                bool isFunctionDeclarationInBlock = funcSymbol->GetIsBlockVar();
 
                 // Track all vars/lets/consts register slot function declarations.
                 if (ShouldTrackDebuggerMetadata()
@@ -4481,11 +4486,24 @@ void ByteCodeGenerator::EmitPropStore(Js::RegSlot rhsLocation, Symbol *sym, Iden
     // isFncDeclVar denotes that the symbol being stored to here is the var
     // binding of a function declaration and we know we want to store directly
     // to it, skipping over any dynamic scopes that may lie in between.
-    Scope *scope = isFncDeclVar ? symScope : nullptr;
-    Js::RegSlot scopeLocation = isFncDeclVar ? scope->GetLocation() : Js::Constants::NoRegister;
+    Scope *scope = nullptr;
+    Js::RegSlot scopeLocation = Js::Constants::NoRegister;
     bool scopeAcquired = false;
     Js::OpCode op;
 
+    if (isFncDeclVar)
+    {
+        // async functions allow for the fncDeclVar to be in the body or parameter scope
+        // of the parent function, so we need to calculate envIndex in lieu of the while
+        // loop below.
+        do
+        {
+            scope = this->FindScopeForSym(symScope, scope, &envIndex, funcInfo);
+        } while (scope != symScope);
+        Assert(scope == symScope);
+        scopeLocation = scope->GetLocation();
+    }
+
     while (!isFncDeclVar)
     {
         scope = this->FindScopeForSym(symScope, scope, &envIndex, funcInfo);

lib/Runtime/ByteCode/ByteCodeGenerator.cpp

@@ -1800,6 +1800,19 @@ FuncInfo *ByteCodeGenerator::FindEnclosingNonLambda()
     return nullptr;
 }
 
+FuncInfo* GetParentFuncInfo(FuncInfo* child)
+{
+    for (Scope* scope = child->GetBodyScope(); scope; scope = scope->GetEnclosingScope())
+    {
+        if (scope->GetFunc() != child)
+        {
+            return scope->GetFunc();
+        }
+    }
+    Assert(0);
+    return nullptr;
+}
+
 bool ByteCodeGenerator::CanStackNestedFunc(FuncInfo * funcInfo, bool trace)
 {
 #if ENABLE_DEBUG_CONFIG_OPTIONS
@@ -2530,7 +2543,16 @@ void AssignFuncSymRegister(ParseNode * pnode, ByteCodeGenerator * byteCodeGenera
                 byteCodeGenerator->AssignRegister(sym);
                 pnode->location = sym->GetLocation();
 
-                Assert(byteCodeGenerator->GetCurrentScope()->GetFunc() == sym->GetScope()->GetFunc());
+                Assert(byteCodeGenerator->GetCurrentScope()->GetFunc() == sym->GetScope()->GetFunc() ||
+                    sym->GetScope()->GetFunc()->root->sxFnc.IsAsync());
+                if (byteCodeGenerator->GetCurrentScope()->GetFunc() != sym->GetScope()->GetFunc())
+                {
+                    Assert(GetParentFuncInfo(byteCodeGenerator->GetCurrentScope()->GetFunc()) == sym->GetScope()->GetFunc());
+                    sym->GetScope()->SetMustInstantiate(true);
+                    sym->SetHasNonLocalReference(true, byteCodeGenerator);
+                    sym->GetScope()->GetFunc()->SetHasLocalInClosure(true);
+                }
+
                 Symbol * functionScopeVarSym = sym->GetFuncScopeVarSym();
                 if (functionScopeVarSym &&
                     !functionScopeVarSym->GetIsGlobal() &&
@@ -4937,6 +4959,19 @@ void AssignRegisters(ParseNode *pnode, ByteCodeGenerator *byteCodeGenerator)
         {
             sym = pnode->sxVar.sym;
             Assert(sym != nullptr);
+
+            if (sym->GetScope()->GetEnclosingFunc() != byteCodeGenerator->TopFuncInfo())
+            {
+                FuncInfo* parentFunc = GetParentFuncInfo(byteCodeGenerator->TopFuncInfo());
+                Assert(parentFunc == sym->GetScope()->GetEnclosingFunc());
+                if (parentFunc->root->sxFnc.IsAsync())
+                {
+                    // async functions produce a situation where a var decl can have a symbol
+                    // declared from an enclosing function.  In this case just no-op the vardecl.
+                    return;
+                }
+            }
+
             Assert(sym->GetScope()->GetEnclosingFunc() == byteCodeGenerator->TopFuncInfo());
 
             if (pnode->sxVar.isBlockScopeFncDeclVar && sym->GetIsBlockVar())
@@ -4954,14 +4989,14 @@ void AssignRegisters(ParseNode *pnode, ByteCodeGenerator *byteCodeGenerator)
 
                 if (sym->GetIsCatch() || (pnode->nop == knopVarDecl && sym->GetIsBlockVar() && !pnode->sxVar.isBlockScopeFncDeclVar))
                 {
-                // The LHS of the var decl really binds to the local symbol, not the catch or let symbol.
+                    // The LHS of the var decl really binds to the local symbol, not the catch or let symbol.
                     // But the assignment will go to the catch or let symbol. Just assign a register to the local
                     // so that it can get initialized to undefined.
 #if DBG
                     if (!sym->GetIsCatch())
                     {
-                    // Catch cannot be at function scope and let and var at function scope is redeclaration error.
-                    Assert(funcInfo->bodyScope != sym->GetScope() || !byteCodeGenerator->GetScriptContext()->GetConfig()->IsBlockScopeEnabled());
+                        // Catch cannot be at function scope and let and var at function scope is redeclaration error.
+                        Assert(funcInfo->bodyScope != sym->GetScope() || !byteCodeGenerator->GetScriptContext()->GetConfig()->IsBlockScopeEnabled());
                     }
 #endif
                     auto symName = sym->GetName();
@@ -4973,12 +5008,12 @@ void AssignRegisters(ParseNode *pnode, ByteCodeGenerator *byteCodeGenerator)
                     Assert((sym && !sym->GetIsCatch() && !sym->GetIsBlockVar()));
                 }
                 // Don't give the declared var a register if it's in a closure, because the closure slot
-            // is its true "home". (Need to check IsGlobal again as the sym may have changed above.)
+                // is its true "home". (Need to check IsGlobal again as the sym may have changed above.)
                 if (!sym->GetIsGlobal() && !sym->IsInSlot(funcInfo))
                 {
-                if (PHASE_TRACE(Js::DelayCapturePhase, funcInfo->byteCodeFunction))
+                    if (PHASE_TRACE(Js::DelayCapturePhase, funcInfo->byteCodeFunction))
                     {
-                    if (sym->NeedsSlotAlloc(byteCodeGenerator->TopFuncInfo()))
+                        if (sym->NeedsSlotAlloc(byteCodeGenerator->TopFuncInfo()))
                         {
                             Output::Print(L"--- DelayCapture: Delayed capturing symbol '%s' during initialization.\n", sym->GetName());
                             Output::Flush();

lib/Runtime/ByteCode/Symbol.cpp

@@ -245,7 +245,23 @@ Symbol * Symbol::GetFuncScopeVarSym() const
     if (fncScopeSym == nullptr && parentFuncInfo->GetParamScope() != nullptr)
     {
         // We couldn't find the sym in the body scope, try finding it in the parameter scope.
-        fncScopeSym = parentFuncInfo->GetParamScope()->FindLocalSymbol(this->GetName());
+        Scope* paramScope = parentFuncInfo->GetParamScope();
+        fncScopeSym = paramScope->FindLocalSymbol(this->GetName());
+        if (fncScopeSym == nullptr)
+        {
+            FuncInfo* parentParentFuncInfo = paramScope->GetEnclosingScope()->GetFunc();
+            if (parentParentFuncInfo->root->sxFnc.IsAsync())
+            {
+                // In the case of async functions the func-scope var sym might have
+                // come from the parent function parameter scope due to the syntax
+                // desugaring implementation of async functions.
+                fncScopeSym = parentParentFuncInfo->GetBodyScope()->FindLocalSymbol(this->GetName());
+                if (fncScopeSym == nullptr)
+                {
+                    fncScopeSym = parentParentFuncInfo->GetParamScope()->FindLocalSymbol(this->GetName());
+                }
+            }
+        }
     }
     Assert(fncScopeSym);
     // Parser should have added a fake var decl node for block scoped functions in non-strict mode

test/es7/asyncawait-functionality.baseline

@@ -13,6 +13,7 @@ Executing test #7 - Await keyword with a lambda expressions
 Executing test #8 - Async function with default arguments's value
 Executing test #9 - Promise in an Async function
 Executing test #10 - %AsyncFunction% constructor creates async functions analogous to Function constructor
+Executing test #11 - local variables with same names as formal parameters have proper redeclaration semantics (non-error cases, var and function)
 
 Completion Results:
 Test #1 - Success lambda expression with no argument called with result = 'true'
@@ -38,6 +39,8 @@ Test #5 - Success async in a class #10 called with result = '10'
 Test #8 - Success async function with default arguments's value overwritten #1 called with result = 'true'
 Test #8 - Success async function with default arguments's value has been rejected as expected by 'err' #2 called with err = 'expected error'
 Test #8 - Success async function with default arguments's value #3 called with result = 'true'
+Test #11 - Success inner var x overwrote formal parameter x only after the declaration statement
+Test #11 - Success inner function x() overwrote formal parameter x
 Test #6 - Success await in an async function #1 called with result = '-4'
 Test #6 - Success await in an async function #2 called with result = '2'
 Test #6 - Success await in an async function catch a rejected Promise in 'err'. Error = 'Error: My Error'

test/es7/asyncawait-functionality.js

@@ -496,6 +496,38 @@ var tests = [
             });
         }
     },
+    {
+        name: "local variables with same names as formal parameters have proper redeclaration semantics (non-error cases, var and function)",
+        body: function (index) {
+            async function af1(x) { var y = x; var x = 'b'; return y + x; }
+
+            af1('a').then(result => {
+                if (result === 'ab') {
+                    echo(`Test #${index} - Success inner var x overwrote formal parameter x only after the declaration statement`);
+                } else {
+                    echo(`Test #${index} - Failure x appears to have an unexpected value x = ${x}`);
+                }
+            }, err => {
+                echo(`Test #${index} - Error var redeclaration with err = ${err}`);
+            }).catch(err => {
+                echo(`Test #${index} - Catch var redeclaration with err = ${err}`);
+            });
+
+            async function af2(x) { var xx = x(); function x() { return 'afx'; } return xx; }
+
+            af2(function () { return ''; }).then(result => {
+                if (result === 'afx') {
+                    echo(`Test #${index} - Success inner function x() overwrote formal parameter x`);
+                } else {
+                    echo(`Test #${index} - Failure x appears not assigned with inner function x(), x = ${x}`);
+                }
+            }, err => {
+                echo(`Test #${index} - Error err = ${err}`);
+            }).catch(err => {
+                echo(`Test #${index} - Catch err = ${err}`);
+            });
+        }
+    },
 ];
 
 var index = 1;

test/es7/asyncawait-syntax.js

@@ -186,6 +186,16 @@ var tests = [
             assert.throws(function () { "use strict"; eval("async function af(a, b, a) { }"); }, SyntaxError, "duplicate parameter names are not allowed in an async function that is already in strict mode (when there are other names)", "Duplicate formal parameter names not allowed in strict mode");
         }
     },
+    {
+        name: "local variables with same names as formal parameters have proper redeclaration semantics",
+        body: function () {
+            assert.doesNotThrow(function () { eval("async function af(x) { var x; }"); }, "var with same name as formal is not an error");
+            assert.throws(function () { eval("async function af(x) { let x; }"); }, SyntaxError, "let with same name as formal is an error", "Let/Const redeclaration");
+            assert.throws(function () { eval("async function af(x) { const x = 1; }"); }, SyntaxError, "const with same name as formal is an error", "Let/Const redeclaration");
+            assert.doesNotThrow(function () { eval("async function af(x) { function x() { } }"); }, "local function with same name as formal is not an error");
+            assert.throws(function () { eval("async function af(x) { class x { } }"); }, SyntaxError, "class with same name as formal is an error", "Let/Const redeclaration");
+        }
+    },
 ];
 
 testRunner.runTests(tests, { verbose: WScript.Arguments[0] != "summary" });