Permalink
Browse files

:Allow $this on closures

In Zend 5.3 they decided that closures should inherit the ##$this## from the containing scope. This brings us close to paraity with them. The remaining thing is to make

    function() use ($this) {}

fatal after we purge it from WWW.
  • Loading branch information...
1 parent 3718c61 commit 4d7004e955c764c5e7474c4b366d3606348f61eb @ptarjan ptarjan committed with sgolemon Mar 20, 2013
Showing with 676 additions and 190 deletions.
  1. +20 −4 hphp/compiler/analysis/block_scope.cpp
  2. +2 −0 hphp/compiler/analysis/block_scope.h
  3. +42 −35 hphp/compiler/analysis/emitter.cpp
  4. +5 −0 hphp/compiler/expression/closure_expression.cpp
  5. +14 −18 hphp/idl/closure.idl.php
  6. +47 −6 hphp/runtime/ext/ext_closure.cpp
  7. +0 −48 hphp/runtime/ext/ext_closure.ext_hhvm.cpp
  8. +17 −8 hphp/runtime/ext/ext_closure.h
  9. +2 −4 hphp/runtime/ext_hhvm/ext_hhvm_infotabs.cpp
  10. +23 −4 hphp/runtime/vm/bytecode.cpp
  11. +5 −0 hphp/runtime/vm/bytecode.h
  12. +71 −8 hphp/runtime/vm/func.cpp
  13. +18 −3 hphp/runtime/vm/func.h
  14. +35 −22 hphp/runtime/vm/runtime.cpp
  15. +3 −0 hphp/runtime/vm/runtime.h
  16. +11 −1 hphp/runtime/vm/translator/translator-x64.cpp
  17. +4 −4 hphp/runtime/vm/unit.cpp
  18. +2 −2 hphp/runtime/vm/unit.h
  19. +0 −6 hphp/system/class_map.cpp
  20. +1 −1 hphp/system/closure.inc
  21. +13 −10 hphp/test/test_code_run.cpp
  22. +1 −3 hphp/test/vm/closure.php.exp
  23. +30 −0 hphp/test/vm/closure_class.php
  24. +12 −0 hphp/test/vm/closure_class.php.exp
  25. +29 −0 hphp/test/vm/closure_get_class.php
  26. +10 −0 hphp/test/vm/closure_get_class.php.exp
  27. +18 −0 hphp/test/vm/closure_in_generator.php
  28. +1 −0 hphp/test/vm/closure_in_generator.php.exp
  29. +20 −0 hphp/test/vm/closure_in_generator2.php
  30. +1 −0 hphp/test/vm/closure_in_generator2.php.exp
  31. +73 −0 hphp/test/vm/closure_private.php
  32. +20 −0 hphp/test/vm/closure_private.php.exp
  33. +18 −0 hphp/test/vm/closure_recursive.php
  34. +1 −0 hphp/test/vm/closure_recursive.php.exp
  35. +17 −0 hphp/test/vm/closure_recursive_bad.php
  36. +2 −0 hphp/test/vm/closure_recursive_bad.php.exp
  37. +1 −0 hphp/test/vm/closure_recursive_bad.php.filter
  38. +49 −0 hphp/test/vm/zend_closure_005.php
  39. +7 −0 hphp/test/vm/zend_closure_005.php.exp
  40. +26 −0 hphp/test/vm/zend_closure_007.php
  41. +4 −0 hphp/test/vm/zend_closure_007.php.exp
  42. +1 −3 hphp/test/vm/zend_closure_020.php.exp
