Skip to content
Permalink
Browse files

Stop implicitly coercing collections to arrays in func args.

Summary:
This coercion is left over from when we were trying to migrate from arrays to collections. We're migrating to hack arrays now, so this has outlived its usefulness. Delete the code that implements it, adjust the test.

This code happened to be the last user of Func::mustBeRef, an awful manual whitelist of builtins that were allowed to take some (or all) their arguments either by reference or by value. Delete this also.

Reviewed By: paulbiss

Differential Revision: D13793474

fbshipit-source-id: 2e8421ee117b168ede3d5ebaa1ba3c549ea49e8b
  • Loading branch information...
alexeyt authored and hhvm-bot committed Jan 27, 2019
1 parent 2de3c23 commit 471d41d5ae9acd7e95afde788aab0c3781a15d46
@@ -477,38 +477,6 @@ bool Func::byRef(int32_t arg) const {
return *ref & (1ull << bit); return *ref & (1ull << bit);
} }


const StaticString s_extract("extract");
const StaticString s_current("current");
const StaticString s_key("key");
const StaticString s_array_multisort("array_multisort");

bool Func::mustBeRef(int32_t arg) const {
if (!byRef(arg)) return false;
if (arg == 0) {
if (UNLIKELY(m_attrs & AttrBuiltin)) {
// This hacks mustBeRef() to return false for the first parameter of
// extract(), current(), key(), and array_multisort(). These functions
// try to take their first parameter by reference but they also allow
// expressions that cannot be taken by reference (ex. an array literal).
// TODO Task #4442937: Come up with a cleaner way to do this.
if (cls()) return true;
auto const name = displayName();
if (name == s_extract.get()) return false;
if (name == s_current.get()) return false;
if (name == s_key.get()) return false;
if (name == s_array_multisort.get()) return false;
}
}

// We force mustBeRef() to return false for array_multisort(). It tries to
// pass all variadic arguments by reference, but it also allow expressions
// that cannot be taken by reference (ex. SORT_REGULAR flag).
return
arg < numParams() ||
!(m_attrs & AttrVariadicByRef) ||
!(name() == s_array_multisort.get() && !cls());
}



