Skip to content

Commit 1128e6a

Browse files
Andrew Kennedymeta-codesync[bot]
Andrew Kennedy
authored and
meta-codesync[bot]
committed
Permit optional to precede splat in functions
Summary: No longer reject the use of type splat syntax `...T` following optional parameters (in abstract methods and function hints) or parameters with defaults (in concrete functions and methods). We want to support this because it's a common pattern for (typed) format strings, e.g. `function myinvariant<Targs as (mixed...)>(bool $condition, ?HH\TypedFormatString<SprintfFormat,Targs> = null, ...Targs $args):void`. When the splat is instantiated, an optional parameter may become *effectively* a required parameter, if it is followed by another required parameter, e.g. in this example if we instantiate `Targs` with the tuple type `(string,int)`. Reviewed By: enetsee Differential Revision: D88363220 fbshipit-source-id: cfa9c3652b4b15e236a9dd6b25d88e58a1d14db6
1 parent f9363cb commit 1128e6a

18 files changed

Lines changed: 447 additions & 169 deletions

‎hphp/hack/src/decl/decl_fun_utils.ml‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -164,7 +164,9 @@ let check_params_for_required_after_optional ~from_abstract_method paraml :
164164
| Init when is_optional_or_default -> Seen_optional_or_default
165165
| Init -> Init
166166
| Seen_optional_or_default
167-
when is_optional_or_default || Aast_utils.is_param_variadic param ->
167+
when is_optional_or_default
168+
|| Aast_utils.is_param_variadic param
169+
|| Aast_utils.is_param_splat param ->
168170
Seen_optional_or_default
169171
| Seen_optional_or_default ->
170172
Found_required_after_optional_or_default param

