Remove duplicately named variables from a closure class

Remove it from the usevar list. Also, add assert to catch stack pointer bugs.

hphp/compiler/analysis/emitter.cpp

@@ -3756,8 +3756,7 @@ bool EmitterVisitor::visitImpl(ConstructPtr node) {
             ParameterExpressionPtr var(
               static_pointer_cast<ParameterExpression>((*useList)[i]));
             StringData* varName = StringData::GetStaticString(var->getName());
-            useVars.push_back(
-              ClosureUseVar(varName, var->isRef()));
+            useVars.push_back(ClosureUseVar(varName, var->isRef()));
           }
         }
 
@@ -3788,8 +3787,8 @@ bool EmitterVisitor::visitImpl(ConstructPtr node) {
         // Instance variables.
         TypedValue uninit;
         tvWriteUninit(&uninit);
-        for (int i = 0; i < useCount; ++i) {
-          pce->addProperty(useVars[i].first, AttrPrivate, nullptr, &uninit);
+        for (auto& useVar : useVars) {
+          pce->addProperty(useVar.first, AttrPrivate, nullptr, &uninit);
         }
 
         // The constructor. This is entirely generated; all it does is stash its
@@ -5254,20 +5253,10 @@ void EmitterVisitor::emitPostponedMeths() {
       fe->allocVarId(StringData::GetStaticString("0Closure"));
 
       ClosureUseVarVec* useVars = p.m_closureUseVars;
-      auto it = useVars->begin();
-      while (it != useVars->end()) {
-        const StringData* name = it->first;
-        if (fe->hasVar(name) && fe->lookupVarId(name) < fe->numParams()) {
-          // Because PHP is insane you can have a use variable with the same
-          // name as a param name.
-          // In that case, params win (which is different than zend but much easier)
-          it = useVars->erase(it);
-        } else {
-          // These are all locals. I want them right after the params so I don't
-          // have to keep track of which one goes where at runtime.
-          fe->allocVarId(name);
-          it++;
-        }
+      for (auto& useVar : *useVars) {
+        // These are all locals. I want them right after the params so I don't
+        // have to keep track of which one goes where at runtime.
+        fe->allocVarId(useVar.first);
       }
     }
 

hphp/compiler/expression/closure_expression.cpp

@@ -43,6 +43,20 @@ ClosureExpression::ClosureExpression
       (new ExpressionList(vars->getScope(), vars->getLocation()));
     // push the vars in reverse order, not retaining duplicates
     std::set<string> seenBefore;
+
+    // Because PHP is insane you can have a use variable with the same
+    // name as a param name.
+    // In that case, params win (which is different than zend but much easier)
+    ExpressionListPtr bodyParams = m_func->getParams();
+    if (bodyParams) {
+      int nParams = bodyParams->getCount();
+      for (int i = 0; i < nParams; i++) {
+        ParameterExpressionPtr par(
+          static_pointer_cast<ParameterExpression>((*bodyParams)[i]));
+        seenBefore.insert(par->getName());
+      }
+    }
+
     for (int i = vars->getCount() - 1; i >= 0; i--) {
       ParameterExpressionPtr param(
         dynamic_pointer_cast<ParameterExpression>((*vars)[i]));

hphp/runtime/vm/bytecode.cpp

@@ -4444,7 +4444,8 @@ inline void OPTBLD_INLINE VMExecutionContext::iopRetC(PC& pc) {
   ActRec* sfp = arGetSfp(m_fp);
   // Memcpy the the return value on top of the activation record. This works
   // the same regardless of whether the return value is boxed or not.
-  memcpy(&(m_fp->m_r), m_stack.topTV(), sizeof(TypedValue));
+  TypedValue* retval_ptr = &m_fp->m_r;
+  memcpy(retval_ptr, m_stack.topTV(), sizeof(TypedValue));
   // Adjust the stack
   m_stack.ndiscard(m_fp->m_func->numSlotsInFrame() + 1);
 
@@ -4453,6 +4454,7 @@ inline void OPTBLD_INLINE VMExecutionContext::iopRetC(PC& pc) {
     m_fp = sfp;
     pc = m_fp->m_func->unit()->entry() + m_fp->m_func->base() + soff;
     m_stack.ret();
+    assert(m_stack.topTV() == retval_ptr);
   } else {
     // No caller; terminate.
     m_stack.ret();