Disallow override of async by non-async

Andrew Kennedy · facebook-github-bot · commit 415b6f84cf03 · 2022-10-28T07:50:57.000-07:00
Summary: There is a consensus on the Hack Pack workchat that it's a mistake for Hack to allow `async` methods to be overridden by non-async methods that return `Awaitable`. This pattern is a problem for Sound Dynamic pessimisation, as it is an example of an override that doesn't preserve enforcement (as an `async` method returning `Awaitable<int>` enforces `int` but a non-async method only enforces `Awaitable`). Let's outlaw this in Hack. A separate diff for www asyncifies the ~220 instances of this pattern in www. Reviewed By: vassilmladenov Differential Revision: D38940712 fbshipit-source-id: 64b0e3d63feeff4a945486307e3ddee15984ef9d
diff --git a/hphp/hack/src/errors/error_codes.ml b/hphp/hack/src/errors/error_codes.ml
@@ -744,6 +744,7 @@ module Typing = struct
     | InvalidMethCallerReadonlyReturn [@value 4464]
     | AbstractMemberInConcreteClass [@value 4465]
     | TraitNotUsed [@value 4466]
+    | OverrideAsync [@value 4467]
   (* Add new Typing codes here! Comment out when deprecating. *)
   [@@deriving enum, show { with_path = false }]
 
diff --git a/hphp/hack/src/errors/typing_error.ml b/hphp/hack/src/errors/typing_error.ml
@@ -6566,6 +6566,10 @@ and Secondary : sig
         pos: Pos_or_decl.t;
         parent_pos: Pos_or_decl.t;
       }
+    | Override_async of {
+        pos: Pos_or_decl.t;
+        parent_pos: Pos_or_decl.t;
+      }
     | Override_lsb of {
         pos: Pos_or_decl.t;
         member_name: string;
@@ -6840,6 +6844,10 @@ end = struct
         pos: Pos_or_decl.t;
         parent_pos: Pos_or_decl.t;
       }
+    | Override_async of {
+        pos: Pos_or_decl.t;
+        parent_pos: Pos_or_decl.t;
+      }
     | Override_lsb of {
         pos: Pos_or_decl.t;
         member_name: string;
@@ -7290,6 +7298,16 @@ end = struct
     in
     (Error_code.OverrideFinal, reasons, [])
 
+  let override_async pos parent_pos =
+    let reasons =
+      lazy
+        [
+          (pos, "You cannot override this method with a non-async method");
+          (parent_pos, "It was declared as async");
+        ]
+    in
+    (Error_code.OverrideAsync, reasons, [])
+
   let override_lsb pos member_name parent_pos =
     let reasons =
       lazy
@@ -7716,6 +7734,8 @@ end = struct
       Eval_result.single (ifc_policy_mismatch pos policy pos_super policy_super)
     | Override_final { pos; parent_pos } ->
       Eval_result.single (override_final pos parent_pos)
+    | Override_async { pos; parent_pos } ->
+      Eval_result.single (override_async pos parent_pos)
     | Override_lsb { pos; member_name; parent_pos } ->
       Eval_result.single (override_lsb pos member_name parent_pos)
     | Multiple_concrete_defs
diff --git a/hphp/hack/src/errors/typing_error.mli b/hphp/hack/src/errors/typing_error.mli
@@ -1566,6 +1566,10 @@ module Secondary : sig
         pos: Pos_or_decl.t;
         parent_pos: Pos_or_decl.t;
       }
