Stricter enforcement of arity for unpacked arguments

Summary: This diff adds a typechecker option to be stricter with argument unpacking. Consider the following function: ``` function max<T as num>( T $first, T $second, T ...$rest, ): T; ``` The typechecker currently allows us to call the function like this: ``` $max = max(...$my_numbers); ``` If that particular Container has no elements, this will be a runtime exception. This diff requires that, when the option is set, that users pass in at least two arguments, and unpack the rest, or use a function that supports Containers: ``` $max = max($first, $second, ...$my_numbers); // OR: $max = max2($my_numbers); ``` Note that, in case of a tuple, we will allow the arguments to be unpacked because we can guarantee the arity: ``` $my_numbers = tuple(1, 2); $max = max(...$my_numbers); // OK ``` Reviewed By: jamesjwu Differential Revision: D6926957 fbshipit-source-id: 27942e0420d4e5d31c22493e72ea45689c6bd666
facebook · Feb 8, 2018 · 0e4d8c05c4dfc6bd3aee89a6c50e9220f9f7b0e7 · 0e4d8c0
1 parent f918d01
commit 0e4d8c05c4dfc6bd3aee89a6c50e9220f9f7b0e7
diff --git a/hphp/hack/src/options/globalOptions.ml b/hphp/hack/src/options/globalOptions.ml
@@ -135,6 +135,20 @@ let tco_experimental_no_fallback_in_namespaces =
 *)
 let tco_experimental_nonnull = "nonnull"

+(*
+ * Check that the minimum number of arguments to a variadic
+ * function are passed in, e.g. given the function:
+ *
+ * function max<T as num>(T $first, T $second, T ...$rest): T;
+ *
+ * The following calls will be errors if this option is enabled:
+ *
+ * $max = max(...$my_numbers);
+ * $max = max($first, ...$my_numbers);
+ *)
+let tco_unpacking_check_arity =
+  "unpacking_check_arity"
+
 let tco_experimental_all =
 SSet.empty |> List.fold_right SSet.add
   [
@@ -157,6 +171,7 @@ let tco_experimental_all =
     tco_experimental_is_expression;
     tco_experimental_no_fallback_in_namespaces;
     tco_experimental_nonnull;
+     tco_unpacking_check_arity;
   ]

 let tco_migration_flags_all =
diff --git a/hphp/hack/src/options/globalOptions.mli b/hphp/hack/src/options/globalOptions.mli
@@ -121,6 +121,7 @@ val tco_experimental_no_trait_reuse : string
 val tco_experimental_is_expression : string
 val tco_experimental_no_fallback_in_namespaces : string
 val tco_experimental_nonnull : string
+val tco_unpacking_check_arity : string
 val tco_experimental_all : SSet.t
 val tco_migration_flags_all : SSet.t
 val ignored_fixme_codes : t -> ISet.t
diff --git a/hphp/hack/src/options/typecheckerOptions.ml b/hphp/hack/src/options/typecheckerOptions.ml
@@ -51,6 +51,8 @@ let experimental_is_expression = GlobalOptions.tco_experimental_is_expression
 let experimental_no_fallback_in_namespaces =
  GlobalOptions.tco_experimental_no_fallback_in_namespaces
 let experimental_nonnull = GlobalOptions.tco_experimental_nonnull
+let experimental_unpacking_check_arity =
+  GlobalOptions.tco_unpacking_check_arity

 let experimental_all = GlobalOptions.tco_experimental_all
 let migration_flags_all = GlobalOptions.tco_migration_flags_all
diff --git a/hphp/hack/src/typing/typing.ml b/hphp/hack/src/typing/typing.ml
@@ -4754,12 +4754,13 @@ and call_construct p env class_ params el uel cid =
      let env, tel, tuel, _ty = call ~expected:None p env m el uel in
      env, tcid, tel, tuel