@@ -89,13 +89,29 @@ AnalysisResultRawPtr BlockScope::getContainingProgram() {
return AnalysisResultRawPtr((AnalysisResult*)bs);
}
-ClassScopeRawPtr BlockScope::getContainingClass() {
+FunctionScopeRawPtr BlockScope::getContainingNonClosureFunction() {
BlockScope *bs = this;
- if (bs->is(BlockScope::FunctionScope)) {
+ // walk out through all the closures
+ while (bs && bs->is(BlockScope::FunctionScope)) {
+ HPHP::FunctionScope *fs = static_cast<HPHP::FunctionScope*>(bs);
+ if (!fs->isClosure() && !fs->isGeneratorFromClosure()) {
+ return FunctionScopeRawPtr(fs);
+ }
+ bs = bs->m_outerScope.get();
+ }
+ return FunctionScopeRawPtr();
+}
+
+ClassScopeRawPtr BlockScope::getContainingClass() {
+ BlockScope *bs = getContainingNonClosureFunction().get();
+ if (!bs) {
+ bs = this;
+ }
+ if (bs && bs->is(BlockScope::FunctionScope)) {
bs = bs->m_outerScope.get();
}
- if (bs && !bs->is(BlockScope::ClassScope)) {
- bs = 0;
+ if (!bs || !bs->is(BlockScope::ClassScope)) {
+ return ClassScopeRawPtr();
}
return ClassScopeRawPtr((HPHP::ClassScope*)bs);
}
@@ -341,6 +341,8 @@ class BlockScope : private boost::noncopyable,
int m_updated;
int m_runId;
private:
+ FunctionScopeRawPtr getContainingNonClosureFunction();
+
Marks m_mark;
BlockScopeRawPtrFlagsPtrVec m_orderedDeps;
BlockScopeRawPtrFlagsVec m_orderedUsers;
@@ -2501,7 +2501,6 @@ bool EmitterVisitor::visitImpl(ConstructPtr node) {
not_reached();
case Statement::KindOfFunctionStatement: {
- assert(!node->getClassScope()); // Handled directly by emitClass().
MethodStatementPtr m(static_pointer_cast<MethodStatement>(node));
// Only called for fn defs not on the top level
StringData* nName = StringData::GetStaticString(m->getOriginalName());
@@ -2523,6 +2522,7 @@ bool EmitterVisitor::visitImpl(ConstructPtr node) {
postponeMeth(m, nullptr, true);
} else {
+ assert(!node->getClassScope()); // Handled directly by emitClass().
FuncEmitter* fe = m_ue.newFuncEmitter(nName, false);
e.DefFunc(fe->id());
postponeMeth(m, fe, false);
@@ -3540,9 +3540,9 @@ bool EmitterVisitor::visitImpl(ConstructPtr node) {
scalarExp(static_pointer_cast<ScalarExpression>(node));
// Inside traits, __class__ cannot be resolved yet,
// so emit call to get_class.
- if (scalarExp->getType() == T_CLASS_C && m_curFunc &&
- m_curFunc->pce() &&
- (m_curFunc->pce()->attrs() & VM::AttrTrait)) {
+ if (scalarExp->getType() == T_CLASS_C &&
+ ex->getFunctionScope()->getContainingClass() &&
+ ex->getFunctionScope()->getContainingClass()->isTrait()) {
static const StringData* fname =
StringData::GetStaticString("get_class");
Offset fpiStart = m_ue.bcPos();
@@ -5259,6 +5259,28 @@ void EmitterVisitor::emitPostponedMeths() {
m_curFunc = fe;
+ if (fe->isClosureBody()) {
+ // We are going to keep the closure as the first local
+ 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++;
+ }
+ }
+ }
+
// Assign ids to all of the local variables eagerly. This gives us the
// nice property that all named local variables will be assigned ids
// 0 through k-1, while any unnamed local variable will have an id >= k.
@@ -5337,35 +5359,6 @@ void EmitterVisitor::emitPostponedMeths() {
assert(tc.typeName()->data() != (const char*)0xdeadba5eba11f00d);
e.VerifyParamType(i);
}
- if (fe->isClosureBody()) {
- assert(p.m_closureUseVars != nullptr);
- // Emit code to unpack the instance variables (which store the
- // use-variables) into locals. Some of the use-variables may have the
- // same name, in which case the last one wins.
- unsigned n = p.m_closureUseVars->size();
- for (unsigned i = 0; i < n; ++i) {
- StringData* name = (*p.m_closureUseVars)[i].first;
- bool byRef = (*p.m_closureUseVars)[i].second;
- emitVirtualLocal(fe->lookupVarId(name));
- if (i) {
- m_metaInfo.add(m_ue.bcPos(), Unit::MetaInfo::GuardedThis,
- false, 0, 0);
- }
- e.CheckThis();
- m_evalStack.push(StackSym::H);
- m_evalStack.push(StackSym::T);
- m_evalStack.setString(name);
- markProp(e);
- if (byRef) {
- emitVGet(e);
- emitBind(e);
- } else {
- emitCGet(e);
- emitSet(e);
- }
- emitPop(e);
- }
- }
if (funcScope->isAbstract()) {
StringData* msg =
@@ -5411,8 +5404,7 @@ void EmitterVisitor::emitPostponedMeths() {
e.ContExit();
} else {
e.Null();
- if ((p.m_meth->getStmts() && p.m_meth->getStmts()->isGuarded()) ||
- (fe->isClosureBody() && p.m_closureUseVars->size())) {
+ if ((p.m_meth->getStmts() && p.m_meth->getStmts()->isGuarded())) {
m_metaInfo.add(m_ue.bcPos(), Unit::MetaInfo::GuardedThis,
false, 0, 0);
}
@@ -5665,6 +5657,21 @@ void EmitterVisitor::emitPostponedClosureCtors() {
emitPop(e);
}
}
+
+ // call parent::__construct()
+ static StringData* s___construct = StringData::GetStaticString("__construct");
+ e.String(s___construct);
+ static StringData* s_Closure = StringData::GetStaticString("Closure");
+ e.String(s_Closure);
+ e.AGetC();
+ Offset fpiStart = m_ue.bcPos();
+ e.FPushClsMethodF(0);
+ {
+ FPIRegionRecorder fpi(this, m_ue, m_evalStack, fpiStart);
+ }
+ e.FCall(0);
+ e.PopR();
+
e.Null();
if (n > 0) {
m_metaInfo.add(m_ue.bcPos(), Unit::MetaInfo::GuardedThis,
@@ -47,6 +47,11 @@ ClosureExpression::ClosureExpression
ParameterExpressionPtr param(
dynamic_pointer_cast<ParameterExpression>((*vars)[i]));
assert(param);
+ if (param->getName() == "this") {
+ // "this" is automatically included.
+ // Once we get rid of all the callsites, make this an error
+ continue;
+ }
if (seenBefore.find(param->getName().c_str()) == seenBefore.end()) {
seenBefore.insert(param->getName().c_str());
m_vars->insertElement(param);
@@ -12,6 +12,7 @@
// Preamble: C++ code inserted at beginning of ext_{name}.h
DefinePreamble(<<<CPP
+#include <runtime/vm/func.h>
CPP
);
@@ -88,40 +89,35 @@
array(
'name' => 'Closure',
'desc' => 'Used as the base class for all closures',
+ // https://phabricator.fb.com/D727298 means this can't be abstract
+ // 'flags' => IsAbstract,
'footer' => <<<EOT
+public:
+ public: ObjectData* clone();
+ ObjectData* getThisOrClass() { return m_thisOrClass; }
+ const VM::Func* getInvokeFunc() { return m_func; }
+ HphpArray* getStaticLocals();
+ TypedValue* getUseVars() { return propVec(); }
+ int getNumUseVars() { return m_cls->numDeclProperties(); }
protected:
virtual bool php_sleep(Variant &ret);
+private:
+ SmartPtr<HphpArray> m_VMStatics;
+ ObjectData* m_thisOrClass;
+ const VM::Func* m_func;
EOT
,
)
);
-DefineProperty(
- array(
- 'name' => '__static_locals',
- 'type' => String,
- 'flags' => IsProtected,
- 'desc' => 'For storing the static locals from the closure body',
- ));
-
DefineFunction(
array(
'name' => '__construct',
- 'args' => array(),
'return' => array(
'type' => null,
),
));
-DefineFunction(
- array(
- 'name' => '__invoke',
- 'flags' => VariableArguments,
- 'return' => array(
- 'type' => Variant,
- ),
- ));
-
EndClass();
BeginClass(
@@ -17,25 +17,66 @@
#include <runtime/ext/ext_closure.h>
#include <runtime/base/builtin_functions.h>
+#include <runtime/vm/translator/translator-inline.h>
namespace HPHP {
///////////////////////////////////////////////////////////////////////////////
-c_Closure::c_Closure(VM::Class* cb) : ExtObjectData(cb) {}
-c_Closure::~c_Closure() {}
+c_Closure::c_Closure(VM::Class* cb) : ExtObjectData(cb),
+ m_thisOrClass(nullptr), m_func(nullptr) {}
+c_Closure::~c_Closure() {
+ // same as ar->hasThis()
+ if (m_thisOrClass && !(intptr_t(m_thisOrClass) & 3LL)) {
+ m_thisOrClass->decRefCount();
+ }
+}
+
+void c_Closure::t___construct() {
+ VM::ActRec* ar;
+ {
+ // like calling CallerFrame twice
+ VM::Transl::VMRegAnchor _;
+ VMExecutionContext* context = g_vmContext;
+ VM::ActRec* me = context->getFP();
+ VM::ActRec* childClosure = context->getPrevVMState(me);
+ ar = context->getPrevVMState(childClosure);
+ }
-void c_Closure::t___construct() {}
+ // I don't care if it is a $this or a late bound class because we will just
+ // put it back in the same place on an ActRec.
+ m_thisOrClass = ar->m_this;
+ if (ar->hasThis()) {
+ ar->getThis()->incRefCount();
+ }
+
+ // Change my __invoke's m_cls to be the same as my creator's
+ static StringData* invokeName = StringData::GetStaticString("__invoke");
+ VM::Class* scope = ar->m_func->cls();
+ m_func = getVMClass()->lookupMethod(invokeName)->cloneAndSetClass(scope);
+}
-Variant c_Closure::t___invoke(int _argc, CArrRef _argv) {
- always_assert(false);
- return uninit_null();
+ObjectData* c_Closure::clone() {
+ ObjectData* obj = ObjectData::clone();
+ auto closure = static_cast<c_Closure*>(obj);
+ closure->m_VMStatics = m_VMStatics;
+ closure->m_thisOrClass = m_thisOrClass;
+ closure->m_func = m_func;
+ return obj;
}
+
bool c_Closure::php_sleep(Variant &ret) {
ret = false;
return true;
}
+HphpArray* c_Closure::getStaticLocals() {
+ if (m_VMStatics.get() == NULL) {
+ m_VMStatics = NEW(HphpArray)(1);
+ }
+ return m_VMStatics.get();
+}
+
///////////////////////////////////////////////////////////////////////////////
c_DummyClosure::c_DummyClosure(VM::Class* cb) :
@@ -70,54 +70,6 @@ TypedValue* tg_7Closure___construct(HPHP::VM::ActRec *ar) {
return &ar->m_r;
}
-/*
-HPHP::Variant HPHP::c_Closure::t___invoke(int, HPHP::Array const&)
-_ZN4HPHP9c_Closure10t___invokeEiRKNS_5ArrayE
-
-(return value) => rax
-_rv => rdi
-this_ => rsi
-_argc => rdx
-_argv => rcx
-*/
-
-TypedValue* th_7Closure___invoke(TypedValue* _rv, ObjectData* this_, int64_t _argc, Value* _argv) asm("_ZN4HPHP9c_Closure10t___invokeEiRKNS_5ArrayE");
-
-TypedValue* tg_7Closure___invoke(HPHP::VM::ActRec *ar) {
- TypedValue rv;
- int64_t count = ar->numArgs();
- TypedValue* args UNUSED = ((TypedValue*)ar) - 1;
- ObjectData* this_ = (ar->hasThis() ? ar->getThis() : NULL);
- if (this_) {
- Array extraArgs;
- {
- ArrayInit ai(count-0);
- for (int64_t i = 0; i < count; ++i) {
- TypedValue* extraArg = ar->getExtraArg(i-0);
- if (tvIsStronglyBound(extraArg)) {
- ai.setRef(i-0, tvAsVariant(extraArg));
- } else {
- ai.set(i-0, tvAsVariant(extraArg));
- }
- }
- extraArgs = ai.create();
- }
- th_7Closure___invoke((&(rv)), (this_), (count), (Value*)(&extraArgs));
- if (rv.m_type == KindOfUninit) rv.m_type = KindOfNull;
- frame_free_locals_inl(ar, 0);
- memcpy(&ar->m_r, &rv, sizeof(TypedValue));
- return &ar->m_r;
- } else {
- throw_instance_method_fatal("Closure::__invoke");
- }
- rv.m_data.num = 0LL;
- rv.m_type = KindOfNull;
- frame_free_locals_inl(ar, 0);
- memcpy(&ar->m_r, &rv, sizeof(TypedValue));
- return &ar->m_r;
- return &ar->m_r;
-}
-
HPHP::VM::Instance* new_DummyClosure_Instance(HPHP::VM::Class* cls) {
size_t nProps = cls->numDeclProperties();
size_t builtinPropSize = sizeof(c_DummyClosure) - sizeof(ObjectData);
Oops, something went wrong.

0 comments on commit 4d7004e

Please sign in to comment.