Simplify treatment of parent::__construct and fix issues with like types

Summary:
The logic for checking `parent::__construct` calls is shared with that for checking `new`. It's broken in at least a couple of ways:
* There is no check that a constructor exists in the parent (T38050584)
* It doesn't properly support like-type calling i.e. treating the constructor as "supporting dynamic"
Also, the logic is kind of nonsense: for a base class that has generics e.g. `Base<TB>` we generate fresh type variables for the generics, do all the argument checking, and then after the fact check that `Base<#1> = Base<TB>`.

It's much simpler instead to treat `parent::__construct` just like a normal call. We don't need the nonsense of fresh type variables, e.g. if we have `Derived<T> extends Base<vec<T>>` then we're simply checking parameter types that involve `T` in scope. But in order to enable like-type calling, we must make sure that `supportdyn` is wrapped around the function type of the constructor, just as with methods.

Note that we already check constructors under both static and dynamic assumptions, so this "use-site" `supportdyn` treatment now fits with the "definition-site" checking of the constructor.

Differential Revision: D87070529

fbshipit-source-id: d37417f699d57fcacf27de9d37cc1d071a0a24ab

Author: Andrew Kennedy

hphp/hack/src/decl/decl_folded_class.ml

@@ -390,7 +390,7 @@ let build_constructor
           ~superfluous_override:false
           ~dynamicallycallable:false
           ~readonly_prop_or_needs_concrete:false
-          ~support_dynamic_type:false
+          ~support_dynamic_type:(sm_support_dynamic_type method_)
           ~needs_init:false
           ~no_auto_likes:(sm_no_auto_likes method_)
           ~safe_global_variable:false;

hphp/hack/src/decl/direct_decl_smart_constructors.rs

@@ -2490,6 +2490,35 @@ impl<'o, 't> DirectDeclSmartConstructors<'o, 't> {
         )
     }
 
+    fn possibly_make_supportdyn_method(
+        &mut self,
+        class_attributes: &Attributes,
+        method: ShallowMethod,
+    ) -> ShallowMethod {
+        if ((self.implicit_sdt() && !class_attributes.no_support_dynamic_type)
+            || class_attributes.support_dynamic_type
+            || class_attributes.dynamically_referenced)
+            && !method.flags.contains(MethodFlags::SUPPORT_DYNAMIC_TYPE)
+        {
+            let type_ = match *method.type_.1 {
+                Ty_::Tfun(ft) => {
+                    let flags = ft.flags | FunTypeFlags::SUPPORT_DYNAMIC_TYPE;
+                    let ft = FunType { flags, ..ft };
+                    Ty(method.type_.0, Box::new(Ty_::Tfun(ft)))
+                }
+                _ => method.type_,
+            };
+            let flags = method.flags | MethodFlags::SUPPORT_DYNAMIC_TYPE;
+            ShallowMethod {
+                type_,
+                flags,
+                ..method
+            }
+        } else {
+            method
+        }
+    }
+
     // For a polymorphic context with form `ctx $f` (represented here as
     // `Tapply "$f"`), add a type parameter named `Tctx$f`, and rewrite the
     // parameter `(function (ts)[_]: t) $f` as `(function (ts)[Tctx$f]: t) $f`
