Make trait constants behave like interface constants

Summary:
Trait constants were added to the language recently to obviate a workaround where traits themselves could not declare constants, but could bring in constants via interfaces. However, the semantics on conflict were to drop the conflicting constant; this is in contrast to interface conflicts, where you get a fatal at class loading time.

```
class A {
  const int X = 3;
}
interface I {
  const string X = "hello";
}

class C extends A implements I {} // Fatal runtime error

trait TI implements I {}
class D extends A { use TI; } // no fatal

// by extension
trait T { const bool X = false; }
class E extends A { use T; } // also no fatal
```
This flag allows the above cases to fatal, and it also fixes the behavior of defaults + makes HHVM align with Hack on member resolution order.

```
abstract class A {
  abstract const type T = arraykey;
}
trait T {
  const type T = int;
}
class C extends A { use T; } // C::T = int, was arraykey previously.
```
Equivalently, coeffect unsoundness given trait conflicts is resolved
```
abstract class A {
  abstract const ctx C = [];
}
trait T {
  const ctx C = [defaults];
  public function f()[this::C]: void {
    echo "impure";
  }
}
class C extends A { use T; } // runtime used to think (new C())->f() is pure, now it's correctly defaults.
```

The flag also changes HHBBC to insert trait constants before inserting locally declared constants of a class, which resolves a bug with printing the conflict error message and a static analysis failure where the trait constant was expecting to conflict with a shallowly declared constant (see trait_slot_conflict_repo.php).

Reviewed By: oulgen

Differential Revision: D28887745

fbshipit-source-id: a698e1818925ba17c2608ce34d50dc1400cfabd0

Author: vassilmladenov

hphp/compiler/analysis/emitter.cpp