‎hphp/hack/src/elab/passes/validate_function_hint_optional_parameters.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ impl Pass for ValidateFunctionHintOptionalParametersPass {
6060
} else {
6161
match previous_optional_param {
6262
None => (),
63+
Some(_) if p.pre_ellipsis.is_ellipsis() => (),
6364
Some(_) => {
6465
env.emit_error(NamingPhaseError::Parsing(ParsingError::ParsingError {
6566
pos: pos.clone(),

‎hphp/hack/src/naming/naming_validate_function_hint_optional_parameters.ml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ let on_hint on_error hint ~ctx =
1717
~init:(false, false)
1818
~f:(fun (seen_optional, optional_precedes_non_optional) pi ->
1919
match pi with
20+
| Some { Aast.hfparam_splat = Some Ast_defs.Splat; _ } ->
21+
(seen_optional, optional_precedes_non_optional)
2022
| Some { Aast.hfparam_optional = Some Ast_defs.Optional; _ } ->
2123
(true, optional_precedes_non_optional)
2224
| _ ->

‎hphp/hack/src/typing/typing.ml‎

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7342,8 +7342,10 @@ end = struct
73427342
Supposing that $yi have types yi, we just need to assert
73437343
(u1, ..., un) <: (t1, ..., tm, `...`t)
73447344
*)
7345-
let consumed =
7346-
List.length plain_params - List.length plain_params_remaining
7345+
let (consumed, required_params, optional_params) =
7346+
split_remaining_params_required_optional
7347+
non_variadic_or_splat_params
7348+
plain_params_remaining
73477349
in
73487350
let remaining_actual_tys =
73497351
List.drop (List.map argtys ~f:snd) consumed
@@ -7389,13 +7391,15 @@ end = struct
73897391
in
73907392
let expected_ty =
73917393
mk
7392-
( Reason.unpack_param (Pos.none, pos_def, consumed),
7394+
( Reason.witness_from_decl splat_param.fp_pos,
73937395
Ttuple
73947396
{
73957397
t_required =
7396-
List.map plain_params_remaining ~f:(fun (_, fp) ->
7398+
List.map required_params ~f:(fun (_, fp) ->
7399+
fp.fp_type);
7400+
t_optional =
7401+
List.map optional_params ~f:(fun (_, fp) ->
73977402
fp.fp_type);
7398-
t_optional = [];
73997403
t_extra = Tsplat splat_param.fp_type;
74007404
} )
74017405
in

‎hphp/hack/src/typing/typing_error.ml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1707,6 +1707,7 @@ and Secondary : sig
17071707
| Fun_param_required_but_expected_optional of {
17081708
pos: Pos_or_decl.t;
17091709
decl_pos: Pos_or_decl.t;
1710+
(* If empty, this is a positional parameter *)
17101711
param_names: string list;
17111712
}
17121713
| Required_field_is_optional of {

‎hphp/hack/src/typing/typing_error.mli‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1550,6 +1550,7 @@ and Secondary : sig
15501550
| Fun_param_required_but_expected_optional of {
15511551
pos: Pos_or_decl.t;
15521552
decl_pos: Pos_or_decl.t;
1553+
(* If empty, this is a positional parameter *)
15531554
param_names: string list;
15541555
}
15551556
| Required_field_is_optional of {

‎hphp/hack/src/typing/typing_error_utils.ml‎

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5510,17 +5510,20 @@ end = struct
55105510
lazy
55115511
[
55125512
( pos,
5513-
Printf.sprintf
5514-
"Named parameter%s %s %s required but expected to be optional"
5515-
(if List.length param_names > 1 then
5516-
"s"
5517-
else
5518-
"")
5519-
names_str
5520-
(if List.length param_names > 1 then
5521-
"are"
5522-
else
5523-
"is") );
5513+
if List.is_empty param_names then
5514+
"Parameter required but expected to be optional"
5515+
else
5516+
Printf.sprintf
5517+
"Named parameter%s %s %s required but expected to be optional"
5518+
(if List.length param_names > 1 then
5519+
"s"
5520+
else
5521+
"")
5522+
names_str
5523+
(if List.length param_names > 1 then
5524+
"are"
5525+
else
5526+
"is") );
55245527
(decl_pos, "Because of this definition");
55255528
]
55265529
in

‎hphp/hack/src/typing/typing_subtype.ml‎

Lines changed: 66 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -1639,25 +1639,15 @@ end = struct
16391639
(r_super, idx_super, ty_super)
16401640

16411641
and simplify_param_modes ~subtype_env ~fn_param_sub ~fn_param_super env =
1642-
let { fp_pos = pos1; _ } = fn_param_super in
1643-
let { fp_pos = pos2; _ } = fn_param_sub in
1644-
match (get_fp_mode fn_param_super, get_fp_mode fn_param_sub) with
1645-
| (FPnormal, FPnormal)
1646-
| (FPinout, FPinout) ->
1647-
valid env
1648-
| (FPnormal, FPinout) ->
1649-
invalid
1650-
~fail:
1651-
(Option.map
1652-
subtype_env.Subtype_env.on_error
1653-
~f:
1654-
Typing_error.(
1655-
fun on_error ->
1656-
apply_reasons ~on_error
1657-
@@ Secondary.Inoutness_mismatch
1658-
{ pos = pos2; decl_pos = pos1 }))
1659-
env
1660-
| (FPinout, FPnormal) ->
1642+
let { fp_pos = pos1; fp_name = name1; _ } = fn_param_super in
1643+
let { fp_pos = pos2; fp_name = name2; _ } = fn_param_sub in
1644+
(* Usually an arity check covers this case, except in the case that a splat parameter is preceded by an optional parameter *)
1645+
if
1646+
get_fp_is_optional fn_param_super
1647+
&& (not (get_fp_is_optional fn_param_sub))
1648+
&& Option.is_none name1
1649+
&& Option.is_none name2
1650+
then
16611651
invalid
16621652
~fail:
16631653
(Option.map
@@ -1666,9 +1656,38 @@ end = struct
16661656
Typing_error.(
16671657
fun on_error ->
16681658
apply_reasons ~on_error
1669-
@@ Secondary.Inoutness_mismatch
1670-
{ pos = pos1; decl_pos = pos2 }))
1659+
@@ Secondary.Fun_param_required_but_expected_optional
1660+
{ pos = pos2; decl_pos = pos1; param_names = [] }))
16711661
env
1662+
else
1663+
match (get_fp_mode fn_param_super, get_fp_mode fn_param_sub) with
1664+
| (FPnormal, FPnormal)
1665+
| (FPinout, FPinout) ->
1666+
valid env
1667+
| (FPnormal, FPinout) ->
1668+
invalid
1669+
~fail:
1670+
(Option.map
1671+
subtype_env.Subtype_env.on_error
1672+
~f:
1673+
Typing_error.(
1674+
fun on_error ->
1675+
apply_reasons ~on_error
1676+
@@ Secondary.Inoutness_mismatch
1677+
{ pos = pos2; decl_pos = pos1 }))
1678+
env
1679+
| (FPinout, FPnormal) ->
1680+
invalid
1681+
~fail:
1682+
(Option.map
1683+
subtype_env.Subtype_env.on_error
1684+
~f:
1685+
Typing_error.(
1686+
fun on_error ->
1687+
apply_reasons ~on_error
1688+
@@ Secondary.Inoutness_mismatch
1689+
{ pos = pos1; decl_pos = pos2 }))
1690+
env
16721691

16731692
and simplify_param_accept_disposable
16741693
~subtype_env ~fn_param_sub ~fn_param_super env =
@@ -2137,7 +2156,6 @@ end = struct
21372156
~arg_pos subtype_env fn_param_sub fn_param_super env =
21382157
let { fp_type = ty_sub; _ } = fn_param_sub
21392158
and { fp_type = ty_super; _ } = fn_param_super in
2140-
21412159
(* Construct the subtype proposition for the two parameters; function
21422160
paramaters are contravariant unless they are marked with `inout`
21432161
in which case they are typed as invariant *)
@@ -2351,7 +2369,32 @@ end = struct
23512369
~this_ty:None
23522370
~lhs:{ sub_supportdyn = None; ty_sub = tuple_ty }
23532371
~rhs:{ super_like = false; super_supportdyn = false; ty_super = ty_sub }
2354-
| ([], _) -> valid
2372+
(* We've run out of parameters *)
2373+
| ([], _ :: _) ->
2374+
let splat_super =
2375+
match List.last positional_params_super with
2376+
| Some fp -> get_fp_splat fp
2377+
| None -> false
2378+
in
2379+
if splat_super then
2380+
let fail =
2381+
Option.map
2382+
subtype_env.Subtype_env.on_error
2383+
~f:
2384+
Typing_error.(
2385+
fun on_error ->
2386+
apply_reasons ~on_error
2387+
@@ Secondary.Fun_too_few_args
2388+
{
2389+
pos = Reason.to_pos r_sub;
2390+
decl_pos = Reason.to_pos r_super;
2391+
actual = idx_sub;
2392+
expected = List.length fn_params_super;
2393+
})
2394+
in
2395+
invalid ~fail
2396+
else
2397+
valid
23552398
| (_, []) -> valid
23562399
| (fn_param_sub :: fn_params_sub, fn_param_super :: fn_params_super) ->
23572400
let (arg_pos, arg_posl) =

‎hphp/hack/test/typecheck/splat/type_splat_bad.php‎

Lines changed: 39 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -3,38 +3,64 @@
33
<<file: __EnableUnstableFeatures('open_tuples', 'type_splat')>>
44

55
// Illegal because splat is followed by another parameter (two errors)
6-
function bad1<Targs as (mixed...)>((function(...Targs, int):void) $f, ...Targs $params, int $arg):void {
6+
function bad1<Targs as (mixed...)>(
7+
(function(...Targs, int): void) $f,
8+
... Targs $params,
9+
int $arg,
10+
): void {
711
}
812

913
// Illegal because splat is followed by a variadic (two errors)
10-
function bad2<Targs as (mixed...)>((function(...Targs, int...):void) $f, ... Targs $params, int... $args):void {
14+
function bad2<Targs as (mixed...)>(
15+
(function(...Targs, int...): void) $f,
16+
... Targs $params,
17+
int ...$args
18+
): void {
1119
}
1220

1321
// Illegal because splat is followed by an optional (two errors)
14-
function bad3<Targs as (mixed...)>((function(...Targs, optional int):void) $f, ... Targs $params, int $arg = 3):void {
15-
}
16-
17-
// Illegal because splat is preceded by an optional or default parameter (two errors)
18-
function bad4<Targs as (mixed...)>((function(optional int, ...Targs):void) $f, int $arg = 2, ... Targs $params):void {
22+
function bad3<Targs as (mixed...)>(
23+
(function(...Targs, optional int): void) $f,
24+
... Targs $params,
25+
int $arg = 3,
26+
): void {
1927
}
2028

2129
// Illegal because splat is preceded by a variadic (two errors)
22-
function bad5<Targs as (mixed...)>((function(int..., ...Targs):void) $f, int... $args, ... Targs $params):void {
30+
function bad5<Targs as (mixed...)>(
31+
(function(int..., ...Targs): void) $f,
32+
int ...$args,
33+
... Targs $params,
34+
): void {
2335
}
2436

2537
// Illegal because splat has default value (one error)
26-
function bad6<Targs super (int,int,mixed...)>(... Targs $params = tuple(1,2)):void {
38+
function bad6<Targs super (int, int, mixed...)>(
39+
... Targs $params = tuple(1, 2),
40+
): void {
2741
}
2842

2943
interface I {
3044
// Illegal because splat is optional (two errors)
31-
public function bad7<Targs as (mixed...)>((function(int, optional ...Targs):void) $f, optional ...Targs $params):void;
45+
public function bad7<Targs as (mixed...)>(
46+
(function(int, optional ...Targs): void) $f,
47+
optional ... Targs $params,
48+
): void;
3249
// Illegal because splat is variadic (two errors)
33-
public function bad8<Targs as (mixed...)>((function(int, ...Targs...):void) $f, ...Targs... $params):void;
50+
public function bad8<Targs as (mixed...)>(
51+
(function(int, ...Targs...): void) $f,
52+
... Targs ...$params
53+
): void;
3454
}
3555

3656
// Illegal because splat has inout (two errors)
37-
function bad9<Targs as (mixed...)>((function(int, inout ...Targs):void) $f, inout ...Targs $args):void { }
57+
function bad9<Targs as (mixed...)>(
58+
(function(int, inout ...Targs): void) $f,
59+
inout ... Targs $args,
60+
): void {}
3861

3962
// Illegal because splat has readonly (two errors)
40-
function bad10<Targs as (mixed...)>((function(int, readonly ...Targs):void) $f, readonly ...Targs $args):void { }
63+
function bad10<Targs as (mixed...)>(
64+
(function(int, readonly ...Targs): void) $f,
65+
readonly ... Targs $args,
66+
): void {}

‎hphp/hack/test/typecheck/splat/type_splat_bad.php.exp‎

Lines changed: 24 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,54 +1,48 @@
1-
ERROR: File "type_splat_bad.php", line 6, characters 46-53:
1+
ERROR: File "type_splat_bad.php", line 7, characters 13-20:
22
A splat parameter `...` may only appear at the end of a parameter list. (Parsing[1002])
3-
ERROR: File "type_splat_bad.php", line 6, characters 71-86:
3+
ERROR: File "type_splat_bad.php", line 8, characters 3-19:
44
A splat parameter `...` may only appear at the end of a parameter list. (Parsing[1002])
5-
ERROR: File "type_splat_bad.php", line 10, characters 46-53:
5+
ERROR: File "type_splat_bad.php", line 15, characters 13-20:
66
A splat parameter `...` may only appear at the end of a parameter list. (Parsing[1002])
7-
ERROR: File "type_splat_bad.php", line 10, characters 74-90:
7+
ERROR: File "type_splat_bad.php", line 16, characters 3-19:
88
A splat parameter `...` may only appear at the end of a parameter list. (Parsing[1002])
9-
ERROR: File "type_splat_bad.php", line 14, characters 46-53:
9+
ERROR: File "type_splat_bad.php", line 23, characters 13-20:
1010
A splat parameter `...` may only appear at the end of a parameter list. (Parsing[1002])
11-
ERROR: File "type_splat_bad.php", line 14, characters 80-96:
11+
ERROR: File "type_splat_bad.php", line 24, characters 3-19:
1212
A splat parameter `...` may only appear at the end of a parameter list. (Parsing[1002])
13-
ERROR: File "type_splat_bad.php", line 18, characters 36-74:
14-
Optional parameter cannot precede non-optional parameter (Parsing[1002])
15-
ERROR: File "type_splat_bad.php", line 22, characters 46-51:
13+
ERROR: File "type_splat_bad.php", line 31, characters 13-18:
1614
A variadic parameter `...` may only appear at the end of a parameter list. (Parsing[1002])
17-
ERROR: File "type_splat_bad.php", line 22, characters 74-85:
15+
ERROR: File "type_splat_bad.php", line 32, characters 3-14:
1816
A variadic parameter `...` may only appear at the end of a parameter list. (Parsing[1002])
19-
ERROR: File "type_splat_bad.php", line 26, characters 47-76:
17+
ERROR: File "type_splat_bad.php", line 39, characters 3-33:
2018
A splat parameter `...` must not have a default value. (Parsing[1002])
21-
ERROR: File "type_splat_bad.php", line 31, characters 60-76:
19+
ERROR: File "type_splat_bad.php", line 46, characters 20-36:
2220
A splat parameter cannot also be optional. (Parsing[1002])
23-
ERROR: File "type_splat_bad.php", line 31, characters 89-113:
21+
ERROR: File "type_splat_bad.php", line 47, characters 5-30:
2422
A splat parameter cannot also be optional. (Parsing[1002])
25-
ERROR: File "type_splat_bad.php", line 33, characters 60-70:
23+
ERROR: File "type_splat_bad.php", line 51, characters 20-30:
2624
A splat parameter cannot also be variadic. (Parsing[1002])
27-
ERROR: File "type_splat_bad.php", line 33, characters 83-101:
25+
ERROR: File "type_splat_bad.php", line 52, characters 5-24:
2826
A splat parameter cannot also be variadic. (Parsing[1002])
29-
ERROR: File "type_splat_bad.php", line 37, characters 51-64:
27+
ERROR: File "type_splat_bad.php", line 58, characters 18-31:
3028
A splat parameter `...` cannot have a modifier that changes the calling convention, like `inout`. (Parsing[1002])
31-
ERROR: File "type_splat_bad.php", line 37, characters 77-96:
29+
ERROR: File "type_splat_bad.php", line 59, characters 3-23:
3230
A splat parameter `...` cannot have a modifier that changes the calling convention, like `inout`. (Parsing[1002])
33-
ERROR: File "type_splat_bad.php", line 40, characters 52-68:
31+
ERROR: File "type_splat_bad.php", line 64, characters 18-34:
3432
Splat parameters cannot be marked readonly (Parsing[1002])
35-
ERROR: File "type_splat_bad.php", line 40, characters 81-103:
33+
ERROR: File "type_splat_bad.php", line 65, characters 3-26:
3634
Splat parameters cannot be marked readonly (Parsing[1002])
37-
ERROR: File "type_splat_bad.php", line 18, characters 104-110:
38-
A previous parameter has a default value.
39-
Remove all the default values for the preceding parameters,
40-
or add a default value to this one. (Typing[4077])
41-
ERROR: File "type_splat_bad.php", line 26, characters 51-55:
35+
ERROR: File "type_splat_bad.php", line 39, characters 7-11:
4236
Some type arguments violate their constraints (Typing[4323])
43-
File "type_splat_bad.php", line 26, characters 57-63:
37+
File "type_splat_bad.php", line 39, characters 13-19:
4438
Expected `(mixed...)`
45-
File "type_splat_bad.php", line 26, characters 51-55:
39+
File "type_splat_bad.php", line 39, characters 7-11:
4640
But got `Targs super (int, int, mixed...)`
47-
ERROR: File "type_splat_bad.php", line 26, characters 51-55:
41+
ERROR: File "type_splat_bad.php", line 39, characters 7-11:
4842
Some type arguments violate their constraints (Typing[4323])
49-
File "type_splat_bad.php", line 26, characters 57-63:
43+
File "type_splat_bad.php", line 39, characters 13-19:
5044
Expected `(mixed...)`
51-
File "type_splat_bad.php", line 26, characters 51-55:
45+
File "type_splat_bad.php", line 39, characters 7-11:
5246
But got `mixed`
53-
File "type_splat_bad.php", line 26, characters 51-55:
47+
File "type_splat_bad.php", line 39, characters 7-11:
5448
by the definition of `Targs` arising from an implicit `as mixed` constraint on this type

0 commit comments

Comments
 (0)