Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Remove class-loading check for overriding type constants
Summary:
This fatals, but hh is fine with it

```
interface I1 {
  abstract const type T = nothing;
}
interface I2 extends I1 {
  const type T = mixed;
}
```

I went digging and discovered that HHVM doesn't understand that the type const in I1 is abstract. In fact the abstract keyword doesn't result in any emission differences.

This is the rust structure:
```
pub struct HhasTypeConstant {
    pub name: String,
    pub initializer: Option<TypedValue>,
}
```

It's just "Is there a value? no -> abstract, else -> non-abstract"

Then in the class loading code we have this:
```
// Forbid redefining constants from interfaces, but not superclasses.
// Constants from interfaces implemented by superclasses can be
// overridden.
```

This is super weird IMO. At the very least we need to not fatal on this case (it has direct applications for coeffects). In the best case, we'd probably have HHVM understand the notion of abstract w/ default.

As I see it there are basically 2 options:
1. delete the check (for type constants). It seems weird enough as it is that it's the only restrictions on redefinitions. Then again, that results in HHVM not having any confirmation that the hierarchy is reasonable, but we don't enforce them anyway, so that might be OK?
2. do a bunch of work to redo how HHVM sees type constants and potentially add more checks.

I'm going to do the first here as a short term solution and then we can discuss the latter in more detail.

Reviewed By: paulbiss

Differential Revision: D26076753

fbshipit-source-id: 6aedfc10d4ab31fc61782adbadd09203aa8b9cc8
  • Loading branch information
DavidSnider authored and facebook-github-bot committed Jan 27, 2021
1 parent 534d890 commit 206760e
Show file tree
Hide file tree
Showing 4 changed files with 16 additions and 8 deletions.
2 changes: 1 addition & 1 deletion hphp/hhbbc/index.cpp
Expand Up @@ -1544,7 +1544,7 @@ bool build_class_constants(BuildClsInfo& info,

// A constant from an interface or from an included enum collides
// with an existing constant.
if (rparent->cls->attrs & (AttrInterface | AttrEnum)) {
if (rparent->cls->attrs & (AttrInterface | AttrEnum) && !c.isTypeconst) {
ITRACE(2,
"build_cls_info_rec failed for `{}' because "
"`{}' was defined by both `{}' and `{}'\n",
Expand Down
12 changes: 6 additions & 6 deletions hphp/runtime/vm/class.cpp
Expand Up @@ -2299,15 +2299,15 @@ void Class::setConstants() {
if (it2 != builder.end()) {
auto definingClass = builder[it2->second].cls;
// Forbid redefining constants from interfaces, but not superclasses.
// Constants from interfaces implemented by superclasses can be
// overridden.
// Constants from interfaces implemented by superclasses can be overridden.
// Note that we don't do this check for type constants due to the
// existence of abstract type constants with defaults, which we don't
// currently have a way of tracking within HHVM.
if (definingClass->attrs() & AttrInterface) {
for (auto interface : m_declInterfaces) {
if (interface->hasConstant(preConst->name()) ||
interface->hasTypeConstant(preConst->name())) {
raise_error("Cannot override previously defined %sconstant "
if (interface->hasConstant(preConst->name())) {
raise_error("Cannot override previously defined constant "
"%s::%s in %s",
builder[it2->second].isType() ? "type " : "",
builder[it2->second].cls->name()->data(),
preConst->name()->data(),
m_preClass->name()->data());
Expand Down
8 changes: 8 additions & 0 deletions hphp/test/slow/class_type_constant/type_constant14.php
Expand Up @@ -8,7 +8,15 @@ class C implements I {
const type T = string;
}

interface I2 extends I {
const type T = string;
}

class C2 implements I2 {}

<<__EntryPoint>>
function main(): void {
new C();
new C2();
echo "Done.\n";
}
@@ -1 +1 @@
Fatal error: Cannot override previously defined type constant I::T in C in %stest/slow/class_type_constant/type_constant14.php on line 7
Done.

0 comments on commit 206760e

Please sign in to comment.