+    | Override_async of {
+        pos: Pos_or_decl.t;
+        parent_pos: Pos_or_decl.t;
+      }
     | Override_lsb of {
         pos: Pos_or_decl.t;
         member_name: string;
diff --git a/hphp/hack/src/oxidized/gen/error_codes.rs b/hphp/hack/src/oxidized/gen/error_codes.rs
@@ -3,7 +3,7 @@
 // This source code is licensed under the MIT license found in the
 // LICENSE file in the "hack" directory of this source tree.
 //
-// @generated SignedSource<<cc7fdc5368fda8084df29558e67e5303>>
+// @generated SignedSource<<36e1333531376fab7153d3cdb7e9b09e>>
 //
 // To regenerate this file, run:
 //   hphp/hack/src/oxidized_regen.sh
@@ -577,6 +577,7 @@ pub enum Typing {
     InvalidMethCallerReadonlyReturn = 4464,
     AbstractMemberInConcreteClass = 4465,
     TraitNotUsed = 4466,
+    OverrideAsync = 4467,
 }
 impl TrivialDrop for Typing {}
 arena_deserializer::impl_deserialize_in_arena!(Typing);
diff --git a/hphp/hack/src/typing/typing_extends.ml b/hphp/hack/src/typing/typing_extends.ml
@@ -430,6 +430,14 @@ let check_lateinit parent_class_elt class_elt on_error =
                 parent_is_lateinit = get_ce_lateinit parent_class_elt;
               })
 
+let check_async ft_parent ft_child parent_pos pos on_error =
+  match (get_ft_async ft_parent, get_ft_async ft_child) with
+  | (true, false) ->
+    Errors.add_typing_error
+      Typing_error.(
+        apply_reasons ~on_error @@ Secondary.Override_async { pos; parent_pos })
+  | _ -> ()
+
 let check_xhp_attr_required env parent_class_elt class_elt on_error =
   if not (TypecheckerOptions.check_xhp_attribute (Env.get_tcopt env)) then
     ()
@@ -864,6 +872,12 @@ let check_override
         class_elt.ce_origin
         member_name
         member_kind;
+      check_async
+        ft_parent
+        ft_child
+        (Typing_reason.to_pos r_parent)
+        (Typing_reason.to_pos r_child)
+        on_error;
       check_ambiguous_inheritance
         (check_subtype_methods
            env
diff --git a/hphp/hack/test/typecheck/async_function_untyped5.php.exp b/hphp/hack/test/typecheck/async_function_untyped5.php.exp
@@ -6,3 +6,9 @@ The method `f` is not compatible with the overridden method (Typing[4341])
   Expected `Awaitable<_>` (result of an `async` function)
   File "async_function_untyped5.php", line 8, characters 24-26:
   But got `int`
+File "async_function_untyped5.php", line 8, characters 19-19:
+The method `f` is not compatible with the overridden method (Typing[4341])
+  File "async_function_untyped5.php", line 8, characters 19-19:
+  You cannot override this method with a non-async method
+  File "async_function_untyped5.php", line 4, characters 25-25:
+  It was declared as async
diff --git a/hphp/hack/test/typecheck/async_override_bad.php b/hphp/hack/test/typecheck/async_override_bad.php
@@ -0,0 +1,24 @@
+<?hh
+// (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary.
+
+class C {
+  public async function foo():Awaitable<int> {
+    return 3;
+  }
+  public function boo():Awaitable<int> {
+    return $this->foo();
+  }
+}
+class D extends C {
+  public async function bar():Awaitable<int> {
+    return 4;
+  }
+  // Illegal: do not permit non-async methods to override async ones
+  public function foo():Awaitable<int> {
+    return $this->bar();
+  }
+  // Legal: allow async methods to override non-async ones
+  public async function boo():Awaitable<int> {
+    return 5;
+  }
+}
diff --git a/hphp/hack/test/typecheck/async_override_bad.php.exp b/hphp/hack/test/typecheck/async_override_bad.php.exp
@@ -0,0 +1,6 @@
+File "async_override_bad.php", line 17, characters 19-21:
+The method `foo` is not compatible with the overridden method (Typing[4341])
+  File "async_override_bad.php", line 17, characters 19-21:
+  You cannot override this method with a non-async method
+  File "async_override_bad.php", line 5, characters 25-27:
+  It was declared as async
