From b9b8c127a772dd2ca11a1403d0525f2020af4b7f Mon Sep 17 00:00:00 2001 From: Leo Osvald Date: Wed, 11 Nov 2020 14:14:19 -0800 Subject: [PATCH] Register capabilities after saving env for anon. fun Summary: This was mistakenly done too soon, before the locals are saved (so that they could be restored after type-checking lambdas). The fixed version is more complicated because we need to thread through either a `Tfun` or a capability (implicit parameters); I chose the former as it offers a slight perf win (we don't need to construct a `Tfun` twice). To avoid ad-hoc changes to signatures and munging returned data across *several* higher-order functions, generalize the return type of `anon_make` as `env * 'a`, which is now consistent with other helper functions around it. Reviewed By: vassilmladenov Differential Revision: D24881024 fbshipit-source-id: ead1b424aa0bca4ac71c6fb0c1f90358ccf19915 --- hphp/hack/src/typing/typing.ml | 88 ++++++++++------------ hphp/hack/src/typing/typing_env.ml | 4 +- hphp/hack/src/typing/typing_env.mli | 6 +- .../typecheck/coeffects/closures_restore_env.php | 24 ++++++ .../coeffects/closures_restore_env.php.exp | 6 ++ 5 files changed, 73 insertions(+), 55 deletions(-) create mode 100644 hphp/hack/test/typecheck/coeffects/closures_restore_env.php create mode 100644 hphp/hack/test/typecheck/coeffects/closures_restore_env.php.exp diff --git a/hphp/hack/src/typing/typing.ml b/hphp/hack/src/typing/typing.ml index 6cda3c7fdf8..07aff349548 100644 --- a/hphp/hack/src/typing/typing.ml +++ b/hphp/hack/src/typing/typing.ml @@ -2658,29 +2658,6 @@ and expr_ | _ -> () end; - (* Extract capabilities from AAST and add them to the environment *) - let (env, capability) = - match (hint_of_type_hint f.f_cap, hint_of_type_hint f.f_unsafe_cap) with - | (None, None) -> - (* if the closure has no explicit coeffect annotations, - do _not_ insert (unsafe) capabilities into the environment; - instead, rely on the fact that a capability from an enclosing - scope can simply be captured, which has the same semantics - as redeclaring and shadowing with another same-typed capability. - This avoid unnecessary overhead in the most common case, i.e., - when a closure does not need a different (usually smaller) - set of capabilities. *) - (env, Env.get_local env Typing_coeffects.local_capability_id) - | (_, _) -> - let (env, f_cap, f_unsafe_cap) = - type_capability env f.f_cap f.f_unsafe_cap (fst f.f_name) - in - Typing_coeffects.register_capabilities - env - (type_of_type_hint f_cap) - (type_of_type_hint f_unsafe_cap) - in - (* Is the return type declared? *) let is_explicit_ret = Option.is_some (hint_of_type_hint f.f_ret) in let reactivity = @@ -2696,13 +2673,12 @@ and expr_ { ft with ft_reactive = reactivity; - ft_implicit_params = { capability }; ft_flags = Typing_defs_flags.( set_bit ft_flags_is_coroutine is_coroutine ft.ft_flags); } in - let (env, tefun, ty) = anon_make ?ret_ty env p f ft idl is_anon in + let (env, (tefun, ty, ft)) = anon_make ?ret_ty env p f ft idl is_anon in let env = Env.set_env_reactive env old_reactivity in let inferred_ty = mk @@ -3221,30 +3197,48 @@ and stash_conts_for_anon env p is_anon captured f = let tpenv = Env.get_tpenv env in (initial_locals, initial_fakes, tpenv)) in - let (env, (tfun, result)) = - Typing_lenv.stash_and_do env (Env.all_continuations env) (fun env -> - let env = - match init with - | None -> env - | Some (initial_locals, initial_fakes, tpenv) -> - let env = Env.reinitialize_locals env in - let env = Env.set_locals env initial_locals in - let env = Env.set_fake_members env initial_fakes in - let env = Env.env_with_tpenv env tpenv in - env - in - let (env, tfun, result) = f env in - (env, (tfun, result))) - in - (env, tfun, result) + Typing_lenv.stash_and_do env (Env.all_continuations env) (fun env -> + let env = + match init with + | None -> env + | Some (initial_locals, initial_fakes, tpenv) -> + let env = Env.reinitialize_locals env in + let env = Env.set_locals env initial_locals in + let env = Env.set_fake_members env initial_fakes in + let env = Env.env_with_tpenv env tpenv in + env + in + f env) (* Make a type-checking function for an anonymous function. *) (* Here ret_ty should include Awaitable wrapper *) (* TODO: ?el is never set; so we need to fix variadic use of lambda *) and anon_make ?el ?ret_ty env lambda_pos f ft idl is_anon = - let anon_lenv = env.lenv in let nb = Nast.assert_named_body f.f_body in - Env.anon anon_lenv env (fun env -> + Env.anon env.lenv env (fun env -> + (* Extract capabilities from AAST and add them to the environment *) + let (env, capability) = + match (hint_of_type_hint f.f_cap, hint_of_type_hint f.f_unsafe_cap) with + | (None, None) -> + (* if the closure has no explicit coeffect annotations, + do _not_ insert (unsafe) capabilities into the environment; + instead, rely on the fact that a capability from an enclosing + scope can simply be captured, which has the same semantics + as redeclaring and shadowing with another same-typed capability. + This avoid unnecessary overhead in the most common case, i.e., + when a closure does not need a different (usually smaller) + set of capabilities. *) + (env, Env.get_local env Typing_coeffects.local_capability_id) + | (_, _) -> + let (env, f_cap, f_unsafe_cap) = + type_capability env f.f_cap f.f_unsafe_cap (fst f.f_name) + in + Typing_coeffects.register_capabilities + env + (type_of_type_hint f_cap) + (type_of_type_hint f_unsafe_cap) + in + let ft = { ft with ft_implicit_params = { capability } } in stash_conts_for_anon env lambda_pos is_anon idl (fun env -> let env = Env.clear_params env in let make_variadic_arg env varg tyl = @@ -3401,7 +3395,6 @@ and anon_make ?el ?ret_ty env lambda_pos f ft idl is_anon = Aast.f_file_attributes = []; Aast.f_user_attributes = user_attributes; Aast.f_body = { Aast.fb_ast = tb; fb_annotation = () }; - (* TODO(T70095684) definitely fix f_cap *) Aast.f_cap; Aast.f_unsafe_cap; Aast.f_params = t_params; @@ -3424,10 +3417,9 @@ and anon_make ?el ?ret_ty env lambda_pos f ft idl is_anon = Aast.Lfun (tfun_, idl) ) in let env = Env.set_tyvar_variance env ty in - (env, te, hret))) - -(* stash_conts_for_anon *) -(* Env.anon *) + (env, (te, hret, ft)) + (* stash_conts_for_anon *)) + (* Env.anon *)) (*****************************************************************************) (* End of anonymous functions. *) diff --git a/hphp/hack/src/typing/typing_env.ml b/hphp/hack/src/typing/typing_env.ml index 221904bbcb8..e0598581b94 100644 --- a/hphp/hack/src/typing/typing_env.ml +++ b/hphp/hack/src/typing/typing_env.ml @@ -1295,13 +1295,13 @@ let anon anon_lenv env f = let outer_fun_kind = get_fn_kind env in let env = { env with lenv = anon_lenv } in (* Typing *) - let (env, tfun, result) = f env in + let (env, ret) = f env in (* Cleaning up the environment. *) let env = { env with lenv = old_lenv } in let env = set_params env old_params in let env = set_return env old_return in let env = set_fn_kind env outer_fun_kind in - (env, tfun, result) + (env, ret) let in_try env f = let old_in_try = env.in_try in diff --git a/hphp/hack/src/typing/typing_env.mli b/hphp/hack/src/typing/typing_env.mli index 7744b7e81c6..d5b0c0ec22f 100644 --- a/hphp/hack/src/typing/typing_env.mli +++ b/hphp/hack/src/typing/typing_env.mli @@ -420,11 +420,7 @@ val env_with_locals : env -> Typing_per_cont_env.t -> env val reinitialize_locals : env -> env -val anon : - local_env -> - env -> - (env -> env * Tast.expr * locl_ty) -> - env * Tast.expr * locl_ty +val anon : local_env -> env -> (env -> env * 'a) -> env * 'a val in_try : env -> (env -> env * 'a) -> env * 'a diff --git a/hphp/hack/test/typecheck/coeffects/closures_restore_env.php b/hphp/hack/test/typecheck/coeffects/closures_restore_env.php new file mode 100644 index 00000000000..1ee059ced81 --- /dev/null +++ b/hphp/hack/test/typecheck/coeffects/closures_restore_env.php @@ -0,0 +1,24 @@ +> + +function callee()[non_det]: void {} + +function good_caller()[non_det]: void { + $l_unrelated = ()[] ==> {}; + // regression test to verify that capabilities are restored + // _after_ type-checking lambda; they shouldn't leak into enclosing scope + callee(); + + { + $l_unrelated = ()[rx] ==> {}; + callee(); + } +} + +function bad_caller()[]: void { + $l_unrelated = ()[non_det] ==> {}; + // test the inverse case than above; capabilities should _not_ persist + // _after_ type-checking the lambda, therefore the call should be disallowed + callee(); +} diff --git a/hphp/hack/test/typecheck/coeffects/closures_restore_env.php.exp b/hphp/hack/test/typecheck/coeffects/closures_restore_env.php.exp new file mode 100644 index 00000000000..11649cfc319 --- /dev/null +++ b/hphp/hack/test/typecheck/coeffects/closures_restore_env.php.exp @@ -0,0 +1,6 @@ +File "closures_restore_env.php", line 23, characters 3-10: +This call is not allowed because its coeffects are incompatible with the context (Typing[4390]) + File "closures_restore_env.php", line 19, characters 22-23: + From this declaration, the context of this function body provides the capability set {} + File "closures_restore_env.php", line 5, characters 18-26: + But the function being called requires the capability set {NonDet} -- 2.11.4.GIT