split isame() into tsame() + fsame()

Summary: Let's take a divide & conquer approach to fixing dynamic case collisions by separating function and type name collisions. Unfortunately, throughout HHVM we have hardcoded how names of each kind should be compared by explicitly calling isame(), istrcmp(), etc. A better design would have been to wrap names in zero-overhead newtype structs so their operations could be implemented in one place, and C++ type safety could help us properly juggle multiple name types. Anyway, this takes a first step; I cloned isame() and istrcmp() into t/fsame() and t/fstrcmp(), then propagated the split through the codebase. This gives me a way to segregate logs while also improving type safety in the HHVM code. Reviewed By: aorenste Differential Revision: D52598964 fbshipit-source-id: 81600b87c41adba3d07262b570016ab5710d50e1
facebook · Jan 9, 2024 · 351126a · 351126a
1 parent bcea048
commit 351126a
Show file tree

Hide file tree

Showing 70 changed files with 713 additions and 538 deletions.
diff --git a/hphp/compiler/compiler.cpp b/hphp/compiler/compiler.cpp
@@ -421,11 +421,17 @@ struct SymbolSets {
     };
   }
 
-  using IMap = folly_concurrent_hash_map_simd<
+  using TMap = folly_concurrent_hash_map_simd<
     const StringData*,
     const StringData*,
     string_data_hash,
-    string_data_isame
+    string_data_tsame
+  >;
+  using FMap = folly_concurrent_hash_map_simd<
+    const StringData*,
+    const StringData*,
+    string_data_hash,
+    string_data_fsame
   >;
   using Map = folly_concurrent_hash_map_simd<
     const StringData*,
