From ca8fa31e4e79b2b0254e8279303f601a33a037e7 Mon Sep 17 00:00:00 2001 From: Andrew Kennedy Date: Mon, 15 Jul 2019 13:20:46 -0700 Subject: [PATCH] Generate fresh type parameters away from others in same method body Summary: When refining to a generic type, we generate "fresh" abstract types with names derived from the generic parameters. For example, `$x is C<_>` for a generic class `class C { ... }` would generate a name such as `Tc#1`. We generate names "away from" existing generic parameters. Unfortunately, they may clash with other names generated in the same body, and end up flowing to a common place. This is unsound. For example: ``` if ($b) { invariant($y is C<_>, "C"); } else { invariant($y is D<_>, "D"); } ``` This might generate the *same* name for the abstract type parameter in both branches of the conditional, which then leaks out at the join point. See the test in this diff for an example of a program that should be rejected but which is currently accepted. The fix is easy: maintain in `env` a set of already-generated type parameter names. Fresh names should be generated "away" from these. Reviewed By: kmeht Differential Revision: D16200769 fbshipit-source-id: 9a251039d296596b732be01d8ed9052feaf9c5a5 --- hphp/hack/src/typing/typing_env.ml | 4 ++- hphp/hack/src/typing/typing_env_types.ml | 1 + hphp/hack/src/typing/typing_env_types_sig.mli | 1 + hphp/hack/src/typing/typing_log.ml | 2 ++ .../instanceof/instanceof_static_with_reqs.php.exp | 2 +- .../test/typecheck/is_expression/merge_is_bad.php | 33 ++++++++++++++++++++++ .../typecheck/is_expression/merge_is_bad.php.exp | 8 ++++++ 7 files changed, 49 insertions(+), 2 deletions(-) create mode 100644 hphp/hack/test/typecheck/is_expression/merge_is_bad.php create mode 100644 hphp/hack/test/typecheck/is_expression/merge_is_bad.php.exp diff --git a/hphp/hack/src/typing/typing_env.ml b/hphp/hack/src/typing/typing_env.ml index 901ba8ca79a..c45f1e16c22 100644 --- a/hphp/hack/src/typing/typing_env.ml +++ b/hphp/hack/src/typing/typing_env.ml @@ -378,7 +378,7 @@ let add_generic_parameters env tparaml = (List.fold_left tparaml ~f:add_empty_bounds ~init:(get_tpenv env)) let is_generic_parameter env name = - SMap.mem name (get_tpenv env) + SMap.mem name (get_tpenv env) || SSet.mem name env.fresh_typarams let get_generic_parameters env = SMap.keys (SMap.union (get_tpenv env) env.global_tpenv) @@ -476,6 +476,7 @@ let add_fresh_generic_parameter env prefix ~reified ~enforceable ~newable = let name = Printf.sprintf "%s#%d" prefix i in if is_generic_parameter env name then iterate (i+1) else name in let name = iterate 1 in + let env = { env with fresh_typarams = SSet.add name env.fresh_typarams } in let env = env_with_tpenv env (SMap.add name {lower_bounds = empty_bounds; @@ -539,6 +540,7 @@ let empty tcopt file ~droot = { function_pos = Pos.none; tenv = IMap.empty; subst = IMap.empty; + fresh_typarams = SSet.empty; lenv = initial_local SMap.empty Nonreactive; in_loop = false; in_try = false; diff --git a/hphp/hack/src/typing/typing_env_types.ml b/hphp/hack/src/typing/typing_env_types.ml index baa18020747..eaab217cfe2 100644 --- a/hphp/hack/src/typing/typing_env_types.ml +++ b/hphp/hack/src/typing/typing_env_types.ml @@ -87,6 +87,7 @@ type env = { function_pos: Pos.t; tenv : locl_ty IMap.t ; subst : int IMap.t ; + fresh_typarams : SSet.t; lenv : local_env ; genv : genv ; decl_env: Decl_env.env; diff --git a/hphp/hack/src/typing/typing_env_types_sig.mli b/hphp/hack/src/typing/typing_env_types_sig.mli index 23269d6602c..3731db1650b 100644 --- a/hphp/hack/src/typing/typing_env_types_sig.mli +++ b/hphp/hack/src/typing/typing_env_types_sig.mli @@ -45,6 +45,7 @@ function_pos: Pos.t ; tenv : locl ty IMap.t ; subst : int IMap.t ; + fresh_typarams : SSet.t; lenv : local_env ; genv : genv ; decl_env: Decl_env.env; diff --git a/hphp/hack/src/typing/typing_log.ml b/hphp/hack/src/typing/typing_log.ml index 8e9fb545346..9602f58a92e 100644 --- a/hphp/hack/src/typing/typing_log.ml +++ b/hphp/hack/src/typing/typing_log.ml @@ -416,6 +416,7 @@ let env_as_value env = function_pos; tenv; subst; + fresh_typarams; lenv; genv; decl_env = _; @@ -437,6 +438,7 @@ let env_as_value env = "tvenv", tvenv_as_value env tvenv; "tenv", tenv_as_value env tenv; "subst", subst_as_value subst; + "fresh_typarams", Set fresh_typarams; "tyvars_stack", tyvars_stack_as_value tyvars_stack; "lenv", lenv_as_value env lenv; "genv", genv_as_value env genv; diff --git a/hphp/hack/test/typecheck/instanceof/instanceof_static_with_reqs.php.exp b/hphp/hack/test/typecheck/instanceof/instanceof_static_with_reqs.php.exp index 0ef670c926f..c8fd5cce43f 100644 --- a/hphp/hack/test/typecheck/instanceof/instanceof_static_with_reqs.php.exp +++ b/hphp/hack/test/typecheck/instanceof/instanceof_static_with_reqs.php.exp @@ -3,7 +3,7 @@ File "instanceof_static_with_reqs.php", line 10, characters 5-19: File "instanceof_static_with_reqs.php", line 17, characters 5-19: this as BaseField File "instanceof_static_with_reqs.php", line 26, characters 7-21: - (this as BaseField & INeedsFoo) + (this as BaseField & INeedsFoo) File "instanceof_static_with_reqs.php", line 30, characters 5-19: this as BaseField No errors diff --git a/hphp/hack/test/typecheck/is_expression/merge_is_bad.php b/hphp/hack/test/typecheck/is_expression/merge_is_bad.php new file mode 100644 index 00000000000..bcf91fd2206 --- /dev/null +++ b/hphp/hack/test/typecheck/is_expression/merge_is_bad.php @@ -0,0 +1,33 @@ + { + public function get():T; +} +class C implements IGet { + public function __construct(private T $item) { } + public function get():T { return $this->item; } +} +class D implements IGet { + public function __construct(private T $item) { } + public function get():T { return $this->item; } +} + +class E implements I { } +function expectJ(J $x):void { } +function testit(mixed $m, bool $b):void { + if ($b) { + invariant($m is C<_>, "C"); + } else { + invariant($m is D<_>, "D"); + } + $a = $m->get(); + expectJ($a); +} + +<<__EntryPoint>> +function breakit():void { + testit(new C(new E()), true); +} diff --git a/hphp/hack/test/typecheck/is_expression/merge_is_bad.php.exp b/hphp/hack/test/typecheck/is_expression/merge_is_bad.php.exp new file mode 100644 index 00000000000..f4e5580f1ac --- /dev/null +++ b/hphp/hack/test/typecheck/is_expression/merge_is_bad.php.exp @@ -0,0 +1,8 @@ +File "merge_is_bad.php", line 27, characters 11-12: +Invalid argument (Typing[4110]) +File "merge_is_bad.php", line 19, characters 18-18: +Expected J +File "merge_is_bad.php", line 22, characters 21-24: +But got T#2 from this is expression test +File "merge_is_bad.php", line 11, characters 25-25: + via this generic T -- 2.11.4.GIT