From 6ff03e2d33c45e87b4c574e6dc0b6eacbf02235f Mon Sep 17 00:00:00 2001 From: James Wu Date: Fri, 19 Feb 2021 13:11:15 -0800 Subject: [PATCH] Readonly prop modifications Summary: This diff allows code that modifies readonly/mutable properties to work properly. Reviewed By: zilberstein Differential Revision: D26467795 fbshipit-source-id: a01493ce66f704dc06a72e1c8d55b8c046283f02 --- hphp/hack/src/typing/tast_check/readonly_check.ml | 69 +++++++++++++++------- hphp/hack/test/typecheck/readonly/args.php | 7 ++- hphp/hack/test/typecheck/readonly/args.php.exp | 10 ++-- hphp/hack/test/typecheck/readonly/prop.php | 7 +++ hphp/hack/test/typecheck/readonly/prop.php.exp | 12 +++- .../typecheck/readonly/prop.php.legacy_decl.exp | 12 +++- .../typecheck/readonly/prop.php.like_types.exp | 12 +++- 7 files changed, 96 insertions(+), 33 deletions(-) diff --git a/hphp/hack/src/typing/tast_check/readonly_check.ml b/hphp/hack/src/typing/tast_check/readonly_check.ml index df9576f7a80..cf388457608 100644 --- a/hphp/hack/src/typing/tast_check/readonly_check.ml +++ b/hphp/hack/src/typing/tast_check/readonly_check.ml @@ -91,14 +91,29 @@ let check = | (_, Array_get (e, Some _)) -> self#ty_expr e | _ -> Mut - method assign lval rval = + method assign env lval rval = match lval with - | (_, Obj_get (e1, _, _, _)) -> + | (_, Obj_get (obj, get, _, _)) -> begin - match self#ty_expr e1 with - | Readonly -> Errors.readonly_modified (Tast.get_position e1) + match self#ty_expr obj with + | Readonly -> Errors.readonly_modified (Tast.get_position obj) | Mut -> () - end + end; + let prop_elt = self#get_prop_elt env obj get in + (match (prop_elt, self#ty_expr rval) with + | (Some elt, Readonly) when not (Typing_defs.get_ce_readonly_prop elt) + -> + Errors.readonly_mismatch + "Invalid property assignment" + (Tast.get_position lval) + ~reason_sub: + [(Tast.get_position rval, "This expression is readonly")] + ~reason_super: + [ + ( Lazy.force elt.Typing_defs.ce_pos, + "But it's being assigned to a mutable property" ); + ] + | _ -> ()) | (_, Lvar (_, lid)) -> let var_ro_opt = SMap.find_opt (Local_id.to_string lid) ctx.lenv in begin @@ -195,7 +210,7 @@ let check = check_readonly_call caller_ty is_readonly; check_args caller_ty args unpacked_arg - method obj_get env obj get = + method get_prop_elt env obj get = let open Typing_defs in match (get_node (Tast.get_type obj), get) with (* Basic case of a single class and a statically known id: @@ -205,20 +220,20 @@ let check = (match Decl_provider.get_class provider_ctx (snd id) with | Some class_decl -> let prop = Cls.get_prop class_decl (snd prop_id) in - (match prop with - | Some elt -> - let prop_ro = get_ce_readonly_prop elt in - (match (prop_ro, self#ty_expr obj) with - | (true, Mut) -> - Errors.explicit_readonly_cast - "property" - (Tast.get_position get) - (Lazy.force elt.ce_pos) - | _ -> ()) - | None -> ()) - (* Class doesn't exist, error elsewhere*) - | None -> ()) + prop + (* Class doesn't exist, assume mutable *) + | None -> None) (* TODO: Handle more complex generic cases *) + | _ -> None + + method obj_get env obj get = + let prop_elt = self#get_prop_elt env obj get in + match (prop_elt, self#ty_expr obj) with + | (Some elt, Mut) when Typing_defs.get_ce_readonly_prop elt -> + Errors.explicit_readonly_cast + "property" + (Tast.get_position get) + (Lazy.force elt.Typing_defs.ce_pos) | _ -> () (* TODO: support obj get on generics, aliases and expression dependent types *) @@ -286,12 +301,22 @@ let check = ctx <- old_ctx; result - (* TODO: property accesses *) - (* TODO: lambda expressions *) method! on_expr env e = match e with + (* Property assignment *) + | ( _, + Binop + ( (Ast_defs.Eq _ as bop), + ((_, Obj_get (obj, get, nullable, is_prop_call)) as lval), + rval ) ) -> + self#assign env lval rval; + self#on_bop env bop; + (* During a property assignment, skip the self#expr call to avoid erroring *) + self#on_Obj_get env obj get nullable is_prop_call; + self#on_expr env rval + (* All other assignment *) | (_, Binop (Ast_defs.Eq _, lval, rval)) -> - self#assign lval rval; + self#assign env lval rval; super#on_expr env e (* Readonly calls *) | (_, ReadonlyExpr (_, Call (caller, targs, args, unpacked_arg))) -> diff --git a/hphp/hack/test/typecheck/readonly/args.php b/hphp/hack/test/typecheck/readonly/args.php index cde42f13e40..78d11e94123 100644 --- a/hphp/hack/test/typecheck/readonly/args.php +++ b/hphp/hack/test/typecheck/readonly/args.php @@ -9,8 +9,10 @@ class Foo { public readonly function takes_mutable(Foo $other) : void { $other->prop = 4; } - public readonly function takes_readonly(readonly Foo $other, Foo $mutable) : void { - $mutable->prop = $other->prop; + public readonly function takes_readonly( + readonly Foo $other, + Foo $mutable + ) : void { } } function takes_mutable(Foo $x) : void { @@ -18,7 +20,6 @@ function takes_mutable(Foo $x) : void { } function takes_readonly(readonly Foo $other, Foo $mutable) : void { - $mutable->prop = $other->prop; } function test(): void { diff --git a/hphp/hack/test/typecheck/readonly/args.php.exp b/hphp/hack/test/typecheck/readonly/args.php.exp index 75e138b995b..d51cfd25418 100644 --- a/hphp/hack/test/typecheck/readonly/args.php.exp +++ b/hphp/hack/test/typecheck/readonly/args.php.exp @@ -1,12 +1,12 @@ -File "args.php", line 28, characters 17-18: +File "args.php", line 29, characters 17-18: Invalid argument (Typing[4411]) - File "args.php", line 28, characters 17-18: + File "args.php", line 29, characters 17-18: This expression is readonly - File "args.php", line 16, characters 28-29: + File "args.php", line 18, characters 28-29: It is incompatible with this parameter, which is mutable -File "args.php", line 29, characters 21-22: +File "args.php", line 30, characters 21-22: Invalid argument (Typing[4411]) - File "args.php", line 29, characters 21-22: + File "args.php", line 30, characters 21-22: This expression is readonly File "args.php", line 9, characters 46-51: It is incompatible with this parameter, which is mutable diff --git a/hphp/hack/test/typecheck/readonly/prop.php b/hphp/hack/test/typecheck/readonly/prop.php index 1bc66c64324..e2a14f5aa2a 100644 --- a/hphp/hack/test/typecheck/readonly/prop.php +++ b/hphp/hack/test/typecheck/readonly/prop.php @@ -4,9 +4,11 @@ class Bar {} class Foo { public int $prop; public readonly Bar $ro; + public Bar $not_ro; public function __construct() { $this->prop = 1; $this->ro = new Bar(); + $this->not_ro = new Bar(); } public function set(int $y) : void { $this->prop = $y; @@ -24,4 +26,9 @@ function test(Foo $x): void { $z = readonly new Foo(); $a = $z->ro; // since $z is known to be readonly, // $z->ro is therefore always readonly, and doesn't need to be cast. + $x->ro = readonly new Bar(); + // Error, readonly value assigned to mutable + $x->not_ro = readonly new Bar(); + // Error, right side needs to be wrapped in readonly + $x->ro = $x->ro; } diff --git a/hphp/hack/test/typecheck/readonly/prop.php.exp b/hphp/hack/test/typecheck/readonly/prop.php.exp index 24b16a1a269..b39345e35ee 100644 --- a/hphp/hack/test/typecheck/readonly/prop.php.exp +++ b/hphp/hack/test/typecheck/readonly/prop.php.exp @@ -1,4 +1,14 @@ -File "prop.php", line 22, characters 12-13: +File "prop.php", line 24, characters 12-13: +This property returns a readonly value. It must be explicitly wrapped in a readonly expression. (Typing[4412]) + File "prop.php", line 6, characters 23-25: + The property is defined here. +File "prop.php", line 31, characters 3-12: +Invalid property assignment (Typing[4411]) + File "prop.php", line 31, characters 16-33: + This expression is readonly + File "prop.php", line 7, characters 14-20: + But it's being assigned to a mutable property +File "prop.php", line 33, characters 16-17: This property returns a readonly value. It must be explicitly wrapped in a readonly expression. (Typing[4412]) File "prop.php", line 6, characters 23-25: The property is defined here. diff --git a/hphp/hack/test/typecheck/readonly/prop.php.legacy_decl.exp b/hphp/hack/test/typecheck/readonly/prop.php.legacy_decl.exp index 292d3dd34c1..92f7f729bf5 100644 --- a/hphp/hack/test/typecheck/readonly/prop.php.legacy_decl.exp +++ b/hphp/hack/test/typecheck/readonly/prop.php.legacy_decl.exp @@ -1,4 +1,14 @@ -File "prop.php", line 22, characters 12-13: +File "prop.php", line 24, characters 12-13: +This property returns a readonly value. It must be explicitly wrapped in a readonly expression. (Typing[4412]) + File "prop.php", line 6, characters 19-21: + The property is defined here. +File "prop.php", line 31, characters 3-12: +Invalid property assignment (Typing[4411]) + File "prop.php", line 31, characters 16-33: + This expression is readonly + File "prop.php", line 7, characters 10-12: + But it's being assigned to a mutable property +File "prop.php", line 33, characters 16-17: This property returns a readonly value. It must be explicitly wrapped in a readonly expression. (Typing[4412]) File "prop.php", line 6, characters 19-21: The property is defined here. diff --git a/hphp/hack/test/typecheck/readonly/prop.php.like_types.exp b/hphp/hack/test/typecheck/readonly/prop.php.like_types.exp index 292d3dd34c1..92f7f729bf5 100644 --- a/hphp/hack/test/typecheck/readonly/prop.php.like_types.exp +++ b/hphp/hack/test/typecheck/readonly/prop.php.like_types.exp @@ -1,4 +1,14 @@ -File "prop.php", line 22, characters 12-13: +File "prop.php", line 24, characters 12-13: +This property returns a readonly value. It must be explicitly wrapped in a readonly expression. (Typing[4412]) + File "prop.php", line 6, characters 19-21: + The property is defined here. +File "prop.php", line 31, characters 3-12: +Invalid property assignment (Typing[4411]) + File "prop.php", line 31, characters 16-33: + This expression is readonly + File "prop.php", line 7, characters 10-12: + But it's being assigned to a mutable property +File "prop.php", line 33, characters 16-17: This property returns a readonly value. It must be explicitly wrapped in a readonly expression. (Typing[4412]) File "prop.php", line 6, characters 19-21: The property is defined here. -- 2.11.4.GIT