@@ -4569,6 +4598,10 @@ impl<'o, 't> FlattenSmartConstructors for DirectDeclSmartConstructors<'o, 't> {
                     }
                 }
                 Node::Constructor(box ConstructorNode { method, properties }) => {
+                    // Annoyingly, the <<__SupportDynamicType>> annotation on a
+                    // class implicitly changes the decls of every constructor inside
+                    // it, so we have to reallocate them here.
+                    let method = self.possibly_make_supportdyn_method(&class_attributes, method);
                     constructor = Some(method);
                     for property in properties {
                         props.push(property)
@@ -4578,29 +4611,7 @@ impl<'o, 't> FlattenSmartConstructors for DirectDeclSmartConstructors<'o, 't> {
                     // Annoyingly, the <<__SupportDynamicType>> annotation on a
                     // class implicitly changes the decls of every method inside
                     // it, so we have to reallocate them here.
-                    let method = if ((self.implicit_sdt()
-                        && !class_attributes.no_support_dynamic_type)
-                        || class_attributes.support_dynamic_type
-                        || class_attributes.dynamically_referenced)
-                        && !method.flags.contains(MethodFlags::SUPPORT_DYNAMIC_TYPE)
-                    {
-                        let type_ = match *method.type_.1 {
-                            Ty_::Tfun(ft) => {
-                                let flags = ft.flags | FunTypeFlags::SUPPORT_DYNAMIC_TYPE;
-                                let ft = FunType { flags, ..ft };
-                                Ty(method.type_.0, Box::new(Ty_::Tfun(ft)))
-                            }
-                            _ => method.type_,
-                        };
-                        let flags = method.flags | MethodFlags::SUPPORT_DYNAMIC_TYPE;
-                        ShallowMethod {
-                            type_,
-                            flags,
-                            ..method
-                        }
-                    } else {
-                        method
-                    };
+                    let method = self.possibly_make_supportdyn_method(&class_attributes, method);
                     if is_static {
                         static_methods.push(method);
                     } else {
@@ -4950,7 +4961,7 @@ impl<'o, 't> FlattenSmartConstructors for DirectDeclSmartConstructors<'o, 't> {
         flags.set(MethodFlags::PHP_STD_LIB, attributes.php_std_lib);
         flags.set(
             MethodFlags::SUPPORT_DYNAMIC_TYPE,
-            !is_constructor && attributes.support_dynamic_type,
+            attributes.support_dynamic_type,
         );
         flags.set(MethodFlags::NO_AUTO_LIKES, attributes.no_auto_likes);
         flags.set(MethodFlags::NEEDS_CONCRETE, attributes.needs_concrete);

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

@@ -472,7 +472,7 @@ impl<'a, R: Reason> DeclFolder<'a, R> {
                 is_lateinit: false,
                 is_dynamicallycallable: false,
                 is_readonly_prop_or_needs_concrete: false,
-                supports_dynamic_type: false,
+                supports_dynamic_type: meth_flags.supports_dynamic_type(),
                 needs_init: false,
                 safe_global_variable: false,
                 no_auto_likes: false,

hphp/hack/src/typing/typing.ml

@@ -2600,7 +2600,6 @@ module rec Expr : sig
       as in `<<MyAttr()>> class C` (note that attributes are classes) *)
   val new_object :
     expected:ExpectedTy.t option ->
-    check_parent:bool ->
     check_not_abstract:bool ->
     is_using_clause:bool ->
     is_attribute:bool ->
@@ -4602,7 +4601,6 @@ end = struct
         new_object
           ~expected
           ~is_using_clause:ctxt.Context.is_using_clause
-          ~check_parent:false
           ~check_not_abstract:true
           ~is_attribute:false
           pos
@@ -5240,7 +5238,6 @@ end = struct
 
   and new_object
       ~(expected : ExpectedTy.t option)
-      ~check_parent
       ~check_not_abstract
       ~is_using_clause
       ~is_attribute
@@ -5320,8 +5317,7 @@ end = struct
           end
       in
       if
-        (not check_parent)
-        && (not is_using_clause)
+        (not is_using_clause)
         && Typing_disposable.is_disposable_class env class_info
       then
         Typing_error_utils.add_typing_error
@@ -5357,8 +5353,6 @@ end = struct
         let (env, cid_ty) = Env.expand_type env cid_ty in
         if is_generic cid_ty then
           (env, cid_ty)
-        else if check_parent then
-          (env, c_ty)
         else
           ExprDepTy.make env ~cid:cid_ c_ty
       in
@@ -5494,8 +5488,6 @@ end = struct
       in
       if is_generic stripped_cid_ty then
         (env, cid_ty)
-      else if check_parent then
-        (env, ty)
       else
         ExprDepTy.make env ~cid:cid_ ty
     in
@@ -5593,85 +5585,23 @@ end = struct
 
     (env, (te, ty))
 
-  and check_parent_construct pos recv_pos env el unpacked_element env_parent =
-    let check_not_abstract = false in
-    let ((env, ty_err_opt), env_parent) =
-      Phase.localize_no_subst env ~ignore_errors:true env_parent
-    in
-    Option.iter ~f:(Typing_error_utils.add_typing_error ~env) ty_err_opt;
-    let ( env,
-          _tcid,
-          _tal,
-          tel,
-          typed_unpack_element,
-          parent,
-          fty,
-          should_forget_fakes ) =
-      new_object
-        ~expected:None
-        ~check_parent:true
-        ~check_not_abstract
-        ~is_using_clause:false
-        ~is_attribute:false
-        pos
-        env
-        ((), recv_pos, CIparent)
-        []
-        el
-        unpacked_element
-    in
-    (* Not sure why we need to equate these types *)
-    let (env, e1) =
-      Type.sub_type
-        pos
-        Reason.URnone
-        env
-        env_parent
-        parent
-        Typing_error.Callback.unify_error
-    in
-    let (env, e2) =
-      Type.sub_type
-        pos
-        Reason.URnone
-        env
-        parent
-        env_parent
-        Typing_error.Callback.unify_error
-    in
-    Option.(
-      iter ~f:(Typing_error_utils.add_typing_error ~env)
-      @@ merge e1 e2 ~f:Typing_error.both);
-    ( env,
-      tel,
-      typed_unpack_element,
-      MakeType.void (Reason.witness pos),
-      parent,
-      fty,
-      should_forget_fakes )
-
-  and call_parent_construct pos recv_pos env el unpacked_element =
+  (* Check that the parent exists in a parent::__construct call *)
+  and check_parent_construct_receiver pos env =
     match Env.get_parent_ty env with
-    | Some parent ->
-      check_parent_construct pos recv_pos env el unpacked_element parent
+    (* Direct parent, so ok *)
+    | Some _parent -> ()
     | None ->
-      (* continue here *)
-      let (env, ty) = Env.fresh_type_error env pos in
-      let should_invalidate_fake_members = true in
-      let default =
-        (env, [], None, ty, ty, ty, should_invalidate_fake_members)
-      in
       (match Env.get_self_id env with
       | Some self ->
         let open TraitMostConcreteParent in
         (match Env.get_class env self with
+        (* Self is a trait, so look at trait's parents *)
         | Decl_entry.Found trait when Ast_defs.is_c_trait (Cls.kind trait) ->
           (match trait_most_concrete_parent trait env with
           | None ->
             Typing_error_utils.add_typing_error
               ~env
-              Typing_error.(primary @@ Primary.Parent_in_trait pos);
-            default
+              Typing_error.(primary @@ Primary.Parent_in_trait pos)
           | Some NotFound ->
             let trait_reqs =
               Some
@@ -5683,9 +5613,8 @@ end = struct
             Typing_error_utils.add_typing_error
               ~env
               Typing_error.(
-                primary @@ Primary.Parent_undefined { pos; trait_reqs });
-            default
-          | Some (Found (c, parent_ty)) ->
+                primary @@ Primary.Parent_undefined { pos; trait_reqs })
+          | Some (Found (c, _parent_ty)) ->
             (match Typing_env.get_construct env c with
             | (_, Inconsistent) ->
               Typing_error_utils.add_typing_error
@@ -5694,29 +5623,13 @@ end = struct
                   primary
                   @@ Primary.Trait_parent_construct_inconsistent
                        { pos; decl_pos = Cls.pos c })
-            | _ -> ());
-            check_parent_construct
-              pos
-              recv_pos
-              env
-              el
-              unpacked_element
-              parent_ty)
-        | Decl_entry.Found _self_tc ->
-          Typing_error_utils.add_typing_error
-            ~env
-            Typing_error.(
-              primary @@ Primary.Parent_undefined { pos; trait_reqs = None });
-          default
-        | Decl_entry.NotYetAvailable
-        | Decl_entry.DoesNotExist ->
-          assert false)
+            | _ -> ()))
+        | _ -> ())
       | None ->
+        (* We're not even inside a class *)
         Typing_error_utils.add_typing_error
           ~env
-          Typing_error.(primary @@ Primary.Parent_outside_class pos);
-        let (env, ty) = Env.fresh_type_error env pos in
-        (env, [], None, ty, ty, ty, should_invalidate_fake_members))
+          Typing_error.(primary @@ Primary.Parent_outside_class pos))
 
   (* Depending on the kind of expression we are dealing with
    * The typing of call is different.
@@ -6257,37 +6170,16 @@ end = struct
             | _ -> (env, res))
       | _ -> dispatch_class_const env class_id method_id
     end
-    (* Special function `parent::__construct` *)
-    | Class_const ((_, pos, CIparent), ((_, construct) as id))
-      when String.equal construct SN.Members.__construct ->
-      let ( env,
-            tel,
-            typed_unpack_element,
-            ty,
-            pty,
-            ctor_fty,
-            should_forget_fakes ) =
-        call_parent_construct p pos env el unpacked_element
-      in
-      let (env, te) =
-        Typing_helpers.make_simplify_typed_expr
-          env
-          fpos
-          ctor_fty
-          (Aast.Class_const ((pty, pos, Aast.CIparent), id))
-      in
-      let result =
-        make_call
-          env
-          te
-          [] (* tal: no type arguments to constructor *)
-          tel
-          typed_unpack_element
-          ty
-      in
-      (result, should_forget_fakes)
     (* Calling parent / class method *)
-    | Class_const (class_id, m) -> dispatch_class_const env class_id m
+    | Class_const (class_id, m) ->
+      let () =
+        match (class_id, m) with
+        | ((_, pos, CIparent), (_, construct))
+          when String.equal construct SN.Members.__construct ->
+          check_parent_construct_receiver pos env
+        | _ -> ()
+      in
+      dispatch_class_const env class_id m
     (* Readonly Expressions do not affect the type, but need to be threaded through when they're part of a call *)
     | ReadonlyExpr r ->
       let env = Env.set_readonly env true in
@@ -11272,7 +11164,6 @@ end = struct
       let (env, _, _, _, _, _, _, _) =
         Expr.new_object
           ~expected:None
-          ~check_parent:false
           ~check_not_abstract:false
           ~is_using_clause:false
           ~is_attribute:true

hphp/hack/src/typing/typing_object_get.ml

@@ -112,7 +112,10 @@ let fold_mismatch_opts opt_errs =
 
 let get_member ~is_method env class_info member_name =
   if is_method then
-    Env.get_method env class_info member_name
+    if String.equal member_name SN.Members.__construct then
+      fst (Env.get_construct env class_info)
+    else
+      Env.get_method env class_info member_name
   else
     Env.get_prop env class_info member_name
 
@@ -901,17 +904,6 @@ and obj_get_concrete_class_without_member_info
       (mk (Reason.none, Tfun ft), []),
       dflt_lval_mismatch,
       dflt_rval_mismatch )
-  else if String.equal id_str SN.Members.__construct then
-    (* __construct is not an instance method and shouldn't be invoked directly
-       Note that we already raise a NAST check error in `illegal_name_check` but
-       we raise a related typing error here to properly keep track of failure.
-       We prefer a specific error here since the generic 4053 `MemberNotFound`
-       error, below, would be quite confusing telling us there is no instance
-       method `__construct` *)
-    let ty_err =
-      Typing_error.(primary @@ Primary.Construct_not_instance_method id_pos)
-    in
-    default (Some ty_err)
   else
     let ty_err =
       member_not_found