-and check_arity ?(check_min=true) pos pos_def (arity:int) exp_arity =
+and check_arity ?(check_min=true) ?(did_unpack=false) pos pos_def (arity:int) exp_arity =
  let exp_min = (Typing_defs.arity_min exp_arity) in
  if check_min && arity < exp_min then
    Errors.typing_too_few_args pos pos_def;
  match exp_arity with
    | Fstandard (_, exp_max) ->
+      let arity = if did_unpack then arity + 1 else arity in
      if (arity > exp_max)
      then Errors.typing_too_many_args pos pos_def;
    | Fvariadic _ | Fellipsis _ -> ()
@@ -4936,9 +4937,9 @@ and call_ ~expected pos env fty el uel =
      | Some x -> x
      | None -> failwith "missing parameter in check_args" in
    let tel = List.map rl (fun (_, opt) -> fst (get_param opt)) in
-    let env, tuel, arity, check_min =
+    let env, tuel, arity, check_min, did_unpack =
      match uel with
-      | [] -> env, [], List.length el, true
+      | [] -> env, [], List.length el, true, false
      | e :: _ ->
        (* Enforces that e is unpackable. If e is a tuple, check types against
         * parameter types *)
@@ -4956,7 +4957,7 @@ and call_ ~expected pos env fty el uel =
                let env = call_param env param (e, ty) in
                check_elements env tyl paraml in
          let env = check_elements env tyl paraml in
-          env, [te], List.length el + List.length tyl, true
+          env, [te], List.length el + List.length tyl, true, false
        | _ ->
          let param_tyl = List.map paraml (fun param -> param.fp_type) in
          let add_variadic_param_ty param_tyl =
@@ -4970,12 +4971,16 @@ and call_ ~expected pos env fty el uel =
            let traversable_ty = make_unpacked_traversable_ty pos param_ty in
            Type.sub_type pos Reason.URparam env ety traversable_ty)
          in
