fix fanout bug when making an attribute required

CatherineGasnier · facebook-github-bot · commit 59f6ba82d11b · 2022-06-23T09:47:28.000-07:00
Reviewed By: bobrenjc93

Differential Revision: D37379772

fbshipit-source-id: 423b9ba68666d8867b713787a4636e5ae203c8e5
diff --git a/hphp/hack/src/decl/shallow_class_diff.ml b/hphp/hack/src/decl/shallow_class_diff.ml
@@ -43,6 +43,10 @@ module type Member_S = sig
   (** Whether adding this member implies that the constructor should be considered modified.
     This is the case for example for required XHP attributes. *)
   val constructor_is_modified_when_added : t -> bool
+
+  (** Whether modifying this member implies that the constructor should be considered modified.
+    This is the case for example for required XHP attributes. *)
+  val constructor_is_modified_when_modified : old:t -> new_:t -> bool
 end
 
 (** Returns the diff of two member lists, plus a diff on the constructor if the member changes
@@ -67,18 +71,34 @@ let diff_members
       | (Some _, None) ->
         (SMap.add diff ~key:name ~data:Removed, constructor_change)
       | (None, Some m) ->
-        ( SMap.add diff ~key:name ~data:Added,
+        let constructor_change =
           max_constructor_change
             constructor_change
             (if Member.constructor_is_modified_when_added m then
               Some Modified
             else
-              None) )
+              None)
+        in
+        (SMap.add diff ~key:name ~data:Added, constructor_change)
       | (Some old_member, Some new_member) ->
-        ( Member.diff old_member new_member
+        let member_changes =
+          Member.diff old_member new_member
           |> Option.value_map ~default:diff ~f:(fun ch ->
-                 SMap.add diff ~key:name ~data:ch),
-          constructor_change ))
+                 SMap.add diff ~key:name ~data:ch)
+        in
+        let constructor_change =
+          max_constructor_change
+            constructor_change
+            (if
+             Member.constructor_is_modified_when_modified
+               ~old:old_member
+               ~new_:new_member
+            then
+              Some Modified
+            else
+              None)
+        in
+        (member_changes, constructor_change))
 
 module ClassConst : Member_S with type t = shallow_class_const = struct
   type t = shallow_class_const
@@ -98,6 +118,8 @@ module ClassConst : Member_S with type t = shallow_class_const = struct
   let is_private _ = false
 
   let constructor_is_modified_when_added _ = false
+
+  let constructor_is_modified_when_modified ~old:_ ~new_:_ = false
 end
 
 module TypeConst : Member_S with type t = shallow_typeconst = struct
@@ -119,6 +141,8 @@ module TypeConst : Member_S with type t = shallow_typeconst = struct
   let is_private _ = false
 
   let constructor_is_modified_when_added _ = false
+
+  let constructor_is_modified_when_modified ~old:_ ~new_:_ = false
 end
 
 module Prop : Member_S with type t = shallow_prop = struct
@@ -141,9 +165,11 @@ module Prop : Member_S with type t = shallow_prop = struct
     Aast_defs.equal_visibility p.sp_visibility Aast_defs.Private
 
   let constructor_is_modified_when_added p =
-    match p.sp_xhp_attr with
-    | None -> false
-    | Some attr -> Xhp_attribute.is_required attr
+    Xhp_attribute.opt_is_required p.sp_xhp_attr
+
+  let constructor_is_modified_when_modified ~old ~new_ =
+    Xhp_attribute.opt_is_required new_.sp_xhp_attr
+    && (not @@ Xhp_attribute.opt_is_required old.sp_xhp_attr)
 end
 
 module Method : Member_S with type t = shallow_method = struct
@@ -166,6 +192,8 @@ module Method : Member_S with type t = shallow_method = struct
     Aast_defs.equal_visibility m.sm_visibility Aast_defs.Private
 
   let constructor_is_modified_when_added _ = false
+
+  let constructor_is_modified_when_modified ~old:_ ~new_:_ = false
 end
 
 let diff_constructor old_cls new_cls old_cstr new_cstr : member_change option =
diff --git a/hphp/hack/src/typing/xhp_attribute.ml b/hphp/hack/src/typing/xhp_attribute.ml
@@ -27,3 +27,8 @@ let is_required attr =
   | Some LateInit ->
     false
   | Some Required -> true
+
+let opt_is_required (attr : t option) : bool =
+  match attr with
+  | None -> false
+  | Some attr -> is_required attr
diff --git a/hphp/hack/src/typing/xhp_attribute.mli b/hphp/hack/src/typing/xhp_attribute.mli
@@ -22,3 +22,5 @@ val init : t
 val map_tag : t -> f:(tag option -> tag option) -> t
 
 val is_required : t -> bool
+
+val opt_is_required : t option -> bool
diff --git a/hphp/hack/test/fanout/make_required_attributes.php.incr_old_decl.exp b/hphp/hack/test/fanout/make_required_attributes.php.incr_old_decl.exp
@@ -4,3 +4,4 @@ No errors!
 No errors!
 === fanout ===
 0x52d77445a3eff51b
+Fun xhp_simple_attribute
