Skip to content

Commit

Permalink
Closures of an uninstantiable class should not be instantiable in HHB…
Browse files Browse the repository at this point in the history
…BC (v2)

Summary:
If a class is uninstantiable, then any closures within the
class should also be uninstantiable. Unfortunately we weren't getting
the logic right, leading to conditions where we could assert.

Fix this by detecting it in the FlattenJob, where it belongs.

The first attempt failed because there's some situations where a
closure may end up on a different shard than it's context class, and
thus never realize the context class is uninstantiable.

Reviewed By: mdko

Differential Revision: D51235728

fbshipit-source-id: f96c238b8e07db6a6ae24127a778759dfbe1b32c
ricklavoie authored and facebook-github-bot committed Nov 12, 2023

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
1 parent a282e9c commit 09f6283
Showing 3 changed files with 65 additions and 19 deletions.
69 changes: 50 additions & 19 deletions hphp/hhbbc/index.cpp
Original file line number Diff line number Diff line change
@@ -7574,6 +7574,10 @@ struct FlattenJob {
}()
);

for (auto const& cls : uninstantiable.vals) {
always_assert(index.m_uninstantiable.emplace(cls->name).second);
}

std::vector<std::unique_ptr<php::Class>> newClosures;
newClosures.reserve(worklist.size());

@@ -7662,19 +7666,32 @@ struct FlattenJob {
outMethods.vals.emplace_back(std::move(methods));
};

// If a closure's context is uninstantiable, then so is the
// closure.
auto const isContextInstantiable = [&] (const php::Class& cls) {
if (!cls.closureContextCls) return true;
if (index.uninstantiable(cls.closureContextCls)) {
always_assert(!index.m_classInfos.count(cls.closureContextCls));
return false;
}
return true;
};

// Do the processing which relies on a fully accessible
// LocalIndex

ISStringToOneT<InterfaceConflicts> ifaceConflicts;
for (auto& cls : classes.vals) {
auto const cinfoIt = index.m_classInfos.find(cls->name);
if (cinfoIt == end(index.m_classInfos)) {
if (cinfoIt == end(index.m_classInfos) || !isContextInstantiable(*cls)) {
ITRACE(
4, "{} discovered to be not instantiable, instead "
"creating MethodsWithoutCInfo for it\n",
cls->name
);
always_assert(index.uninstantiable(cls->name));
always_assert(
IMPLIES(!cls->closureContextCls, index.uninstantiable(cls->name))
);
makeMethodsWithoutCInfo(*cls);
continue;
}
@@ -7773,7 +7790,7 @@ struct FlattenJob {
for (auto& cls : classes.vals) {
auto const name = cls->name;
auto const cinfoIt = index.m_classInfos.find(name);
if (cinfoIt == end(index.m_classInfos)) {
if (cinfoIt == end(index.m_classInfos) || !isContextInstantiable(*cls)) {
assertx(outMeta.uninstantiable.count(name));
continue;
}
@@ -10063,7 +10080,7 @@ struct IndexFlattenMetadata {
size_t idx; // Index into allCls vector
bool isClosure{false};
bool uninstantiable{false};
SString closureContext;
SString closureContext{nullptr};
};
ISStringToOneT<ClassMeta> cls;
// All classes to be flattened
@@ -10374,26 +10391,40 @@ flatten_classes_assign(IndexFlattenMetadata& meta) {

for (auto const d : deps) onDep(d);
for (auto const clo : closures) onDep(clo);

if (out.instantiable) return out;
// If a class is not instantiable, then any of the closures
// which have it as a context are not either. Remove them as
// deps.
folly::erase_if(
out.deps,
[&] (SString d) {
auto const cloMeta = folly::get_ptr(meta.cls, d);
return
cloMeta &&
cloMeta->closureContext &&
cloMeta->closureContext->isame(cls);
}
);
return out;
}
);
};

// If a closure's context class is not uninstantiable, then the
// closure is likewise not instantiable. Calculate it here (we
// cannot do it in findAllDeps because it creates a circular
// dependency between the closure and the context class).
parallel::for_each(
meta.allCls,
[&] (SString cls) {
auto& clsMeta = meta.cls.at(cls);
if (!clsMeta.closureContext) return;
ISStringSet visited;
auto const& lookup =
findAllDeps(clsMeta.closureContext, visited, findAllDeps);
if (!lookup.instantiable) {
if (!clsMeta.uninstantiable) {
FTRACE(
4,
"{} is not instantiable because the "
"closure context {} is not instantiable\n",
cls, clsMeta.closureContext
);
}
clsMeta.uninstantiable = true;
}
}
);
// Reset the lookup results so that further lookups will correctly
// incorporate non-instantiable closures.
parallel::for_each(allDeps, [] (LockFreeLazy<DepLookup>& l) { l.reset(); });

constexpr size_t kBucketSize = 2000;
constexpr size_t kMaxBucketSize = 30000;

14 changes: 14 additions & 0 deletions hphp/test/slow/hhbbc/uninstantiable-closure2.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
<?hh

function bar($c) { return $c(1); }

class A {}

enum class B : int extends A {
int FOO = bar($x ==> $x + 1);
}

<<__EntryPoint>>
function main() {
var_dump(B::FOO);
}
1 change: 1 addition & 0 deletions hphp/test/slow/hhbbc/uninstantiable-closure2.php.expectf
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fatal error: B cannot include A - it is not an enum class in %s/uninstantiable-closure2.php on line 7

0 comments on commit 09f6283

Please sign in to comment.