Fix bug with synthesized typeconst inheritance

Summary: This is a follow up from D37656012 (e555f82). For classes (not traits), we say that a synthesized member does not replace a synthesized member when folding a class, even when the trait carries a more specific type. Otherwise, we have a bug where `Parentt` in the example below thinks that T is synthesized, so Child does not report an inheritance conflict from its trait.

Reviewed By: francesco-zappa-nardelli

Differential Revision: D39158782

fbshipit-source-id: 8aead0388d6552c02776db3263f8c8795f5f0c12

Author: vassilmladenov

hphp/hack/src/compare_folded_decls_file.ml

@@ -15,7 +15,8 @@ let popt
     ~disable_xhp_element_mangling
     ~disable_enum_classes
     ~interpret_soft_types_as_like_types
-    ~everything_sdt =
+    ~everything_sdt
+    ~enable_strict_const_semantics =
   let enable_enum_classes = not disable_enum_classes in
   let po = ParserOptions.default in
   let po =
@@ -34,6 +35,13 @@ let popt
       interpret_soft_types_as_like_types
   in
   let po = ParserOptions.with_everything_sdt po everything_sdt in
+  let po =
+    {
+      po with
+      GlobalOptions.tco_enable_strict_const_semantics =
+        enable_strict_const_semantics;
+    }
+  in
   po
 
 let init ~enable_strict_const_semantics popt : Provider_context.t =
@@ -260,6 +268,7 @@ let () =
         ~disable_enum_classes
         ~interpret_soft_types_as_like_types
         ~everything_sdt
+        ~enable_strict_const_semantics
     in
     let tco_experimental_features =
       TypecheckerOptions.experimental_from_flags

hphp/hack/src/decl/decl_inherit.ml

@@ -197,7 +197,10 @@ let add_const name const acc =
 
 let add_members members acc = SMap.fold SMap.add members acc
 
-let add_typeconst ~strict_const_semantics name sig_ typeconsts =
+let add_typeconst env c name sig_ typeconsts =
+  let fix_synthesized =
+    TypecheckerOptions.enable_strict_const_semantics (Decl_env.tcopt env) > 3
+  in
   match SMap.find_opt name typeconsts with
   | None ->
     (* The type constant didn't exist so far, let's add it *)
@@ -221,7 +224,8 @@ let add_typeconst ~strict_const_semantics name sig_ typeconsts =
          old_sig.ttc_kind,
          sig_.ttc_kind )
      with
