From 0f65cc6791fcda21941aca03a256ee199275b94f Mon Sep 17 00:00:00 2001 From: Jordan DeLong Date: Thu, 6 Feb 2014 16:24:07 -0800 Subject: [PATCH] Fix a bug where we set AttrUnique on non-unique classes Hphpc was ignoring interfaces here, which means we could decide a Class* is unique when it actually isn't, which could cause problems. Reviewed By: @jasone Differential Revision: D1163158 --- hphp/compiler/analysis/analysis_result.cpp | 8 +-- hphp/compiler/analysis/class_scope.cpp | 64 ++++++++++------------ hphp/compiler/analysis/class_scope.h | 13 ++--- hphp/compiler/analysis/emitter.cpp | 2 +- hphp/compiler/analysis/type.cpp | 2 +- .../expression/class_constant_expression.cpp | 2 +- hphp/compiler/expression/new_object_expression.cpp | 2 +- .../expression/object_property_expression.cpp | 2 +- hphp/compiler/expression/simple_function_call.cpp | 6 +- .../expression/static_member_expression.cpp | 2 +- 10 files changed, 48 insertions(+), 55 deletions(-) diff --git a/hphp/compiler/analysis/analysis_result.cpp b/hphp/compiler/analysis/analysis_result.cpp index fc54c4d46bb..9d32c5fc6b5 100644 --- a/hphp/compiler/analysis/analysis_result.cpp +++ b/hphp/compiler/analysis/analysis_result.cpp @@ -734,10 +734,10 @@ void AnalysisResult::analyzeProgram(bool system /* = false */) { cls->setStaticDynamic(ar); } StringToFunctionScopePtrMap methods; - cls->collectMethods(ar, methods); + cls->collectMethods(ar, methods, true /* include privates */); bool needAbstractMethodImpl = (!cls->isAbstract() && !cls->isInterface() && - !cls->derivesFromRedeclaring() && + cls->derivesFromRedeclaring() == Derivation::Normal && !cls->getAttribute(ClassScope::UsesUnknownTrait)); for (StringToFunctionScopePtrMap::const_iterator iterMethod = methods.begin(); iterMethod != methods.end(); ++iterMethod) { @@ -756,7 +756,7 @@ void AnalysisResult::analyzeProgram(bool system /* = false */) { string cname; BOOST_FOREACH(tie(cname, cls), m_systemClasses) { StringToFunctionScopePtrMap methods; - cls->collectMethods(ar, methods); + cls->collectMethods(ar, methods, true /* include privates */); for (StringToFunctionScopePtrMap::const_iterator iterMethod = methods.begin(); iterMethod != methods.end(); ++iterMethod) { m_methodToClassDecs[iterMethod->first].push_back(cls); @@ -812,7 +812,7 @@ void AnalysisResult::analyzePerfectVirtuals() { ClassScopePtr cls = iter->second[i]; // being conservative, not to do redeclaring classes at all - if (cls->derivesFromRedeclaring()) { + if (cls->derivesFromRedeclaring() == Derivation::Redeclaring) { addClassRootMethods(ar, cls, redeclaringMethods); continue; } diff --git a/hphp/compiler/analysis/class_scope.cpp b/hphp/compiler/analysis/class_scope.cpp index 57aab40918b..067d8e473be 100644 --- a/hphp/compiler/analysis/class_scope.cpp +++ b/hphp/compiler/analysis/class_scope.cpp @@ -60,7 +60,7 @@ ClassScope::ClassScope(KindOf kindOf, const std::string &name, const std::vector &attrs) : BlockScope(name, docComment, stmt, BlockScope::ClassScope), m_parent(parent), m_bases(bases), m_attribute(0), m_redeclaring(-1), - m_kindOf(kindOf), m_derivesFromRedeclaring(FromNormal), + m_kindOf(kindOf), m_derivesFromRedeclaring(Derivation::Normal), m_traitStatus(NOT_FLATTENED), m_volatile(false), m_persistent(false), m_derivedByDynamic(false), m_needsCppCtor(false), m_needsInit(true), m_knownBases(0) { @@ -90,7 +90,7 @@ ClassScope::ClassScope(AnalysisResultPtr ar, : BlockScope(name, "", StatementPtr(), BlockScope::ClassScope), m_parent(parent), m_bases(bases), m_attribute(0), m_redeclaring(-1), - m_kindOf(KindOfObjectClass), m_derivesFromRedeclaring(FromNormal), + m_kindOf(KindOfObjectClass), m_derivesFromRedeclaring(Derivation::Normal), m_traitStatus(NOT_FLATTENED), m_dynamic(false), m_volatile(false), m_persistent(false), m_derivedByDynamic(false), m_needsCppCtor(false), @@ -283,8 +283,7 @@ void ClassScope::checkDerivation(AnalysisResultPtr ar, hphp_string_iset &seen) { void ClassScope::collectMethods(AnalysisResultPtr ar, StringToFunctionScopePtrMap &funcs, - bool collectPrivate /* = true */, - bool forInvoke /* = false */) { + bool collectPrivate) { // add all functions this class has for (FunctionScopePtrVec::const_iterator iter = m_functionsVec.begin(); iter != m_functionsVec.end(); ++iter) { @@ -310,48 +309,39 @@ void ClassScope::collectMethods(AnalysisResultPtr ar, } } - int n = forInvoke ? m_parent.empty() ? 0 : 1 : m_bases.size(); - // walk up + int n = m_bases.size(); for (int i = 0; i < n; i++) { const string &base = m_bases[i]; ClassScopePtr super = ar->findClass(base); if (super) { if (super->isRedeclaring()) { - if (forInvoke) continue; - const ClassScopePtrVec &classes = ar->findRedeclaredClasses(base); StringToFunctionScopePtrMap pristine(funcs); - BOOST_FOREACH(ClassScopePtr cls, classes) { + + for (auto& cls : classes) { cls->m_derivedByDynamic = true; StringToFunctionScopePtrMap cur(pristine); derivedMagicMethods(cls); - cls->collectMethods(ar, cur, false, forInvoke); + cls->collectMethods(ar, cur, false); inheritedMagicMethods(cls); funcs.insert(cur.begin(), cur.end()); cls->getVariables()-> forceVariants(ar, VariableTable::AnyNonPrivateVars); } - if (base == m_parent) { - m_derivesFromRedeclaring = DirectFromRedeclared; - getVariables()->forceVariants(ar, VariableTable::AnyNonPrivateVars, - false); - getVariables()->setAttribute(VariableTable::NeedGlobalPointer); - } else if (isInterface()) { - m_derivesFromRedeclaring = DirectFromRedeclared; - } + m_derivesFromRedeclaring = Derivation::Redeclaring; + getVariables()->forceVariants(ar, VariableTable::AnyNonPrivateVars, + false); + getVariables()->setAttribute(VariableTable::NeedGlobalPointer); + setVolatile(); } else { derivedMagicMethods(super); - super->collectMethods(ar, funcs, false, forInvoke); + super->collectMethods(ar, funcs, false); inheritedMagicMethods(super); - if (super->derivesFromRedeclaring()) { - if (base == m_parent) { - m_derivesFromRedeclaring = IndirectFromRedeclared; - getVariables()->forceVariants(ar, VariableTable::AnyNonPrivateVars); - } else if (isInterface()) { - m_derivesFromRedeclaring = IndirectFromRedeclared; - } + if (super->derivesFromRedeclaring() == Derivation::Redeclaring) { + m_derivesFromRedeclaring = Derivation::Redeclaring; + getVariables()->forceVariants(ar, VariableTable::AnyNonPrivateVars); setVolatile(); } else if (super->isVolatile()) { setVolatile(); @@ -361,13 +351,17 @@ void ClassScope::collectMethods(AnalysisResultPtr ar, Compiler::Error(Compiler::UnknownBaseClass, m_stmt, base); if (base == m_parent) { ar->declareUnknownClass(m_parent); - m_derivesFromRedeclaring = DirectFromRedeclared; + m_derivesFromRedeclaring = Derivation::Redeclaring; getVariables()->setAttribute(VariableTable::NeedGlobalPointer); getVariables()->forceVariants(ar, VariableTable::AnyNonPrivateVars); setVolatile(); } else { + /* + * TODO(#3685260): this should not be removing interfaces from + * the base list. + */ if (isInterface()) { - m_derivesFromRedeclaring = DirectFromRedeclared; + m_derivesFromRedeclaring = Derivation::Redeclaring; } m_bases.erase(m_bases.begin() + i); n--; @@ -1065,7 +1059,7 @@ FunctionScopePtr ClassScope::findFunction(AnalysisResultConstPtr ar, if (exclIntfBase && super->isInterface()) break; if (super->isRedeclaring()) { if (base == m_parent) { - m_derivesFromRedeclaring = DirectFromRedeclared; + m_derivesFromRedeclaring = Derivation::Redeclaring; break; } continue; @@ -1076,7 +1070,7 @@ FunctionScopePtr ClassScope::findFunction(AnalysisResultConstPtr ar, } } if (!Option::AllDynamic && - derivesFromRedeclaring() == DirectFromRedeclared) { + derivesFromRedeclaring() == Derivation::Redeclaring) { setDynamic(ar, name); } @@ -1099,7 +1093,7 @@ FunctionScopePtr ClassScope::findConstructor(AnalysisResultConstPtr ar, } // walk up - if (recursive && derivesFromRedeclaring() != DirectFromRedeclared) { + if (recursive && derivesFromRedeclaring() != Derivation::Redeclaring) { ClassScopePtr super = ar->findClass(m_parent); if (super) { FunctionScopePtr func = super->findConstructor(ar, true); @@ -1107,7 +1101,7 @@ FunctionScopePtr ClassScope::findConstructor(AnalysisResultConstPtr ar, } } if (!Option::AllDynamic && - derivesFromRedeclaring() == DirectFromRedeclared) { + derivesFromRedeclaring() == Derivation::Redeclaring) { setDynamic(ar, name); } @@ -1121,7 +1115,7 @@ void ClassScope::setStaticDynamic(AnalysisResultConstPtr ar) { if (fs->isStatic()) fs->setDynamic(); } if (!m_parent.empty()) { - if (derivesFromRedeclaring() == DirectFromRedeclared) { + if (derivesFromRedeclaring() == Derivation::Redeclaring) { const ClassScopePtrVec &parents = ar->findRedeclaredClasses(m_parent); BOOST_FOREACH(ClassScopePtr cl, parents) { cl->setStaticDynamic(ar); @@ -1143,7 +1137,7 @@ void ClassScope::setDynamic(AnalysisResultConstPtr ar, FunctionScopePtr fs = iter->second; fs->setDynamic(); } else if (!m_parent.empty()) { - if (derivesFromRedeclaring() == DirectFromRedeclared) { + if (derivesFromRedeclaring() == Derivation::Redeclaring) { const ClassScopePtrVec &parents = ar->findRedeclaredClasses(m_parent); BOOST_FOREACH(ClassScopePtr cl, parents) { cl->setDynamic(ar, name); @@ -1447,7 +1441,7 @@ bool ClassScope::canSkipCreateMethod(AnalysisResultConstPtr ar) const { // 2) no constructor defined (__construct or class name) // 3) no init() defined - if (derivesFromRedeclaring() || + if (derivesFromRedeclaring() == Derivation::Redeclaring || getAttribute(HasConstructor) || getAttribute(ClassNameConstructor) || needsInitMethod()) { diff --git a/hphp/compiler/analysis/class_scope.h b/hphp/compiler/analysis/class_scope.h index 70b06245774..6e37ea624ec 100644 --- a/hphp/compiler/analysis/class_scope.h +++ b/hphp/compiler/analysis/class_scope.h @@ -39,6 +39,11 @@ DECLARE_BOOST_TYPES(FileScope); class Symbol; +enum class Derivation { + Normal, + Redeclaring, // At least one ancestor class or interface is redeclared. +}; + /** * A class scope corresponds to a class declaration. We store all * inferred types and analyzed results here, so not to pollute syntax trees. @@ -90,11 +95,6 @@ public: Abstract = 16, Final = 32 }; - enum Derivation { - FromNormal = 0, - DirectFromRedeclared, - IndirectFromRedeclared - }; enum JumpTableName { JumpTableCallInfo @@ -205,8 +205,7 @@ public: */ void collectMethods(AnalysisResultPtr ar, StringToFunctionScopePtrMap &func, - bool collectPrivate = true, - bool forInvoke = false); + bool collectPrivate); /** * Whether or not we can directly call ObjectData::o_invoke() when lookup diff --git a/hphp/compiler/analysis/emitter.cpp b/hphp/compiler/analysis/emitter.cpp index bd998ad97ef..6738acc923f 100644 --- a/hphp/compiler/analysis/emitter.cpp +++ b/hphp/compiler/analysis/emitter.cpp @@ -7724,7 +7724,7 @@ void EmitterVisitor::emitClass(Emitter& e, AttrNone; if (Option::WholeProgram) { if (!cNode->isRedeclaring() && - !cNode->derivesFromRedeclaring()) { + cNode->derivesFromRedeclaring() == Derivation::Normal) { attr = attr | AttrUnique; if (!cNode->isVolatile()) { attr = attr | AttrPersistent; diff --git a/hphp/compiler/analysis/type.cpp b/hphp/compiler/analysis/type.cpp index 979c59f8170..5c0c2181c45 100644 --- a/hphp/compiler/analysis/type.cpp +++ b/hphp/compiler/analysis/type.cpp @@ -780,7 +780,7 @@ TypePtr Type::InferredObject(AnalysisResultConstPtr ar, } else if (c2ok && cls2->derivesFrom(ar, type1->m_name, true, false)) { resultType = type2; } else if (c1ok && c2ok && cls1->derivedByDynamic() && - cls2->derivesFromRedeclaring()) { + cls2->derivesFromRedeclaring() == Derivation::Redeclaring) { resultType = type2; } else { resultType = type1; diff --git a/hphp/compiler/expression/class_constant_expression.cpp b/hphp/compiler/expression/class_constant_expression.cpp index 0a58a5bee8b..d9ee3c45ffa 100644 --- a/hphp/compiler/expression/class_constant_expression.cpp +++ b/hphp/compiler/expression/class_constant_expression.cpp @@ -201,7 +201,7 @@ TypePtr ClassConstantExpression::inferTypes(AnalysisResultPtr ar, if (defScope) { m_valid = true; m_defScope = defScope; - } else if (cls->derivesFromRedeclaring()) { + } else if (cls->derivesFromRedeclaring() == Derivation::Redeclaring) { m_defScope = cls.get(); } diff --git a/hphp/compiler/expression/new_object_expression.cpp b/hphp/compiler/expression/new_object_expression.cpp index f425dee5f2a..24016c1bdbb 100644 --- a/hphp/compiler/expression/new_object_expression.cpp +++ b/hphp/compiler/expression/new_object_expression.cpp @@ -113,7 +113,7 @@ TypePtr NewObjectExpression::inferTypes(AnalysisResultPtr ar, TypePtr type, getScope()->getVariables()-> setAttribute(VariableTable::NeedGlobalPointer); } - m_dynamic = cls->derivesFromRedeclaring(); + m_dynamic = cls->derivesFromRedeclaring() == Derivation::Redeclaring; bool valid = true; FunctionScopePtr func = cls->findConstructor(ar, true); if (!func) { diff --git a/hphp/compiler/expression/object_property_expression.cpp b/hphp/compiler/expression/object_property_expression.cpp index cc3c1451019..0194ba95600 100644 --- a/hphp/compiler/expression/object_property_expression.cpp +++ b/hphp/compiler/expression/object_property_expression.cpp @@ -278,7 +278,7 @@ TypePtr ObjectPropertyExpression::inferTypes(AnalysisResultPtr ar, } TypePtr ret; - if (m_propSymValid && (!cls->derivesFromRedeclaring() || + if (m_propSymValid && (cls->derivesFromRedeclaring() == Derivation::Normal || m_propSym->isPrivate())) { always_assert(m_symOwner); TypePtr t(m_propSym->getType()); diff --git a/hphp/compiler/expression/simple_function_call.cpp b/hphp/compiler/expression/simple_function_call.cpp index 4ee0939a016..2289f084848 100644 --- a/hphp/compiler/expression/simple_function_call.cpp +++ b/hphp/compiler/expression/simple_function_call.cpp @@ -528,7 +528,7 @@ void SimpleFunctionCall::analyzeProgram(AnalysisResultPtr ar) { (m_className.empty() || (m_classScope && !m_classScope->isTrait() && - !m_classScope->derivesFromRedeclaring() && + m_classScope->derivesFromRedeclaring() == Derivation::Normal && !m_classScope->getAttribute( ClassScope::HasUnknownStaticMethodHandler) && !m_classScope->getAttribute( @@ -1355,7 +1355,7 @@ static int isObjCall(AnalysisResultPtr ar, if (!thisCls || !thisFunc || thisFunc->isStatic()) return 0; if (thisCls == methCls) return 1; if (thisCls->derivesFrom(ar, methClsName, true, false)) return 1; - if (thisCls->derivesFromRedeclaring() && + if (thisCls->derivesFromRedeclaring() == Derivation::Redeclaring && thisCls->derivesFrom(ar, methClsName, true, true)) { return -1; } @@ -1502,7 +1502,7 @@ SimpleFunctionCallPtr SimpleFunctionCall::GetFunctionCallForCallUserFunc( string name = smethod.substr(0, c); if (cls->getName() != name) { if (!cls->derivesFrom(ar, name, true, false)) { - error = cls->derivesFromRedeclaring() == ClassScope::FromNormal; + error = cls->derivesFromRedeclaring() == Derivation::Normal; return SimpleFunctionCallPtr(); } } diff --git a/hphp/compiler/expression/static_member_expression.cpp b/hphp/compiler/expression/static_member_expression.cpp index ba30ba1be36..d789fc936d2 100644 --- a/hphp/compiler/expression/static_member_expression.cpp +++ b/hphp/compiler/expression/static_member_expression.cpp @@ -77,7 +77,7 @@ bool StaticMemberExpression::findMember(AnalysisResultPtr ar, string &name, m_resolvedClass = resolveClass(); if (!m_resolvedClass) return isRedeclared(); - if (m_resolvedClass->derivesFromRedeclaring()) { + if (m_resolvedClass->derivesFromRedeclaring() == Derivation::Redeclaring) { m_dynamicClass = true; } -- 2.11.4.GIT