@@ -183,6 +183,8 @@ RepoGlobalData getGlobalData() {
     RuntimeOption::EvalNoticeOnCoerceForStrConcat;
   gd.NoticeOnCoerceForBitOp =
     RuntimeOption::EvalNoticeOnCoerceForBitOp;
+  gd.TraitConstantInterfaceBehavior =
+    RuntimeOption::EvalTraitConstantInterfaceBehavior;
 
   for (auto const& elm : RuntimeOption::ConstantFunctions) {
     auto const s = internal_serialize(tvAsCVarRef(elm.second));

hphp/hhbbc/index.cpp

@@ -1708,19 +1708,24 @@ bool build_class_constants(ClassInfo* cinfo, ClsPreResolveUpdates& updates) {
         }
       }
 
-      // Constants from traits silently lose
-      if (fromTrait) {
-        removeNoOverride(cns);
-        return true;
+      if (!RO::EvalTraitConstantInterfaceBehavior) {
+        // Constants from traits silently lose
+        if (fromTrait) {
+          removeNoOverride(cns);
+          return true;
+        }
       }
 
-      if ((cns->cls->attrs & (AttrInterface)) && existing->isAbstract) {
+      if ((cns->cls->attrs & AttrInterface ||
+           (RO::EvalTraitConstantInterfaceBehavior && (cns->cls->attrs & AttrTrait))) &&
+          existing->isAbstract) {
         // because existing has val, this covers the case where it is abstract with default
         // allow incoming to win
       } else {
         // A constant from an interface or from an included enum collides
         // with an existing constant.
-        if (cns->cls->attrs & (AttrInterface | AttrEnum | AttrEnumClass)) {
+        if (cns->cls->attrs & (AttrInterface | AttrEnum | AttrEnumClass) ||
+            (RO::EvalTraitConstantInterfaceBehavior && (cns->cls->attrs & AttrTrait))) {
           ITRACE(
             2,
             "build_class_constants failed for `{}' because "
@@ -1754,16 +1759,32 @@ bool build_class_constants(ClassInfo* cinfo, ClsPreResolveUpdates& updates) {
     }
   }
 
-  for (uint32_t idx = 0; idx < cinfo->cls->constants.size(); ++idx) {
-    auto const cns = ClassInfo::ConstIndex { cinfo->cls, idx };
-    if (cinfo->cls->attrs & AttrTrait) removeNoOverride(cns);
-    if (!add(cns, false)) return false;
-  }
+  auto const addShallowConstants = [&]() {
+    for (uint32_t idx = 0; idx < cinfo->cls->constants.size(); ++idx) {
+      auto const cns = ClassInfo::ConstIndex { cinfo->cls, idx };
+      if (cinfo->cls->attrs & AttrTrait) removeNoOverride(cns);
+      if (!add(cns, false)) return false;
+    }
+    return true;
+  };
 
-  for (auto const trait : cinfo->usedTraits) {
-    for (auto const& cns : trait->clsConstants) {
-      if (!add(cns.second, true)) return false;
+  auto const addTraitConstants = [&]() {
+    for (auto const trait : cinfo->usedTraits) {
+      for (auto const& cns : trait->clsConstants) {
+        if (!add(cns.second, true)) return false;
+      }
     }
+    return true;
+  };
+
+  if (RO::EvalTraitConstantInterfaceBehavior) {
+    // trait constants must be inserted before constants shallowly declared on the class
+    // to match the interface semantics
+    if (!addTraitConstants()) return false;
+    if (!addShallowConstants()) return false;
+  } else {
+    if (!addShallowConstants()) return false;
+    if (!addTraitConstants()) return false;
   }
 
   for (auto const ienum : cinfo->includedEnums) {

hphp/hhbbc/main.cpp

@@ -339,6 +339,8 @@ RepoGlobalData get_global_data() {
     RuntimeOption::EvalNoticeOnCoerceForStrConcat;
   gd.NoticeOnCoerceForBitOp =
     RuntimeOption::EvalNoticeOnCoerceForBitOp;
+  gd.TraitConstantInterfaceBehavior =
+    RuntimeOption::EvalTraitConstantInterfaceBehavior;
 
   for (auto const& elm : RuntimeOption::ConstantFunctions) {
     auto const s = internal_serialize(tvAsCVarRef(elm.second));
@@ -524,6 +526,7 @@ int main(int argc, char** argv) try {
   RO::EvalHackArrDVArrs                         = true; // TODO(kshaunak): Clean it up.
   RO::EvalArrayProvenance                       = false; // TODO(kshaunak): Clean it up.
   RO::EvalEnforceGenericsUB                     = gd.HardGenericsUB ? 2 : 1;
+  RO::EvalTraitConstantInterfaceBehavior        = gd.TraitConstantInterfaceBehavior;
 
   if (print_bytecode_stats_and_exit) {
     print_repo_bytecode_stats();

hphp/runtime/base/runtime-option.h

@@ -1417,6 +1417,7 @@ struct RuntimeOption {
   F(bool, EnableAbstractContextConstants, true)                         \
   F(bool, TypeconstAbstractDefaultReflectionIsAbstract, false)          \
   F(bool, AbstractContextConstantUninitAccess, false)                   \
+  F(bool, TraitConstantInterfaceBehavior, false)                       \
   /* */
 
 private:

hphp/runtime/base/unit-cache.cpp

@@ -1365,6 +1365,7 @@ std::string mangleUnitSha1(const std::string& fileSha1,
     + (RuntimeOption::EvalFoldLazyClassKeys ? '1' : '0')
     + (RuntimeOption::EvalHackCompilerUseCompilerPool ? '1' : '0')
     + (RuntimeOption::EvalEnableAbstractContextConstants ? '1': '0')
+    + (RuntimeOption::EvalTraitConstantInterfaceBehavior ? '1' : '0')
     + RuntimeOption::EvalUnitCacheBreaker + '\0'
     + CoeffectsConfig::mangle()
     + opts.cacheKeySha1().toString()

hphp/runtime/vm/class.cpp

@@ -2281,28 +2281,54 @@ void Class::importTraitConsts(ConstMap::Builder& builder) {
       return;
     }
 
-    // Constants in interfaces implemented by traits don't fatal with constants
-    // in declInterfaces
-    if (isFromInterface) { return; }
-
-    // Type and Context constants in interfaces can be overriden.
-    if (tConst.kind() == ConstModifiers::Kind::Type ||
-        tConst.kind() == ConstModifiers::Kind::Context)  {
-      return;
-    }
-    if (existingConst.cls != tConst.cls) {
+    if (RO::EvalTraitConstantInterfaceBehavior) {
+      if (existingConst.isAbstract()) {
+        // the case where the incoming constant is abstract without a default is covered above
+        // there are two remaining cases:
+        //   - the incoming constant is abstract with a default
+        //   - the incoming constant is concrete
+        // In both situations, the incoming constant should win, and separate bookkeeping will
+        // cover situations where there are multiple competing defaults.
+        existingConst.cls = tConst.cls;
+        existingConst.val = tConst.val;
+        return;
+      } else { // existing is concrete
+        // the existing constant will win over any incoming abstracts and retain a fatal when two
+        // concrete constants collide
+        if (!tConst.isAbstract() && existingConst.cls != tConst.cls) {
+          raise_error("%s cannot inherit the %s %s from %s, because "
+                      "it was previously inherited from %s",
+                      m_preClass->name()->data(),
+                      ConstModifiers::show(tConst.kind()),
+                      tConst.name->data(),
+                      tConst.cls->name()->data(),
+                      existingConst.cls->name()->data());
+        }
+      }
+    } else {
+      // Constants in interfaces implemented by traits don't fatal with constants
+      // in declInterfaces
+      if (isFromInterface) { return; }
 
-      // Constants in traits conflict with constants in declared interfaces
-      if (existingConst.cls->attrs() & AttrInterface) {
-        for (auto const& interface : m_declInterfaces) {
-          auto iface = existingConst.cls;
-          if (interface.get() == iface) {
-            raise_error("%s cannot inherit the %s %s, because "
-                        "it was previously inherited from %s",
-                        m_preClass->name()->data(),
-                        ConstModifiers::show(tConst.kind()),
-                        tConst.name->data(),
-                        existingConst.cls->name()->data());
+      // Type and Context constants in interfaces can be overriden.
+      if (tConst.kind() == ConstModifiers::Kind::Type ||
+          tConst.kind() == ConstModifiers::Kind::Context)  {
+        return;
+      }
+      if (existingConst.cls != tConst.cls) {
+
+        // Constants in traits conflict with constants in declared interfaces
+        if (existingConst.cls->attrs() & AttrInterface) {
+          for (auto const& interface : m_declInterfaces) {
+            auto iface = existingConst.cls;
+            if (interface.get() == iface) {
+              raise_error("%s cannot inherit the %s %s, because "
+                          "it was previously inherited from %s",
+                          m_preClass->name()->data(),
+                          ConstModifiers::show(tConst.kind()),
+                          tConst.name->data(),
+                          existingConst.cls->name()->data());
+            }
           }
         }
       }

hphp/runtime/vm/repo-global-data.cpp

@@ -47,6 +47,7 @@ void RepoGlobalData::load(bool loadConstantFuncs) const {
   RO::EvalNoticeOnCoerceForStrConcat            = NoticeOnCoerceForStrConcat;
   RO::EvalNoticeOnCoerceForBitOp                = NoticeOnCoerceForBitOp;
   RO::EvalHackArrDVArrs                         = true; // TODO(kshaunak): Clean up.
+  RO::EvalTraitConstantInterfaceBehavior        = TraitConstantInterfaceBehavior;
 
   if (HardGenericsUB) RO::EvalEnforceGenericsUB = 2;
 
@@ -105,7 +106,7 @@ std::string show(const RepoGlobalData& gd) {
   SHOW(StrictArrayFillKeys);
   SHOW(NoticeOnCoerceForStrConcat);
   SHOW(NoticeOnCoerceForBitOp);
-  SHOW(TypeconstInterfaceInheritanceDefaults);
+  SHOW(TraitConstantInterfaceBehavior);
 #undef SHOW
   return out;
 }

hphp/runtime/vm/repo-global-data.h

@@ -161,8 +161,8 @@ struct RepoGlobalData {
   /* Whether implicit coercions for bit ops trigger logs/exceptions */
   int32_t NoticeOnCoerceForBitOp = 0;
 
-  /* New behavior for inheritance of abstract type constants with defaults */
-  bool TypeconstInterfaceInheritanceDefaults = false;
+  /* Constants from traits behave like constants from interfaces (error on conflict) */
+  bool TraitConstantInterfaceBehavior = false;
 
   /*
    * The Hack.Lang.StrictArrayFillKeys option the repo was compiled with.
@@ -215,7 +215,7 @@ struct RepoGlobalData {
       (StrictArrayFillKeys)
       (NoticeOnCoerceForStrConcat)
       (NoticeOnCoerceForBitOp)
-      (TypeconstInterfaceInheritanceDefaults)
+      (TraitConstantInterfaceBehavior)
       (ConstantFunctions)
       ;
   }

hphp/test/slow/constants/config.ini

@@ -1,3 +1,4 @@
 hhvm.enable_abstract_context_constants=1
 hhvm.typeconst_abstract_default_reflection_is_abstract=1
 hhvm.abstract_context_constant_uninit_access=1
+hhvm.trait_constant_interface_behavior=1

hphp/test/slow/constants/hphp_config.ini

@@ -1,3 +1,4 @@
 hhvm.enable_abstract_context_constants=1
 hhvm.typeconst_abstract_default_reflection_is_abstract=1
 hhvm.abstract_context_constant_uninit_access=1
+hhvm.trait_constant_interface_behavior=1