Skip to content

Commit 72b236c

Browse files
Catherine Gasnierfacebook-github-bot
authored andcommitted
recursive case type: make bound check less restrictive
Summary: This diff aims at supporting cases like: ``` case type CT1 as shape(?'a' => nonnull) = shape(?'a' => CT1); case type CT2 as shape(?'a' => shape(?'a' => nonnull)) = shape(?'a' => CT2); ``` The previous behaviour was to localize CT1/CT2 as mixed and therefore fail the bound check. That localization to mixed was due to passing `~report_cycle:CT1/2` to `localize`. Upon detection of any cycle, localization stops and creates a mixed. Passing `~report_cycle:CT1/2` makes it localize the CT1/2 to mixed, which is wrong for further usages. This diff calls localize twice, once to report cycles, and once to use the result for bound checks and other checks. Reviewed By: andrewjkennedy Differential Revision: D67938199 fbshipit-source-id: bdd850d909294abafaba5ff76fcc37f1229515e3
1 parent 5d6851d commit 72b236c

File tree

6 files changed

+35
-30
lines changed

6 files changed

+35
-30
lines changed

hphp/hack/src/typing/typing_typedef.ml

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,22 @@ let casetype_def env typedef =
205205
Option.iter ~f:(Typing_error_utils.add_typing_error ~env) err2;
206206
env
207207

208+
let check_cycles env (t_pos, t_name) hints =
209+
let (env, cycles) =
210+
List.fold_left hints ~init:(env, []) ~f:(fun (env, cycles) hint ->
211+
let ((env, _ty_err_opt, new_cycles), _ty) =
212+
Phase.localize_hint_no_subst_report_cycles
213+
env
214+
~ignore_errors:true
215+
~report_cycle:(t_pos, Type_expansions.Expandable.Type_alias t_name)
216+
hint
217+
in
218+
(env, new_cycles @ cycles))
219+
in
220+
Type_expansions.report cycles
221+
|> Option.iter ~f:(Typing_error_utils.add_typing_error ~env);
222+
env
223+
208224
let typedef_def ctx typedef =
209225
let env = EnvFromDef.typedef_env ~origin:Decl_counters.TopLevel ctx typedef in
210226
let {
@@ -242,7 +258,6 @@ let typedef_def ctx typedef =
242258
not (recursive_case_types env)),
243259
List.map (variant :: variants) ~f:(fun v -> v.tctv_hint) )
244260
in
245-
let (t_pos, t_name_) = t_name in
246261
let env = Env.set_current_module env t_module in
247262
let env = Env.set_internal env t_internal in
248263
let env = Env.set_current_package_membership env t_package in
@@ -260,28 +275,24 @@ let typedef_def ctx typedef =
260275
Typing_variance.typedef env typedef;
261276

262277
let env =
263-
let (env, ty_err_opt2, cycles, tys) =
278+
if do_report_cycles env then
279+
check_cycles env t_name hints
280+
else
281+
env
282+
in
283+
let env =
284+
let (env, ty_err_opt2, tys) =
264285
List.fold_left
265286
hints
266-
~init:(env, None, [], [])
267-
~f:(fun (env, ty_err_opt2, cycles, tys) hint ->
268-
let ((env, new_ty_err_opt2, new_cycles), ty) =
269-
(* This also detects cyclic definitions *)
270-
Phase.localize_hint_no_subst_report_cycles
271-
env
272-
~ignore_errors:false
273-
~report_cycle:
274-
(t_pos, Type_expansions.Expandable.Type_alias t_name_)
275-
hint
287+
~init:(env, None, [])
288+
~f:(fun (env, ty_err_opt2, tys) hint ->
289+
let ((env, new_ty_err_opt2), ty) =
290+
Phase.localize_hint_no_subst env ~ignore_errors:false hint
276291
in
277292
( env,
278293
Option.merge ~f:Typing_error.both new_ty_err_opt2 ty_err_opt2,
279-
new_cycles @ cycles,
280294
ty :: tys ))
281295
in
282-
if do_report_cycles env then
283-
Type_expansions.report cycles
284-
|> Option.iter ~f:(Typing_error_utils.add_typing_error ~env);
285296
let env = casetype_def env typedef in
286297
Option.iter ~f:(Typing_error_utils.add_typing_error ~env) ty_err_opt2;
287298

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
<?hh
2+
<<file: __EnableUnstableFeatures('case_types')>>
3+
4+
case type CT1 as shape(?'a' => nonnull) = shape(?'a' => CT1);
5+
case type CT2 as shape(?'a' => shape(?'a' => nonnull)) = shape(?'a' => CT2);
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
No errors

hphp/hack/test/typecheck/case_type/recursive/empty_types1.php.exp

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,13 +44,13 @@ ERROR: File "empty_types1.php", line 27, characters 20-29:
4444
Invalid type hint (Typing[4062])
4545
File "empty_types1.php", line 27, characters 29-29:
4646
You are trying to access the type constant `T` but this is a mixed value
47-
File "empty_types1.php", line 27, characters 21-29:
47+
File "empty_types1.php", line 27, characters 11-16:
4848
Definition is here
4949
ERROR: File "empty_types1.php", line 28, characters 21-30:
5050
Invalid type hint (Typing[4062])
5151
File "empty_types1.php", line 28, characters 30-30:
5252
You are trying to access the type constant `T` but this is a mixed value
53-
File "empty_types1.php", line 28, characters 21-30:
53+
File "empty_types1.php", line 28, characters 11-17:
5454
Definition is here
5555
ERROR: File "empty_types1.php", line 29, characters 11-15:
5656
Invalid case type declaration. More than one variant of `Acces` could contain values with the same runtime tag (Typing[4475])
Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,2 @@
11
ERROR: File "subtyping_simple1c.php", line 4, characters 11-11:
2-
Invalid constraint on `newtype` (Typing[4332])
3-
File "subtyping_simple1c.php", line 4, characters 16-21:
4-
Expected `string`
5-
File "subtyping_simple1c.php", line 4, characters 25-25:
6-
But got `mixed`. Using `mixed` instead of A because this is an illegal recursive use of A in the definition of A
7-
ERROR: File "subtyping_simple1c.php", line 4, characters 11-11:
82
This recursive case type is not supported. `A` should not be at the top-level. (Typing[4485])

hphp/hack/test/typecheck/case_type/recursive/subtyping_simple2b.php.exp

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,3 @@ Invalid case type declaration. More than one variant of `A` could contain values
66
via this variant of the case type `A`
77
File "subtyping_simple2b.php", line 5, characters 5-7:
88
It overlaps with `int`, which also includes **ints**
9-
ERROR: File "subtyping_simple2b.php", line 4, characters 11-11:
10-
Invalid constraint on `newtype` (Typing[4332])
11-
File "subtyping_simple2b.php", line 4, characters 16-18:
12-
Expected `int`
13-
File "subtyping_simple2b.php", line 4, characters 22-22:
14-
But got `mixed`. Using `mixed` instead of A because this is an illegal recursive use of A in the definition of A

0 commit comments

Comments
 (0)