/////////////////////////////////////////////////////////////////////////////// ///////////////////////////////////////////////////////////////////////////////
// Locals, iterators, and stack. // Locals, iterators, and stack.
@@ -527,23 +527,15 @@ struct Func final {
uint32_t numNonVariadicParams() const; uint32_t numNonVariadicParams() const;


/* /*
* Whether the arg-th parameter /may/ be taken by reference. * Whether the arg-th parameter are taken by reference.
*/ */
bool byRef(int32_t arg) const; bool byRef(int32_t arg) const;


/* /*
* Whether any parameters /may/ be taken by reference. * Whether any parameters are taken by reference.
*/ */
bool anyByRef() const; bool anyByRef() const;


/*
* Whether the arg-th parameter /must/ be taken by reference.
*
* Some builtins take positional or variadic arguments only optionally by
* ref, hence the distinction.
*/
bool mustBeRef(int32_t arg) const;

/* /*
* Whether the function is declared with a `...' parameter. * Whether the function is declared with a `...' parameter.
*/ */
@@ -154,7 +154,6 @@ SSATmp* implInstanceCheck(IRGS& env, SSATmp* src, const StringData* className,
* - GetVal: Return the SSATmp of the value to test * - GetVal: Return the SSATmp of the value to test
* - PredInner: When the value is a BoxedInitCell, return the predicted inner * - PredInner: When the value is a BoxedInitCell, return the predicted inner
* type of the value. * type of the value.
* - ColToArr: Emit code to deal with any collection to array conversions.
* - FuncToStr: Emit code to deal with any func to string conversions. * - FuncToStr: Emit code to deal with any func to string conversions.
* - Fail: Emit code to deal with the type check failing. * - Fail: Emit code to deal with the type check failing.
* - HackArr: Emit code to deal with a d/varray mismatch. * - HackArr: Emit code to deal with a d/varray mismatch.
@@ -169,7 +168,6 @@ SSATmp* implInstanceCheck(IRGS& env, SSATmp* src, const StringData* className,
*/ */
template <typename GetVal, template <typename GetVal,
typename PredInner, typename PredInner,
typename ColToArr,
typename FuncToStr, typename FuncToStr,
typename ClassToStr, typename ClassToStr,
typename Fail, typename Fail,
@@ -183,7 +181,6 @@ void verifyTypeImpl(IRGS& env,
SSATmp* propCls, SSATmp* propCls,
GetVal getVal, GetVal getVal,
PredInner predInner, PredInner predInner,
ColToArr colToArr,
FuncToStr funcToStr, FuncToStr funcToStr,
ClassToStr classToStr, ClassToStr classToStr,
Fail fail, Fail fail,
@@ -210,7 +207,6 @@ void verifyTypeImpl(IRGS& env,
if (!valType.isKnownDataType()) return giveup(); if (!valType.isKnownDataType()) return giveup();


if (tc.isNullable() && valType <= TInitNull) return; if (tc.isNullable() && valType <= TInitNull) return;
colToArr(valType);


auto const genFail = [&] { auto const genFail = [&] {
auto const thisFailsHard = [&] { auto const thisFailsHard = [&] {
@@ -1101,7 +1097,6 @@ void verifyRetTypeImpl(IRGS& env, int32_t id, bool onlyCheckNullability) {
[] (SSATmp*) -> Type { // Get boxed inner value [] (SSATmp*) -> Type { // Get boxed inner value
PUNT(VerifyReturnTypeBoxed); PUNT(VerifyReturnTypeBoxed);
}, },
[] (Type) {}, // Collection to array conversion
[&] (SSATmp* val) { // func to string conversions [&] (SSATmp* val) { // func to string conversions
auto const str = gen(env, LdFuncName, val); auto const str = gen(env, LdFuncName, val);
discard(env, 1); discard(env, 1);
@@ -1176,12 +1171,6 @@ void verifyParamTypeImpl(IRGS& env, int32_t id) {
[&] (SSATmp* val) { // Get boxed inner type [&] (SSATmp* val) { // Get boxed inner type
return env.irb->predictedLocalInnerType(id); return env.irb->predictedLocalInnerType(id);
}, },
[&] (Type valType) { // Collection to array conversion
if (tc.isArray() && !tc.isSoft() &&
!func->mustBeRef(id) && valType <= TObj) {
PUNT(VerifyParamType-collectionToArray);
}
},
[&] (SSATmp* val) { // func to string conversions [&] (SSATmp* val) { // func to string conversions
auto const str = gen(env, LdFuncName, val); auto const str = gen(env, LdFuncName, val);
stLocRaw(env, id, fp(env), str); stLocRaw(env, id, fp(env), str);
@@ -1265,7 +1254,6 @@ void verifyPropType(IRGS& env,
// We've already asserted that the value is a Cell. // We've already asserted that the value is a Cell.
always_assert(false); always_assert(false);
}, },
[&] (Type) {}, // No collection to array automatic conversions
[&] (SSATmp*) { return false; }, // No func to string automatic conversions [&] (SSATmp*) { return false; }, // No func to string automatic conversions
[&] (SSATmp*) { return false; }, // No class to string automatic conversions [&] (SSATmp*) { return false; }, // No class to string automatic conversions
[&] (Type, bool hard) { // Check failure [&] (Type, bool hard) { // Check failure
@@ -1218,26 +1218,6 @@ void TypeConstraint::verifyFail(const Func* func, TypedValue* tv,
return; return;
} }


// Handle implicit collection->array conversion for array parameter type
// constraints
if (isArray() && !isSoft() && !func->mustBeRef(id) &&
c->m_type == KindOfObject && c->m_data.pobj->isCollection()) {
// To ease migration, the 'array' type constraint will implicitly cast
// collections to arrays, provided the type constraint is not soft and
// the parameter is not by reference. We raise a notice to let the user
// know that there was a type mismatch and that an implicit conversion
// was performed.
raise_notice(
folly::format(
"Argument {} to {}() must be of type {}, {} given; argument {} was "
"implicitly cast to array",
id + 1, func->fullDisplayName(), name, givenType, id + 1
).str()
);
tvCastToArrayInPlace(tv);
return;
}

// Handle parameter type constraint failures // Handle parameter type constraint failures
if (isExtended() && if (isExtended() &&
(isSoft() || (isThis() && RuntimeOption::EvalThisTypeHintLevel == 2))) { (isSoft() || (isThis() && RuntimeOption::EvalThisTypeHintLevel == 2))) {
Oops, something went wrong.

0 comments on commit 471d41d

Please sign in to comment.
You can’t perform that action at this time.