Skip to content

Commit

Permalink
Fix This type annotation to not behave like self.
Browse files Browse the repository at this point in the history
Summary:
D4892972 implements the this typehint to be checked like self.
This diff corrects it to perform the proper typecheck using the context class.
The options available are:
```
Eval.ThisTypeHintLevel = 0  // <- Typecheck like mixed.
Eval.ThisTypeHintLevel = 1  // <- Typecheck like self.
Eval.ThisTypeHintLevel = 2  // <- Typecheck like this, but soft failure even for hard typehints.
Eval.ThisTypeHintLevel = 3  // <- Typecheck like this.

Eval.CheckThisTypeHints   // <- continues to exist but now enables/disables
                          //    HHBBC optimization that is only compatible
                          //    with Eval.ThisTypeHintLevel=3.
```

The changes to allow multiple behaviors (notably `Eval.ThisTypeHintLevel=2`) are a result of conversations with
KendallHopkins about easing the transition to hard `this`.

Depends on D4892972

Reviewed By: markw65

Differential Revision: D5341430

fbshipit-source-id: 78599030423169c93c8f8399d3afa7ef9b44fc2d
  • Loading branch information
mofarrell authored and hhvm-bot committed Oct 9, 2017
1 parent a316d39 commit 8075082
Show file tree
Hide file tree
Showing 100 changed files with 8,955 additions and 387 deletions.
2 changes: 1 addition & 1 deletion hphp/compiler/analysis/emitter.cpp
Expand Up @@ -11380,7 +11380,7 @@ void commitGlobalData(std::unique_ptr<ArrayTypeTable::Builder> arrTable) {
gd.AutoprimeGenerators = RuntimeOption::AutoprimeGenerators;
gd.PromoteEmptyObject = RuntimeOption::EvalPromoteEmptyObject;
gd.EnableRenameFunction = RuntimeOption::EvalJitEnableRenameFunction;
gd.CheckThisTypeHints = RuntimeOption::EvalCheckThisTypeHints;
gd.ThisTypeHintLevel = RuntimeOption::EvalThisTypeHintLevel;
gd.HackArrCompatNotices = RuntimeOption::EvalHackArrCompatNotices;
gd.EnableIntrinsicsExtension = RuntimeOption::EnableIntrinsicsExtension;
gd.InitialNamedEntityTableSize =
Expand Down
4 changes: 2 additions & 2 deletions hphp/compiler/option.cpp
Expand Up @@ -230,8 +230,8 @@ void Option::Load(const IniSetting::Map& ini, Hdf &config) {

Config::Bind(APCProfile, ini, config, "APCProfile");

Config::Bind(RuntimeOption::EvalCheckThisTypeHints, ini, config,
"CheckThisTypeHints", RuntimeOption::EvalCheckThisTypeHints);
Config::Bind(RuntimeOption::EvalThisTypeHintLevel, ini, config,
"ThisTypeHintLevel", RuntimeOption::EvalThisTypeHintLevel);

Config::Bind(RuntimeOption::EnableHipHopSyntax,
ini, config, "EnableHipHopSyntax",
Expand Down
10 changes: 8 additions & 2 deletions hphp/hhbbc/index.cpp
Expand Up @@ -2839,8 +2839,14 @@ folly::Optional<Type> Index::get_type_for_annotated_type(
* typehints (ex. "(function(..): ..)" typehints).
*/
return TGen;
case AnnotMetaType::Self:
case AnnotMetaType::This:
if (auto s = selfCls(ctx)) {
if (!(*s).couldBeOverriden()) {
return subObj(*s);
}
}
break;
case AnnotMetaType::Self:
if (auto s = selfCls(ctx)) return subObj(*s);
break;
case AnnotMetaType::Parent:
Expand Down Expand Up @@ -3373,7 +3379,7 @@ void Index::init_return_type(const php::Func* func) {

auto const constraint = func->retTypeConstraint;
if (constraint.isSoft() ||
(!RuntimeOption::EvalCheckThisTypeHints && constraint.isThis())) {
(RuntimeOption::EvalThisTypeHintLevel != 3 && constraint.isThis())) {
return;
}

Expand Down
4 changes: 2 additions & 2 deletions hphp/hhbbc/interp.cpp
Expand Up @@ -3335,7 +3335,7 @@ void in(ISS& env, const bc::VerifyParamType& op) {
* references if it re-enters, even if Option::HardTypeHints is
* on.
*/
if (!RuntimeOption::EvalCheckThisTypeHints && constraint.isThis()) {
if (RuntimeOption::EvalThisTypeHintLevel != 3 && constraint.isThis()) {
return;
}
if (constraint.hasConstraint() && !constraint.isTypeVar() &&
Expand Down Expand Up @@ -3365,7 +3365,7 @@ void in(ISS& env, const bc::VerifyRetTypeC& /*op*/) {
// then there are no optimizations we can safely do here, so
// just leave the top of stack as is.
if (RuntimeOption::EvalCheckReturnTypeHints < 3 || constraint.isSoft()
|| (!RuntimeOption::EvalCheckThisTypeHints && constraint.isThis())) {
|| (RuntimeOption::EvalThisTypeHintLevel != 3 && constraint.isThis())) {
return;
}

Expand Down
2 changes: 1 addition & 1 deletion hphp/hhbbc/main.cpp
Expand Up @@ -288,7 +288,7 @@ void write_output(
gd.UsedHHBBC = true;
gd.EnableHipHopSyntax = RuntimeOption::EnableHipHopSyntax;
gd.HardTypeHints = RuntimeOption::EvalHardTypeHints;
gd.CheckThisTypeHints = RuntimeOption::EvalCheckThisTypeHints;
gd.ThisTypeHintLevel = RuntimeOption::EvalThisTypeHintLevel;
gd.HardReturnTypeHints = RuntimeOption::EvalCheckReturnTypeHints >= 3;
gd.HardPrivatePropInference = options.HardPrivatePropInference;
gd.DisallowDynamicVarEnvFuncs = RuntimeOption::DisallowDynamicVarEnvFuncs;
Expand Down
2 changes: 1 addition & 1 deletion hphp/hhbbc/optimize.cpp
Expand Up @@ -851,7 +851,7 @@ void do_optimize(const Index& index, FuncAnalysis&& ainfo, bool isFinal) {
tc.isSoft() ||
tc.isTypeVar() ||
tc.isTypeConstant() ||
(tc.isThis() && !RuntimeOption::EvalCheckThisTypeHints) ||
(tc.isThis() && RuntimeOption::EvalThisTypeHintLevel != 3) ||
tc.type() != AnnotType::Object) {
return;
}
Expand Down
7 changes: 6 additions & 1 deletion hphp/runtime/base/annot-type.h
Expand Up @@ -18,6 +18,7 @@
#define incl_HPHP_ANNOT_TYPE_H_

#include "hphp/runtime/base/datatype.h"
#include "hphp/runtime/base/runtime-option.h"

namespace HPHP {

Expand Down Expand Up @@ -167,11 +168,15 @@ annotCompat(DataType dt, AnnotType at, const StringData* annotClsName) {
? AnnotAction::Pass : AnnotAction::Fail;
case AnnotMetaType::Self:
case AnnotMetaType::Parent:
case AnnotMetaType::This:
// For "self" and "parent", if `dt' is not an object we know
// it's not compatible, otherwise more checks are required
return (dt == KindOfObject)
? AnnotAction::ObjectCheck : AnnotAction::Fail;
case AnnotMetaType::This:
return (dt == KindOfObject)
? AnnotAction::ObjectCheck
: (RuntimeOption::EvalThisTypeHintLevel == 0)
? AnnotAction::Pass : AnnotAction::Fail;
case AnnotMetaType::Callable:
// For "callable", if `dt' is not string/array/object we know
// it's not compatible, otherwise more checks are required
Expand Down
14 changes: 9 additions & 5 deletions hphp/runtime/base/runtime-option.h
Expand Up @@ -501,6 +501,15 @@ struct RuntimeOption {
F(bool, LogThreadCreateBacktraces, false) \
F(bool, FailJitPrologs, false) \
F(bool, UseHHBBC, !getenv("HHVM_DISABLE_HHBBC")) \
/* The following option enables the runtime checks for `this` typehints.
* There are 4 possible options:
* 0 - No checking of `this` typehints.
* 1 - Check `this` as hard `self` typehints.
* 2 - Check `this` typehints as soft `this` typehints
* 3 - Check `this` typehints as hard `this` typehints (unless explicitly
* soft). This is the only option which enable optimization in HHBBC.
*/ \
F(int32_t, ThisTypeHintLevel, 0) \
/* CheckReturnTypeHints:
0 - No checks or enforcement for return type hints.
1 - Raises E_WARNING if a return type hint fails.
Expand All @@ -514,11 +523,6 @@ struct RuntimeOption {
error handler returns something other than boolean false,
the runtime will throw a fatal error. */ \
F(int32_t, CheckReturnTypeHints, 2) \
/* Whether to assume that `this` types will be verified by Verify*Type
instructions at runtime. This changes program behavior because
this type hints that are checked at runtime will enable additional
HHBBC optimizations. This is serialized in Repo::GlobalData */ \
F(bool, CheckThisTypeHints, false) \
/* Whether or not to assume that VerifyParamType instructions must
throw if the parameter does not match the associated type
constraint. This changes program behavior because parameter type
Expand Down
6 changes: 0 additions & 6 deletions hphp/runtime/vm/bytecode.cpp
Expand Up @@ -5607,9 +5607,6 @@ OPTBLD_INLINE void iopVerifyParamType(local_var param) {
bool useStrictTypes =
func->unit()->isHHFile() || RuntimeOption::EnableHipHopSyntax ||
!vmfp()->useWeakTypes();
if (UNLIKELY(!RuntimeOption::EvalCheckThisTypeHints && tc.isThis())) {
return;
}
if (!tc.isTypeVar() && !tc.isTypeConstant()) {
tc.verifyParam(param.ptr, func, param.index, useStrictTypes);
}
Expand All @@ -5622,9 +5619,6 @@ OPTBLD_INLINE void implVerifyRetType() {

const auto func = vmfp()->m_func;
const auto tc = func->returnTypeConstraint();
if (UNLIKELY(!RuntimeOption::EvalCheckThisTypeHints && tc.isThis())) {
return;
}
bool useStrictTypes = func->unit()->useStrictTypes();
if (!tc.isTypeVar() && !tc.isTypeConstant()) {
tc.verifyReturn(vmStack().topTV(), func, useStrictTypes);
Expand Down
36 changes: 29 additions & 7 deletions hphp/runtime/vm/jit/irgen-types.cpp
Expand Up @@ -141,7 +141,8 @@ void verifyTypeImpl(IRGS& env, int32_t const id) {
auto func = curFunc(env);
auto const& tc = isReturnType ? func->returnTypeConstraint()
: func->params()[id].typeConstraint;
if (tc.isMixed() || (!RuntimeOption::EvalCheckThisTypeHints && tc.isThis())) {
if (tc.isMixed() || (RuntimeOption::EvalThisTypeHintLevel == 0
&& tc.isThis())) {
return;
}

Expand Down Expand Up @@ -177,8 +178,10 @@ void verifyTypeImpl(IRGS& env, int32_t const id) {
curUnit(env)->isHHFile() ||
!RuntimeOption::PHP7_ScalarTypes;

auto const failHard = strictTypes &&
RuntimeOption::RepoAuthoritative && !tc.isSoft();
auto const failHard = strictTypes
&& RuntimeOption::RepoAuthoritative
&& !tc.isSoft()
&& !(tc.isThis() && RuntimeOption::EvalThisTypeHintLevel == 2);

if (isReturnType) {
updateMarker(env);
Expand Down Expand Up @@ -246,6 +249,25 @@ void verifyTypeImpl(IRGS& env, int32_t const id) {
return;
}

// At this point we know valType is Obj.
if (tc.isThis() && RuntimeOption::EvalThisTypeHintLevel >= 2) {
// For this type checks, the class needs to be an exact match.
auto const ctxCls = gen(env, LdClsCtx, ldCtx(env));
auto const objClass = gen(env, LdObjClass, val);
ifThen(
env,
[&] (Block* taken) {
gen(env, JmpZero, taken, gen(env, EqCls, ctxCls, objClass));
},
[&] {
hint(env, Block::Hint::Unlikely);
genFail();
}
);
return;
}
assert(IMPLIES(tc.isThis(), RuntimeOption::EvalThisTypeHintLevel == 1));

// If we reach here then valType is Obj and tc is Object, Self, or Parent
const StringData* clsName;
const Class* knownConstraint = nullptr;
Expand All @@ -261,7 +283,8 @@ void verifyTypeImpl(IRGS& env, int32_t const id) {
clsName = tc.typeName();
}
} else {
if (tc.isSelf() || tc.isThis()) {
if (tc.isSelf()
|| (tc.isThis() && RuntimeOption::EvalThisTypeHintLevel == 1)) {
tc.selfToClass(curFunc(env), &knownConstraint);
} else {
assertx(tc.isParent());
Expand All @@ -274,12 +297,11 @@ void verifyTypeImpl(IRGS& env, int32_t const id) {
}
clsName = knownConstraint->preClass()->name();
}
assertx(clsName);

// For "self" and "parent", knownConstraint should always be
// non-null at this point
assertx(IMPLIES(tc.isSelf() || tc.isThis() || tc.isParent(),
knownConstraint != nullptr));
assertx(IMPLIES(tc.isSelf() || tc.isParent(), knownConstraint != nullptr));
assertx(IMPLIES(tc.isSelf() || tc.isParent(), clsName != nullptr));

auto const checkCls = ldClassSafe(env, clsName, knownConstraint);
auto const fastIsInstance = implInstanceCheck(env, val, clsName, checkCls);
Expand Down
17 changes: 12 additions & 5 deletions hphp/runtime/vm/jit/translator-runtime.cpp
Expand Up @@ -627,15 +627,22 @@ ALWAYS_INLINE
static bool VerifyTypeSlowImpl(const Class* cls,
const Class* constraint,
const HPHP::TypeConstraint* expected) {
// This helper should only be called for the Object, Self, and Parent cases
assertx(expected->isObject() || expected->isSelf() || expected->isParent());
// For the Self and Parent cases, we must always have a resolved class for
// the constraint
// This helper should only be called for the Object, This, Self, and Parent
// cases
assertx(expected->isObject() || expected->isSelf() || expected->isParent()
|| expected->isThis());
// For the This, Self and Parent cases, we must always have a resolved class
// for the constraint
assertx(IMPLIES(
expected->isSelf() || expected->isParent(), constraint != nullptr));
expected->isSelf() || expected->isParent() || expected->isThis(),
constraint != nullptr
));
// If we have a resolved class for the constraint, all we have to do is
// check if the value's class is compatible with it
if (LIKELY(constraint != nullptr)) {
if (expected->isThis() && RuntimeOption::EvalThisTypeHintLevel >= 2) {
return cls == constraint;
}
return cls->classof(constraint);
}
// The Self and Parent cases should never reach here because they were
Expand Down
4 changes: 2 additions & 2 deletions hphp/runtime/vm/repo-global-data.h
Expand Up @@ -76,7 +76,7 @@ struct Repo::GlobalData {
* This changes program behavior because this type hints that are checked
* at runtime will enable additional HHBBC optimizations.
*/
bool CheckThisTypeHints = true;
int32_t ThisTypeHintLevel = 0;

/*
* Indicates whether a repo was compiled with HardPrivatePropInference.
Expand Down Expand Up @@ -160,7 +160,7 @@ struct Repo::GlobalData {
(InitialNamedEntityTableSize)
(InitialStaticStringTableSize)
(HardTypeHints)
(CheckThisTypeHints)
(ThisTypeHintLevel)
(HardReturnTypeHints)
(HardPrivatePropInference)
(DisallowDynamicVarEnvFuncs)
Expand Down
4 changes: 3 additions & 1 deletion hphp/runtime/vm/repo.cpp
Expand Up @@ -195,7 +195,6 @@ void Repo::loadGlobalData(bool allowFailure /* = false */,
HHBBC::options.ElideAutoloadInvokes = s_globalData.ElideAutoloadInvokes;
RuntimeOption::AutoprimeGenerators = s_globalData.AutoprimeGenerators;
RuntimeOption::EnableHipHopSyntax = s_globalData.EnableHipHopSyntax;
RuntimeOption::EvalCheckThisTypeHints = s_globalData.CheckThisTypeHints;
RuntimeOption::EvalHardTypeHints = s_globalData.HardTypeHints;
RuntimeOption::EvalUseHHBBC = s_globalData.UsedHHBBC;
RuntimeOption::PHP7_Builtins = s_globalData.PHP7_Builtins;
Expand All @@ -208,6 +207,9 @@ void Repo::loadGlobalData(bool allowFailure /* = false */,
if (s_globalData.HardReturnTypeHints) {
RuntimeOption::EvalCheckReturnTypeHints = 3;
}
if (s_globalData.ThisTypeHintLevel == 3) {
RuntimeOption::EvalThisTypeHintLevel = s_globalData.ThisTypeHintLevel;
}

if (RuntimeOption::ServerExecutionMode() &&
RuntimeOption::EvalHackArrCompatNotices) {
Expand Down
38 changes: 35 additions & 3 deletions hphp/runtime/vm/type-constraint.cpp
Expand Up @@ -23,6 +23,7 @@

#include "hphp/runtime/base/autoload-handler.h"
#include "hphp/runtime/base/runtime-error.h"
#include "hphp/runtime/vm/act-rec.h"
#include "hphp/runtime/vm/class.h"
#include "hphp/runtime/vm/func.h"
#include "hphp/runtime/vm/hhbc.h"
Expand Down Expand Up @@ -391,9 +392,26 @@ bool TypeConstraint::check(TypedValue* tv, const Func* func) const {
} else {
switch (metaType()) {
case MetaType::Self:
case MetaType::This:
selfToClass(func, &c);
break;
case MetaType::This:
switch (RuntimeOption::EvalThisTypeHintLevel) {
case 0: // Like Mixed.
return true;
break;
case 1: // Like Self.
selfToClass(func, &c);
break;
case 2: // Soft this in irgen verifyTypeImpl and verifyFail.
case 3: // Hard this.
thisToClass(&c);
if (c) {
return tv->m_data.pobj->getVMClass() == c;
}
return false;
break;
}
break;
case MetaType::Parent:
parentToClass(func, &c);
break;
Expand Down Expand Up @@ -525,7 +543,8 @@ void TypeConstraint::verifyFail(const Func* func, TypedValue* tv,
givenType
).str();
}
if (RuntimeOption::EvalCheckReturnTypeHints >= 2 && !isSoft()) {
if (RuntimeOption::EvalCheckReturnTypeHints >= 2 && !isSoft()
&& (!isThis() || RuntimeOption::EvalThisTypeHintLevel != 2)) {
raise_return_typehint_error(msg);
} else {
raise_warning_unsampled(msg);
Expand Down Expand Up @@ -555,7 +574,8 @@ void TypeConstraint::verifyFail(const Func* func, TypedValue* tv,
}

// Handle parameter type constraint failures
if (isExtended() && isSoft()) {
if (isExtended() &&
(isSoft() || (isThis() && RuntimeOption::EvalThisTypeHintLevel == 2))) {
// Soft extended type hints raise warnings instead of recoverable
// errors, to ease migration.
raise_warning_unsampled(
Expand Down Expand Up @@ -591,6 +611,18 @@ void TypeConstraint::verifyFail(const Func* func, TypedValue* tv,
}
}

void TypeConstraint::thisToClass(const Class **cls) const {
const ActRec* ar = vmfp();
if (ar->func()->cls()) {
if (ar->hasThis()) {
*cls = ar->getThis()->getVMClass();
} else {
assertx(ar->hasClass());
*cls = ar->getClass();
}
}
}

void TypeConstraint::selfToClass(const Func* func, const Class **cls) const {
const Class* c = func->cls();
if (c) {
Expand Down
1 change: 1 addition & 0 deletions hphp/runtime/vm/type-constraint.h
Expand Up @@ -262,6 +262,7 @@ struct TypeConstraint {

// Can not be private; used by the translator.
void selfToClass(const Func* func, const Class **cls) const;
void thisToClass(const Class **cls) const;
void parentToClass(const Func* func, const Class **cls) const;
void verifyFail(const Func* func, TypedValue* tv, int id,
bool useStrictTypes) const;
Expand Down

0 comments on commit 8075082

Please sign in to comment.