-    | (false, true, _, _) when strict_const_semantics ->
+    | (false, true, _, _) when Ast_defs.is_c_class c.sc_kind && fix_synthesized
+      ->
       (* Don't replace a type constant with a synthesized type constant. This
          covers the following case:
 
@@ -296,7 +300,7 @@ let add_constructor (cstr, cstr_consist) (acc, acc_consist) =
   in
   (ce, Decl_utils.coalesce_consistent acc_consist cstr_consist)
 
-let add_inherited _env inherited acc =
+let add_inherited env c inherited acc =
   {
     ih_substs =
       SMap.merge
@@ -331,18 +335,7 @@ let add_inherited _env inherited acc =
     ih_cstr = add_constructor inherited.ih_cstr acc.ih_cstr;
     ih_consts = SMap.fold add_const inherited.ih_consts acc.ih_consts;
     ih_typeconsts =
-      begin
-        (* TODO(T125402906) Re-enable this after fixing inheritance *)
-        let strict_const_semantics =
-          false
-          (* TypecheckerOptions.enable_strict_const_semantics (Decl_env.tcopt env)
-             > 2 *)
-        in
-        SMap.fold
-          (add_typeconst ~strict_const_semantics)
-          inherited.ih_typeconsts
-          acc.ih_typeconsts
-      end;
+      SMap.fold (add_typeconst env c) inherited.ih_typeconsts acc.ih_typeconsts;
     ih_props = add_members inherited.ih_props acc.ih_props;
     ih_sprops = add_members inherited.ih_sprops acc.ih_sprops;
     ih_methods = add_methods inherited.ih_methods acc.ih_methods;
@@ -617,25 +610,26 @@ let parents_which_provide_members c =
 
 let from_parent env c (parents : Decl_store.class_entries SMap.t) acc parent =
   let inherited = from_class env c parents parent in
-  add_inherited env inherited acc
+  add_inherited env c inherited acc
 
 let from_requirements env c parents acc reqs =
   let inherited = from_class env c parents reqs in
   let inherited = mark_as_synthesized inherited in
-  add_inherited env inherited acc
+  add_inherited env c inherited acc
 
 let from_trait env c parents acc uses =
   let inherited = from_class env c parents uses in
-  add_inherited env inherited acc
+  add_inherited env c inherited acc
 
-let from_xhp_attr_use env (parents : Decl_store.class_entries SMap.t) acc uses =
+let from_xhp_attr_use env c (parents : Decl_store.class_entries SMap.t) acc uses
+    =
   let inherited = from_class_xhp_attrs_only env parents uses in
-  add_inherited env inherited acc
+  add_inherited env c inherited acc
 
 let from_interface_constants
-    env (parents : Decl_store.class_entries SMap.t) acc impls =
+    env c (parents : Decl_store.class_entries SMap.t) acc impls =
   let inherited = from_class_constants_only env parents impls in
-  add_inherited env inherited acc
+  add_inherited env c inherited acc
 
 let has_highest_precedence : (OverridePrecedence.t * 'a) option -> bool =
   function
@@ -737,7 +731,7 @@ let make env c ~cache:(parents : Decl_store.class_entries SMap.t) =
   in
   let acc =
     List.fold_left
-      ~f:(from_xhp_attr_use env parents)
+      ~f:(from_xhp_attr_use env c parents)
       ~init:acc
       c.sc_xhp_attr_uses
   in
@@ -748,7 +742,7 @@ let make env c ~cache:(parents : Decl_store.class_entries SMap.t) =
    *)
   let acc =
     List.fold_left
-      ~f:(from_interface_constants env parents)
+      ~f:(from_interface_constants env c parents)
       ~init:acc
       c.sc_req_implements
   in
@@ -759,12 +753,12 @@ let make env c ~cache:(parents : Decl_store.class_entries SMap.t) =
   in
   let acc =
     List.fold_left
-      ~f:(from_interface_constants env parents)
+      ~f:(from_interface_constants env c parents)
       ~init:acc
       included_enums
   in
   List.fold_left
-    ~f:(from_interface_constants env parents)
+    ~f:(from_interface_constants env c parents)
     ~init:acc
     c.sc_implements
 

hphp/hack/src/hackrs/decl_parser/decl_parser.rs

@@ -23,7 +23,7 @@ use ty::reason::Reason;
 #[derive(Debug, Clone)]
 pub struct DeclParser<R: Reason> {
     file_provider: Arc<dyn FileProvider>,
-    opts: ParserOptions,
+    pub opts: ParserOptions,
     // We could make our parse methods generic over `R` instead, but it's
     // usually more convenient for callers (especially tests) to pin the decl
     // parser to a single Reason type.

hphp/hack/src/hackrs/folded_decl_provider/inherit.rs

@@ -220,9 +220,10 @@ impl<R: Reason> Inherited<R> {
     fn add_type_consts(
         &mut self,
         opts: &GlobalOptions,
+        child: &ShallowClass<R>,
         other_type_consts: TypeConstNameIndexMap<TypeConst<R>>,
     ) {
-        let strict_const_semantics = opts.tco_enable_strict_const_semantics > 2;
+        let fix_synthesized = opts.tco_enable_strict_const_semantics > 3;
 
         for (name, mut new_const) in other_type_consts {
             match self.type_consts.entry(name) {
@@ -239,15 +240,20 @@ impl<R: Reason> Inherited<R> {
                         e.get_mut().enforceable = new_const.enforceable.clone();
                     }
                     let old_const = e.get();
+                    let is_class = || match child.kind {
+                        ClassishKind::Cclass(_) => true,
+                        ClassishKind::Ctrait
+                        | ClassishKind::Cinterface
+                        | ClassishKind::Cenum
+                        | ClassishKind::CenumClass(_) => false,
+                    };
                     match (
                         old_const.is_synthesized,
                         new_const.is_synthesized,
                         &old_const.kind,
                         &new_const.kind,
                     ) {
-                        // TODO(T125402906) Re-enable this after fixing inheritance
-                        // (false, true, _, _) => {}
-                        (false, true, _, _) if strict_const_semantics => {}
+                        (false, true, _, _) if is_class() && fix_synthesized => {}
                         // This covers the following case
                         // ```
                         // interface I1 { abstract const type T; }
@@ -308,7 +314,7 @@ impl<R: Reason> Inherited<R> {
         }
     }
 
-    fn add_inherited(&mut self, opts: &GlobalOptions, other: Self) {
+    fn add_inherited(&mut self, opts: &GlobalOptions, child: &ShallowClass<R>, other: Self) {
         let Self {
             substs,
             props,
@@ -326,7 +332,7 @@ impl<R: Reason> Inherited<R> {
         self.add_static_methods(static_methods);
         self.add_constructor(constructor);
         self.add_consts(consts);
-        self.add_type_consts(opts, type_consts);
+        self.add_type_consts(opts, child, type_consts);
     }
 
     fn mark_as_synthesized(&mut self) {
@@ -506,7 +512,7 @@ impl<'a, R: Reason> MemberFolder<'a, R> {
     fn add_from_interface_constants(&mut self) -> Result<()> {
         for ty in self.child.req_implements.iter() {
             self.members
-                .add_inherited(self.opts, self.class_constants_from_class(ty)?)
+                .add_inherited(self.opts, self.child, self.class_constants_from_class(ty)?)
         }
 
         Ok(())
@@ -515,7 +521,7 @@ impl<'a, R: Reason> MemberFolder<'a, R> {
     fn add_from_implements_constants(&mut self) -> Result<()> {
         for ty in self.child.implements.iter() {
             self.members
-                .add_inherited(self.opts, self.class_constants_from_class(ty)?)
+                .add_inherited(self.opts, self.child, self.class_constants_from_class(ty)?)
         }
 
         Ok(())
@@ -524,7 +530,7 @@ impl<'a, R: Reason> MemberFolder<'a, R> {
     fn add_from_xhp_attr_uses(&mut self) -> Result<()> {
         for ty in self.child.xhp_attr_uses.iter() {
             self.members
-                .add_inherited(self.opts, self.xhp_attrs_from_class(ty)?)
+                .add_inherited(self.opts, self.child, self.xhp_attrs_from_class(ty)?)
         }
 
         Ok(())
@@ -554,7 +560,7 @@ impl<'a, R: Reason> MemberFolder<'a, R> {
         // be implemented.
         for ty in tys.iter().rev() {
             self.members
-                .add_inherited(self.opts, self.members_from_class(ty)?);
+                .add_inherited(self.opts, self.child, self.members_from_class(ty)?);
         }
 
         Ok(())
@@ -564,7 +570,7 @@ impl<'a, R: Reason> MemberFolder<'a, R> {
         for ty in self.child.req_extends.iter() {
             let mut inherited = self.members_from_class(ty)?;
             inherited.mark_as_synthesized();
-            self.members.add_inherited(self.opts, inherited);
+            self.members.add_inherited(self.opts, self.child, inherited);
         }
 
         Ok(())
@@ -573,7 +579,7 @@ impl<'a, R: Reason> MemberFolder<'a, R> {
     fn add_from_traits(&mut self) -> Result<()> {
         for ty in self.child.uses.iter() {
             self.members
-                .add_inherited(self.opts, self.members_from_class(ty)?);
+                .add_inherited(self.opts, self.child, self.members_from_class(ty)?);
         }
 
         Ok(())
@@ -582,8 +588,11 @@ impl<'a, R: Reason> MemberFolder<'a, R> {
     fn add_from_included_enums_constants(&mut self) -> Result<()> {
         if let Some(et) = self.child.enum_type.as_ref() {
             for ty in et.includes.iter() {
-                self.members
-                    .add_inherited(self.opts, self.class_constants_from_class(ty)?);
+                self.members.add_inherited(
+                    self.opts,
+                    self.child,
+                    self.class_constants_from_class(ty)?,
+                );
             }
         }
 

hphp/hack/src/hackrs/hackrs_test_utils/decl_provider.rs

@@ -27,6 +27,7 @@ pub fn make_folded_decl_provider<R: Reason>(
     shallow_decl_store: ShallowDeclStore<R>,
     decl_parser: DeclParser<R>,
 ) -> impl FoldedDeclProvider<R> {
+    let opts = Arc::new(decl_parser.opts.clone());
     let shallow_decl_provider: Arc<dyn ShallowDeclProvider<R>> =
         if let Some(naming_table_path) = naming_table {
             Arc::new(LazyShallowDeclProvider::new(
@@ -39,7 +40,7 @@ pub fn make_folded_decl_provider<R: Reason>(
         };
 
     LazyFoldedDeclProvider::new(
-        Arc::new(oxidized::global_options::GlobalOptions::default()),
+        opts,
         match store_opts {
             StoreOpts::Serialized(compression_type) => {
                 Arc::new(SerializingStore::with_compression(compression_type))

hphp/hack/src/providers/hackrs_provider_backend/hackrs_provider_backend.rs

@@ -95,7 +95,7 @@ impl HhServerProviderBackend {
         ))));
 
         let folded_decl_provider = Arc::new(LazyFoldedDeclProvider::new(
-            Arc::new(Default::default()), // TODO: remove?
+            Arc::new(parser_options.clone()),
             Arc::clone(&folded_classes_store) as _,
             Arc::clone(&lazy_shallow_decl_provider) as _,
             dependency_graph,

hphp/hack/test/typecheck/stricter_consts/HH_FLAGS

@@ -1 +1 @@
---enable-strict-const-semantics 2
+--enable-strict-const-semantics 4

hphp/hack/test/typecheck/stricter_consts/synthesized_const_bug.php

@@ -0,0 +1,31 @@
+<?hh
+// (c) Meta Platforms, Inc. and affiliates.
+
+interface ISource {
+  const type T = int;
+}
+
+trait TSource {
+  require implements ISource;
+}
+
+abstract class Grandparent implements ISource {
+  use TSource;
+}
+
+trait TGrandparent {
+  require extends Grandparent;
+}
+
+abstract class Parentt extends Grandparent {
+  use TGrandparent;
+}
+
+trait TParent {
+  require extends Parentt;
+  const type T = int;
+}
+
+final class Child extends Parentt {
+  use TParent;
+}

hphp/hack/test/typecheck/stricter_consts/synthesized_const_bug.php.exp

@@ -0,0 +1,6 @@
+File "synthesized_const_bug.php", line 29, characters 13-17:
+Constant T is defined concretely in multiple ancestors (Typing[4374])
+  File "synthesized_const_bug.php", line 5, characters 14-14:
+  One conflicting definition is here, inherited through Parentt
+  File "synthesized_const_bug.php", line 26, characters 14-14:
+  Another conflicting definition is here