-          env, [te], List.length el + 1, false in
+          let check_min = (TypecheckerOptions.experimental_feature_enabled
+            (Env.get_options env)
+            TypecheckerOptions.experimental_unpacking_check_arity) in
+          env, [te], List.length el, check_min, true
+    in
    (* If we unpacked an array, we don't check arity exactly. Since each
     * unpacked array consumes 1 or many parameters, it is nonsensical to say
     * that not enough args were passed in (so we don't do the min check).
     *)
-    let () = check_arity ~check_min pos pos_def arity ft.ft_arity in
+    let () = check_arity ~check_min ~did_unpack pos pos_def arity ft.ft_arity in
    (* Variadic params cannot be inout so we can stop early *)
    let env = wfold_left2 inout_write_back env ft.ft_params el in
    Typing_hooks.dispatch_fun_call_hooks
diff --git a/hphp/hack/test/typecheck/argument_unpacking/unpack_call1.php.exp b/hphp/hack/test/typecheck/argument_unpacking/unpack_call1.php.exp
@@ -1 +1,44 @@
-No errors
+File "unpack_call1.php", line 38, characters 3-13:
+Too few arguments (Typing[4104])
+  File "unpack_call1.php", line 3, characters 10-10:
+  Definition is here
+File "unpack_call1.php", line 39, characters 11-26:
+Too few arguments (Typing[4104])
+  File "unpack_call1.php", line 8, characters 19-29:
+  Definition is here
+File "unpack_call1.php", line 40, characters 3-20:
+Too few arguments (Typing[4104])
+  File "unpack_call1.php", line 12, characters 19-19:
+  Definition is here
+File "unpack_call1.php", line 41, characters 11-26:
+Too few arguments (Typing[4104])
+  File "unpack_call1.php", line 16, characters 19-29:
+  Definition is here
+File "unpack_call1.php", line 42, characters 3-20:
+Too few arguments (Typing[4104])
+  File "unpack_call1.php", line 12, characters 19-19:
+  Definition is here
+File "unpack_call1.php", line 51, characters 3-23:
+Too few arguments (Typing[4104])
+  File "unpack_call1.php", line 3, characters 10-10:
+  Definition is here
+File "unpack_call1.php", line 53, characters 3-15:
+Too few arguments (Typing[4104])
+  File "unpack_call1.php", line 3, characters 10-10:
+  Definition is here
+File "unpack_call1.php", line 69, characters 3-13:
+Too few arguments (Typing[4104])
+  File "unpack_call1.php", line 3, characters 10-10:
+  Definition is here
+File "unpack_call1.php", line 75, characters 3-13:
+Too few arguments (Typing[4104])
+  File "unpack_call1.php", line 3, characters 10-10:
+  Definition is here
+File "unpack_call1.php", line 85, characters 3-26:
+Too few arguments (Typing[4104])
+  File "unpack_call1.php", line 3, characters 10-10:
+  Definition is here
+File "unpack_call1.php", line 87, characters 3-15:
+Too few arguments (Typing[4104])
+  File "unpack_call1.php", line 3, characters 10-10:
+  Definition is here
diff --git a/hphp/hack/test/typecheck/argument_unpacking/unpack_call11.php b/hphp/hack/test/typecheck/argument_unpacking/unpack_call11.php
@@ -14,31 +14,38 @@ function test_splat_args(): void {
  $e = vec[new E(), new E()];
  $v = vec[new B(), new E()];

-  expect_A(...$empty);
-  expect_A(...$a);
+  $aa = tuple(new A(), new A());
+  $ab = tuple(new A(), new B());
+  $bb = tuple(new B(), new B());
+  $be = tuple(new B(), new E());
+  $ee = tuple(new E(), new E());
+
+  // OK
  expect_As(...$a);
-  expect_AB(new A(), ...$b);
-  expect_ABBB(new A(), ...$b);
-  expect_ABBB(new A(), new B(), ...$b);
+  expect_AB(...$ab);
  expect_ABs(new A(), ...$b);
  expect_ABs(new A(), new B(), ...$b);
-  expect_ABBBBs(new A(), new B(), ...$b);
-  expect_ACD(new A(), ...$e);
-  expect_ACDs(new A(), ...$e);
+  expect_ABE(new A(), ...$be);
+  expect_ABBBBs(new A(), new B(), new B(), ...$bb);

-  // failing tests
-  expect_AB(new A(), new B(), ...$b);
+  // Error
+  expect_AB(...$aa);
+  expect_ABs(new A(), ...$v);
+  expect_ABs(new A(), new B(), ...$v);
  expect_ABE(new A(), ...$v);
+  expect_ACD(new A(), ...$be);
 }

 function f(Traversable<E> $e): void {
-  expect_ACD(new A(), ...$e);
+  // OK
+  expect_ACDs(new A(), new E(), ...$e);
+
+  // Error
+  expect_ABs(new A(), ...$e);
 }

-function expect_A(A $a): void {}
 function expect_As(A ...$a): void {}
 function expect_AB(A $a, B $b): void {}
-function expect_ABBB(A $a, B $b1, B $b2, B $b3): void {}
 function expect_ABs(A $a, B ...$b): void {}
 function expect_ABBBBs(A $a, B $b1, B $b2, B $b3, B ...$b): void {}
 function expect_ABE(A $a, B $b, E $e): void {}
diff --git a/hphp/hack/test/typecheck/argument_unpacking/unpack_call11.php.exp b/hphp/hack/test/typecheck/argument_unpacking/unpack_call11.php.exp
@@ -1,16 +1,46 @@
-File "unpack_call11.php", line 30, characters 3-36:
-Too many arguments (Typing[4105])
-  File "unpack_call11.php", line 40, characters 10-18:
-  Definition is here
-File "unpack_call11.php", line 31, characters 26-27:
+File "unpack_call11.php", line 32, characters 16-18:
+Invalid argument (Typing[4110])
+  File "unpack_call11.php", line 48, characters 26-26:
+  This is an object of type B
+  File "unpack_call11.php", line 17, characters 24-30:
+  It is incompatible with an object of type A
+File "unpack_call11.php", line 33, characters 26-27:
+Invalid argument (Typing[4110])
+  File "unpack_call11.php", line 49, characters 32-33:
+  This is an object of type B (variadic argument)
+  File "unpack_call11.php", line 15, characters 21-27:
+  It is incompatible with an object of type E
+File "unpack_call11.php", line 34, characters 35-36:
+Invalid argument (Typing[4110])
+  File "unpack_call11.php", line 49, characters 32-33:
+  This is an object of type B (variadic argument)
+  File "unpack_call11.php", line 15, characters 21-27:
+  It is incompatible with an object of type E
+File "unpack_call11.php", line 35, characters 26-27:
 Invalid argument (Typing[4110])
-  File "unpack_call11.php", line 44, characters 33-33:
+  File "unpack_call11.php", line 51, characters 33-33:
  This is an object of type E
  File "unpack_call11.php", line 15, characters 12-18:
  It is incompatible with an object of type B
-File "unpack_call11.php", line 31, characters 26-27:
+File "unpack_call11.php", line 35, characters 26-27:
 Invalid argument (Typing[4110])
-  File "unpack_call11.php", line 44, characters 27-27:
+  File "unpack_call11.php", line 51, characters 27-27:
  This is an object of type B
  File "unpack_call11.php", line 15, characters 21-27:
  It is incompatible with an object of type E
+File "unpack_call11.php", line 35, characters 3-28:
+Too few arguments (Typing[4104])
+  File "unpack_call11.php", line 51, characters 10-19:
+  Definition is here
+File "unpack_call11.php", line 36, characters 26-28:
+Invalid argument (Typing[4110])
+  File "unpack_call11.php", line 52, characters 27-27:
+  This is an object of type C
+  File "unpack_call11.php", line 20, characters 15-21:
+  It is incompatible with an object of type B
+File "unpack_call11.php", line 44, characters 26-27:
+Invalid argument (Typing[4110])
+  File "unpack_call11.php", line 49, characters 32-33:
+  This is an object of type B (variadic argument)
+  File "unpack_call11.php", line 39, characters 24-24:
+  It is incompatible with an object of type E
diff --git a/hphp/hack/test/typecheck/argument_unpacking/unpack_call13_arity.php b/hphp/hack/test/typecheck/argument_unpacking/unpack_call13_arity.php
@@ -0,0 +1,42 @@
+<?hh // strict
+
+function test_splat_arity(): void {
+  $tuple2 = tuple(1, 2);
+
+  // OK
+  expect_const2(...$tuple2);
+  expect_const3(0, ...$tuple2);
+  expect_variadic0(...$tuple2);
+  expect_variadic1(...$tuple2);
+  expect_variadic2(...$tuple2);
+
+  // Errors
+  expect_const1(...$tuple2);
+  expect_const2(0, ...$tuple2);
+  expect_variadic3(...$tuple2);
+
+  $vec2 = vec[1, 2];
+
+  // OK
+  expect_variadic0(...$vec2);
+  expect_variadic1(0, ...$vec2);
+  expect_variadic2(0, 1, ...$vec2);
+  expect_variadic3(0, 1, 2, ...$vec2);
+
+  // Errors
+  expect_const1(...$vec2);
+  expect_const1(0, ...$vec2);
+  expect_const2(...$vec2);
+  expect_const2(0, ...$vec2);
+  expect_const2(0, 1, ...$vec2);
+}
+
+type M = mixed;
+
+function expect_const1(M $a): void {}
+function expect_const2(M $a, M $b): void {}
+function expect_const3(M $a, M $b, M $c): void {}
+function expect_variadic0(M ...$as): void {}
+function expect_variadic1(M $a, M ...$bs): void {}
+function expect_variadic2(M $a, M $b, M ...$cs): void {}
+function expect_variadic3(M $a, M $b, M $c, M...$ds): void {}
diff --git a/hphp/hack/test/typecheck/argument_unpacking/unpack_call13_arity.php.exp b/hphp/hack/test/typecheck/argument_unpacking/unpack_call13_arity.php.exp
@@ -0,0 +1,32 @@
+File "unpack_call13_arity.php", line 14, characters 3-27:
+Too many arguments (Typing[4105])
+  File "unpack_call13_arity.php", line 36, characters 10-22:
+  Definition is here
+File "unpack_call13_arity.php", line 15, characters 3-30:
+Too many arguments (Typing[4105])
+  File "unpack_call13_arity.php", line 37, characters 10-22:
+  Definition is here
+File "unpack_call13_arity.php", line 16, characters 3-30:
+Too few arguments (Typing[4104])
+  File "unpack_call13_arity.php", line 42, characters 10-25:
+  Definition is here
+File "unpack_call13_arity.php", line 27, characters 3-25:
+Too few arguments (Typing[4104])
+  File "unpack_call13_arity.php", line 36, characters 10-22:
+  Definition is here
+File "unpack_call13_arity.php", line 28, characters 3-28:
+Too many arguments (Typing[4105])
+  File "unpack_call13_arity.php", line 36, characters 10-22:
+  Definition is here
+File "unpack_call13_arity.php", line 29, characters 3-25:
+Too few arguments (Typing[4104])
+  File "unpack_call13_arity.php", line 37, characters 10-22:
+  Definition is here
+File "unpack_call13_arity.php", line 30, characters 3-28:
+Too few arguments (Typing[4104])
+  File "unpack_call13_arity.php", line 37, characters 10-22:
+  Definition is here
+File "unpack_call13_arity.php", line 31, characters 3-31:
+Too many arguments (Typing[4105])
+  File "unpack_call13_arity.php", line 37, characters 10-22:
+  Definition is here
diff --git a/hphp/hack/test/typecheck/argument_unpacking/unpack_call13_arity.php.no_format b/hphp/hack/test/typecheck/argument_unpacking/unpack_call13_arity.php.no_format
diff --git a/hphp/hack/test/typecheck/argument_unpacking/unpack_call3.php.exp b/hphp/hack/test/typecheck/argument_unpacking/unpack_call3.php.exp
@@ -4,3 +4,7 @@ Invalid argument (Typing[4110])
  This is an object of type Traversable (it is unpacked with '...')
  File "unpack_call3.php", line 6, characters 11-18:
  It is incompatible with a string
+File "unpack_call3.php", line 7, characters 3-13:
+Too few arguments (Typing[4104])
+  File "unpack_call3.php", line 3, characters 10-10:
+  Definition is here
diff --git a/hphp/hack/test/typecheck/argument_unpacking/unpack_call6.php.exp b/hphp/hack/test/typecheck/argument_unpacking/unpack_call6.php.exp
@@ -4,3 +4,7 @@ Invalid argument (Typing[4110])
  This is an object of type Traversable (it is unpacked with '...')
  File "unpack_call6.php", line 11, characters 11-18:
  It is incompatible with a string
+File "unpack_call6.php", line 12, characters 3-18:
+Too few arguments (Typing[4104])
+  File "unpack_call6.php", line 4, characters 19-29:
+  Definition is here
diff --git a/hphp/hack/test/typecheck/construct/parent_construct1.php.exp b/hphp/hack/test/typecheck/construct/parent_construct1.php.exp
@@ -1 +1,4 @@
-No errors
+File "parent_construct1.php", line 36, characters 9-37:
+Too few arguments (Typing[4104])
+File "parent_construct1.php", line 15, characters 19-29:
+Definition is here