@@ -434,10 +440,10 @@ struct SymbolSets {
     string_data_same
   >;
 
-  IMap enums;
-  IMap classes;
-  IMap funcs;
-  IMap typeAliases;
+  TMap enums;
+  TMap classes;
+  FMap funcs;
+  TMap typeAliases;
   Map constants;
   Map modules;
   Map units;

diff --git a/hphp/compiler/decl-provider.h b/hphp/compiler/decl-provider.h
@@ -50,15 +50,18 @@ struct BatchDeclProvider final: hackc::DeclProvider {
   // Maps from Name to serialized inside the UnitDecls given in the
   // constructor.
   using Map = hphp_fast_map<const StringData*, const std::string&>;
-  using IMap = hphp_fast_map<
-    const StringData*, const std::string&, string_data_hash, string_data_isame
+  using TMap = hphp_fast_map<
+    const StringData*, const std::string&, string_data_hash, string_data_tsame
+  >;
+  using FMap = hphp_fast_map<
+    const StringData*, const std::string&, string_data_hash, string_data_fsame
   >;
 
   // Symbols requested but not found
   Package::DeclNames m_missing;
 
-  IMap m_types;
-  IMap m_funcs;
+  TMap m_types;
+  FMap m_funcs;
   Map m_constants;
   Map m_modules;
 };

diff --git a/hphp/compiler/package.h b/hphp/compiler/package.h
@@ -381,10 +381,15 @@ struct UnitIndex final {
     extern_worker::Ref<Package::UnitDecls> declsRef;
   };
 
-  using IMap = folly_concurrent_hash_map_simd<
+  using TMap = folly_concurrent_hash_map_simd<
     const StringData*, std::shared_ptr<Locations>,
     string_data_hash,
-    string_data_isame
+    string_data_tsame
+  >;
+  using FMap = folly_concurrent_hash_map_simd<
+    const StringData*, std::shared_ptr<Locations>,
+    string_data_hash,
+    string_data_fsame
   >;
   using Map = folly_concurrent_hash_map_simd<
     const StringData*, std::shared_ptr<Locations>
@@ -394,8 +399,8 @@ struct UnitIndex final {
   // symbol that is present in this index.
   bool containsAnyMissing(const Package::ParseMetaVec& parseMetas) const;
 
-  IMap types;
-  IMap funcs;
+  TMap types;
+  FMap funcs;
   Map constants;
   Map modules;
 };

diff --git a/hphp/hack/src/parser/rust_parser_errors.rs b/hphp/hack/src/parser/rust_parser_errors.rs
@@ -172,27 +172,29 @@ use NamespaceType::*;
 #[derive(Clone, Debug)]
 enum Strmap<X> {
     YesCase(HashMap<String, X>),
-    NoCase(HashMap<String, X>),
+    FuncCase(HashMap<String, X>),
+    TypeCase(HashMap<String, X>),
+    NsCase(HashMap<String, X>),
 }
 
 impl<X> Strmap<X> {
     fn mem(&self, k: &str) -> bool {
         match &self {
-            NoCase(m) => m.contains_key(&k.to_ascii_lowercase()),
+            FuncCase(m) | TypeCase(m) | NsCase(m) => m.contains_key(&k.to_ascii_lowercase()),
             YesCase(m) => m.contains_key(k),
         }
     }
 
     fn add(&mut self, k: &str, v: X) {
         match self {
-            NoCase(m) => m.insert(k.to_ascii_lowercase(), v),
+            FuncCase(m) | TypeCase(m) | NsCase(m) => m.insert(k.to_ascii_lowercase(), v),
             YesCase(m) => m.insert(k.to_string(), v),
         };
     }
 
     fn get(&self, k: &str) -> Option<&X> {
         match &self {
-            NoCase(m) => m.get(&k.to_ascii_lowercase()),
+            FuncCase(m) | TypeCase(m) | NsCase(m) => m.get(&k.to_ascii_lowercase()),
             YesCase(m) => m.get(k),
         }
     }
@@ -202,7 +204,9 @@ impl<X> Strmap<X> {
         F: Fn(&X) -> bool,
     {
         match self {
-            NoCase(m) => NoCase(m.into_iter().filter(|(_, x)| f(x)).collect()),
+            FuncCase(m) => FuncCase(m.into_iter().filter(|(_, x)| f(x)).collect()),
+            TypeCase(m) => TypeCase(m.into_iter().filter(|(_, x)| f(x)).collect()),
+            NsCase(m) => NsCase(m.into_iter().filter(|(_, x)| f(x)).collect()),
             YesCase(m) => YesCase(m.into_iter().filter(|(_, x)| f(x)).collect()),
         }
     }
@@ -211,24 +215,24 @@ impl<X> Strmap<X> {
 use crate::Strmap::*;
 
 fn empty_trait_require_clauses() -> Strmap<TokenKind> {
-    NoCase(HashMap::default())
+    TypeCase(HashMap::default())
 }
 
 #[derive(Clone, Debug)]
 struct UsedNames {
-    classes: Strmap<FirstUseOrDef>,    // NoCase
-    namespaces: Strmap<FirstUseOrDef>, // NoCase
-    functions: Strmap<FirstUseOrDef>,  // NoCase
+    classes: Strmap<FirstUseOrDef>,    // TypeCase
+    namespaces: Strmap<FirstUseOrDef>, // NsCase
+    functions: Strmap<FirstUseOrDef>,  // FuncCase
     constants: Strmap<FirstUseOrDef>,  // YesCase
     attributes: Strmap<FirstUseOrDef>, // YesCase
 }
 
 impl UsedNames {
     fn empty() -> Self {
         Self {
-            classes: NoCase(HashMap::default()),
-            namespaces: NoCase(HashMap::default()),
-            functions: NoCase(HashMap::default()),
+            classes: TypeCase(HashMap::default()),
+            namespaces: NsCase(HashMap::default()),
+            functions: FuncCase(HashMap::default()),
             constants: YesCase(HashMap::default()),
             attributes: YesCase(HashMap::default()),
         }
@@ -5624,7 +5628,7 @@ impl<'a, State: 'a + Clone> ParserErrors<'a, State> {
         // Reset the function declarations
 
         let constants = std::mem::replace(&mut self.names.constants, YesCase(HashMap::default()));
-        let functions = std::mem::replace(&mut self.names.functions, NoCase(HashMap::default()));
+        let functions = std::mem::replace(&mut self.names.functions, FuncCase(HashMap::default()));
         let trait_require_clauses = std::mem::replace(
             &mut self.trait_require_clauses,
             empty_trait_require_clauses(),

diff --git a/hphp/hhbbc/check.cpp b/hphp/hhbbc/check.cpp
@@ -153,7 +153,7 @@ bool check(const php::Func& f) {
   if (f.isClosureBody) {
     assertx(f.cls);
     assertx(f.cls->parentName);
-    assertx(f.cls->parentName->isame(s_Closure.get()));
+    assertx(f.cls->parentName->tsame(s_Closure.get()));
   }
 
   assertx(checkExnTree(f));

diff --git a/hphp/hhbbc/class-util.cpp b/hphp/hhbbc/class-util.cpp
@@ -50,7 +50,7 @@ const StaticString
 bool has_magic_bool_conversion(SString clsName) {
   return
     collections::isTypeName(clsName) ||
-    clsName->isame(s_SimpleXMLElement.get());
+    clsName->tsame(s_SimpleXMLElement.get());
 }
 
 bool is_collection(res::Class cls) {
@@ -90,15 +90,15 @@ bool is_noflatten_trait(const php::Class* cls) {
 }
 
 bool is_closure_base(SString name) {
-  return name->isame(s_Closure.get());
+  return name->tsame(s_Closure.get());
 }
 
 bool is_closure_base(const php::Class& c) {
-  return c.name->isame(s_Closure.get());
+  return c.name->tsame(s_Closure.get());
 }
 
 bool is_closure(const php::Class& c) {
-  return c.parentName && c.parentName->isame(s_Closure.get());
+  return c.parentName && c.parentName->tsame(s_Closure.get());
 }
 
 bool is_closure_name(SString name) {

diff --git a/hphp/hhbbc/debug.cpp b/hphp/hhbbc/debug.cpp
@@ -247,7 +247,7 @@ void dump_index(const std::string& dir,
     std::sort(
       begin(classes), end(classes),
       [] (const php::Class* a, const php::Class* b) {
-        return string_data_lti{}(a->name, b->name);
+        return string_data_lt_type{}(a->name, b->name);
       }
     );
 

diff --git a/hphp/hhbbc/emit.cpp b/hphp/hhbbc/emit.cpp
@@ -507,7 +507,7 @@ EmitBcInfo emit_bytecode(EmitUnitState& euState, UnitEmitter& ue, FuncEmitter& f
       OpInfo<bc::opcode> data{inst.opcode};                      \
       if (RuntimeOption::EnableIntrinsicsExtension) {            \
         if (Op::opcode == Op::FCallFuncD &&                      \
-            inst.FCallFuncD.str2->isame(                         \
+            inst.FCallFuncD.str2->fsame(                         \
               s_hhbbc_fail_verification.get())) {                \
           fe.emitOp(Op::CheckProp);                              \
           fe.emitInt32(                                          \
@@ -1097,7 +1097,7 @@ void emit_class(EmitUnitState& state, UnitEmitter& ue, PreClassEmitter* pce,
       continue;
     }
     if (cconst.kind == ConstModifiers::Kind::Context) {
-      assertx(cconst.cls->isame(cls.name));
+      assertx(cconst.cls->tsame(cls.name));
       assertx(!cconst.resolvedTypeStructure);
       assertx(cconst.invariance == php::Const::Invariance::None);
       pce->addContextConstant(
@@ -1107,7 +1107,7 @@ void emit_class(EmitUnitState& state, UnitEmitter& ue, PreClassEmitter* pce,
         cconst.isFromTrait
       );
     } else if (!cconst.val.has_value()) {
-      assertx(cconst.cls->isame(cls.name));
+      assertx(cconst.cls->tsame(cls.name));
       assertx(!cconst.resolvedTypeStructure);
       assertx(cconst.invariance == php::Const::Invariance::None);
       pce->addAbstractConstant(
@@ -1119,7 +1119,7 @@ void emit_class(EmitUnitState& state, UnitEmitter& ue, PreClassEmitter* pce,
       needs86cinit |= cconst.val->m_type == KindOfUninit;
       pce->addConstant(
         cconst.name,
-        cconst.cls->isame(cls.name) ? nullptr : cconst.cls,
+        cconst.cls->tsame(cls.name) ? nullptr : cconst.cls,
         &cconst.val.value(),
         ArrNR{cconst.resolvedTypeStructure},
         cconst.kind,