Convert needs concrete override check to warning (was an error)

Summary:
For ease of adoption, we're starting with using warnings instead of errors for `__NeedsConcrete` checking.

This change is particularly important because without it it's hard to migrate the code base gradually, as we'd have to do whole hierarchies in one go.

## Examples

Here are what the warnings look like with pretty-printing and context. These examples are also used in the test suite.

 {F1982437894}

We take care to include the method name for methods defined in traits:

{F1982437893}

context in fb-only link below

Reviewed By: geralt-encore

Differential Revision: D83658029

fbshipit-source-id: 2a824c63649433da866be5debbf80922fe517047

Author: mheiber

hphp/hack/src/errors/error_codes.ml

@@ -776,7 +776,6 @@ module Typing = struct
     | OptionalParameterNotSupported [@value 4484]
     | InvalidRecursiveType [@value 4485]
     | StaticCallOnTraitRequireThisAs [@value 4486]
-    | NeedsConcreteOverride [@value 4487]
     | StringToClassPointer [@value 4488]
     | SwitchNeedsDefault [@value 4489]
     | SimpliHackRunPrompt [@value 4493]
@@ -818,6 +817,7 @@ module Warning = struct
     | CallNeedsConcrete [@value 12024]
     | AbstractAccessViaStatic [@value 12025]
     | UninstantiableClassViaStatic [@value 12026]
+    | NeedsConcreteOverride [@value 12027]
   [@@deriving enum, ord, show { with_path = false }]
 end
 

hphp/hack/src/oxidized/gen/error_codes.rs

@@ -3,7 +3,7 @@
 // This source code is licensed under the MIT license found in the
 // LICENSE file in the "hack" directory of this source tree.
 //
-// @generated SignedSource<<64a9c66d309a4a2bfcd0a768ac221b26>>
+// @generated SignedSource<<2abf2d7b419a65fae9c5b1d22e059609>>
 //
 // To regenerate this file, run:
 //   buck run @fbcode//mode/dev-nosan-lg fbcode//hphp/hack/src:oxidized_regen
@@ -598,7 +598,6 @@ pub enum Typing {
     OptionalParameterNotSupported = 4484,
     InvalidRecursiveType = 4485,
     StaticCallOnTraitRequireThisAs = 4486,
-    NeedsConcreteOverride = 4487,
     StringToClassPointer = 4488,
     SwitchNeedsDefault = 4489,
     SimpliHackRunPrompt = 4493,
@@ -656,6 +655,7 @@ pub enum Warning {
     CallNeedsConcrete = 12024,
     AbstractAccessViaStatic = 12025,
     UninstantiableClassViaStatic = 12026,
+    NeedsConcreteOverride = 12027,
 }
 impl TrivialDrop for Warning {}
 arena_deserializer::impl_deserialize_in_arena!(Warning);

hphp/hack/src/typing/typing_error.ml

@@ -1889,10 +1889,6 @@ and Secondary : sig
         decl_pos: Pos_or_decl.t;
         most_similar: (string * Pos_or_decl.t) option;
       }
-    | Needs_concrete_override of {
-        pos: Pos_or_decl.t;
-        parent_pos: Pos_or_decl.t;
-      }
     | Higher_rank_tparam_escape of {
         tvar_pos: Pos_or_decl.t;
         pos_with_generic: Pos_or_decl.t;
@@ -2212,10 +2208,6 @@ end = struct
         decl_pos: Pos_or_decl.t;
         most_similar: (string * Pos_or_decl.t) option;
       }
-    | Needs_concrete_override of {
-        pos: Pos_or_decl.t;
-        parent_pos: Pos_or_decl.t;
-      }
     | Higher_rank_tparam_escape of {
         tvar_pos: Pos_or_decl.t;
         pos_with_generic: Pos_or_decl.t;

hphp/hack/src/typing/typing_error.mli

@@ -1728,10 +1728,6 @@ and Secondary : sig
         decl_pos: Pos_or_decl.t;
         most_similar: (string * Pos_or_decl.t) option;
       }
-    | Needs_concrete_override of {
-        pos: Pos_or_decl.t;
-        parent_pos: Pos_or_decl.t;
-      }
     | Higher_rank_tparam_escape of {
         tvar_pos: Pos_or_decl.t;
         pos_with_generic: Pos_or_decl.t;

hphp/hack/src/typing/typing_error_utils.ml

@@ -6593,18 +6593,6 @@ end = struct
     in
     create ~code:Error_code.EnumClassLabelUnknown ~reasons ()
 
-  let needs_concrete_override pos parent_pos =
-    let reasons =
-      lazy
-        [
-          ( pos,
-            "Cannot declare this method as __NeedsConcrete, since it overrides a non-__NeedsConcrete method"
-          );
-          (parent_pos, "Previously defined here");
-        ]
-    in
-    create ~code:Error_code.NeedsConcreteOverride ~reasons ()
-
   let higher_rank_tparam_escape
       tvar_pos pos_with_generic generic_reason generic_name =
     let reasons =
@@ -6841,8 +6829,6 @@ end = struct
       Eval_result.single (violated_refinement_constraint cstr)
     | Unknown_label { enum_name; decl_pos; most_similar } ->
       Eval_result.single (label_unknown enum_name decl_pos most_similar)
-    | Needs_concrete_override { pos; parent_pos } ->
-      Eval_result.single (needs_concrete_override pos parent_pos)
     | Higher_rank_tparam_escape
         { tvar_pos; pos_with_generic; generic_reason; generic_name } ->
       Eval_result.single

hphp/hack/src/typing/typing_extends.ml

@@ -737,22 +737,36 @@ let check_abstract_overrides_concrete
                    `property);
              })
 
-let check_needs_concrete_override env parent_class_elt class_elt on_error =
+(** Why this check: It would be unsound for a class that needs concrete (`<<__NeedsConcrete>>`) to override one that does not,
+  * since a __NeedsConcrete method imposes stricter requirements on its input (`static` must point to a concrete class) *)
+let check_needs_concrete_override
+    env ~parent_class_elt class_ class_elt ~class_pos ~member_name =
   if
     (not (get_ce_readonly_prop_or_needs_concrete parent_class_elt))
     && get_ce_readonly_prop_or_needs_concrete class_elt
   then
-    (* It would be unsound for a class that needs concrete (`<<__NeedsConcrete>>`) to override one that does not, because
-     * then `<<__NeedsConcrete>>` methods have stricter requirements (`static` must point to a concrete class). *)
-    Typing_error_utils.add_typing_error
-      ~env
-      Typing_error.(
-        apply_reasons ~on_error
-        @@ Secondary.Needs_concrete_override
-             {
-               pos = Lazy.force class_elt.ce_pos;
-               parent_pos = Lazy.force parent_class_elt.ce_pos;
-             })
+    let defined_in_same_class =
+      String.equal class_elt.ce_origin (Cls.name class_)
+    in
+    let (error_pos, method_name_for_method_defined_outside_class) =
+      match
+        Pos_or_decl.get_raw_pos_or_decl_reference (Lazy.force class_elt.ce_pos)
+      with
+      | `Raw pos when defined_in_same_class -> (pos, None)
+      | `Raw _
+      | `Decl_ref _ ->
+        (class_pos, Some member_name)
+    in
+    Typing_warning_utils.add
+      env
+      ( error_pos,
+        Typing_warning.Needs_concrete_override,
+        {
+          Typing_warning.Needs_concrete_override.pos =
+            Lazy.force class_elt.ce_pos;
+          method_name_for_method_defined_outside_class;
+          parent_pos = Lazy.force parent_class_elt.ce_pos;
+        } )
 
 let detect_multiple_concrete_defs
     (class_elt, class_) (parent_class_elt, parent_class) =
@@ -1002,6 +1016,7 @@ let check_override
     ~parent_class
     ~(parent_type : decl_ty)
     ~parent_pos
+    ~class_pos
     ~(class_elt : class_elt)
     ~parent_class_elt
     on_error =
@@ -1059,7 +1074,13 @@ let check_override
     parent_class_elt
     class_elt
     on_error;
-  check_needs_concrete_override env parent_class_elt class_elt on_error;
+  check_needs_concrete_override
+    env
+    ~parent_class_elt
+    class_
+    class_elt
+    ~class_pos
+    ~member_name;
 
   let (lazy pos) = class_elt.ce_pos in
 
@@ -1582,6 +1603,7 @@ let check_class_against_parent_class_elt
           ~parent_type
           ~parent_pos
           ~class_elt
+          ~class_pos
           (on_error (parent_pos, Cls.name parent_class)) )
   | None ->
     (* The only case when a member belongs to a parent but not the child is if the parent is an
@@ -1770,7 +1792,13 @@ let default_constructor_ce class_ =
 
 (* When an interface defines a constructor, we check that they are compatible *)
 let check_constructors
-    ~parent_pos env (parent_class, parent_type) class_ psubst on_error =
+    ~parent_pos
+    env
+    (parent_class, parent_type)
+    class_
+    ~class_pos
+    psubst
+    on_error =
   let parent_is_interface = Ast_defs.is_c_interface (Cls.kind parent_class) in
   let parent_is_consistent =
     constructor_is_consistent (snd (Cls.construct parent_class))
@@ -1808,6 +1836,7 @@ let check_constructors
             ~parent_type
             ~parent_pos
             ~parent_class_elt:parent_cstr
+            ~class_pos
             ~class_elt:cstr
             on_error
         else
@@ -2166,6 +2195,7 @@ let check_class_extends_parent_constructors
     env
     (parent_class : (Pos.t * string) * decl_ty * decl_ty list * Cls.t)
     class_
+    ~class_pos
     on_error =
   let (_, parent_type, parent_tparaml, parent_class) = parent_class in
   let psubst = Inst.make_subst (Cls.tparams parent_class) parent_tparaml in
@@ -2175,6 +2205,7 @@ let check_class_extends_parent_constructors
       env
       (parent_class, parent_type)
       class_
+      ~class_pos
       psubst
       on_error
   in
@@ -2935,6 +2966,7 @@ let check_implements_extends_uses
           ~parent_pos:(fst parent_name)
           env
           parent
+          ~class_pos:name_pos
           class_
           (on_error parent_name))
   in

hphp/hack/src/typing/typing_warning.ml

@@ -200,6 +200,21 @@ module Uninstantiable_class_via_static = struct
   }
 end
 
+module Needs_concrete_override = struct
+  type t = {
+    pos: Pos_or_decl.t;
+    parent_pos: Pos_or_decl.t;
+    method_name_for_method_defined_outside_class: string option;
+        (** `Some m` iff `m` is a trait method of a trait that is used by the class.
+     * In such cases, the location for the warning will be a class name, so we need
+     * to preserve the method name to give a meaningful error message.
+     * Example:   `class Child extends Parent { use Tr; }`
+     *                   ~~~~~
+     * where trait `Tr` defines the method with the incorrect override of `Parent::m`
+     *)
+  }
+end
+
 type (_, _) kind =
   | Sketchy_equality : (Sketchy_equality.t, warn) kind
   | Is_as_always : (Is_as_always.t, migrated) kind
@@ -220,5 +235,6 @@ type (_, _) kind =
   | Abstract_access_via_static : (Abstract_access_via_static.t, warn) kind
   | Uninstantiable_class_via_static
       : (Uninstantiable_class_via_static.t, warn) kind
+  | Needs_concrete_override : (Needs_concrete_override.t, warn) kind
 
 type ('x, 'a) t = Pos.t * ('x, 'a) kind * 'x

hphp/hack/src/typing/typing_warning_utils.ml

@@ -707,6 +707,45 @@ module Uninstantiable_class_via_static = struct
   let quickfixes _ = []
 end
 
+module Needs_concrete_override = struct
+  type t = Typing_warning.Needs_concrete_override.t
+
+  let code = Codes.NeedsConcreteOverride
+
+  let codes = [code]
+
+  let code _ = code
+
+  let claim
+      {
+        Typing_warning.Needs_concrete_override
+        .method_name_for_method_defined_outside_class;
+        _;
+      } =
+    let method_text =
+      match method_name_for_method_defined_outside_class with
+      | Some method_name -> Printf.sprintf "Method `%s`" method_name
+      | None -> "This method"
+    in
+    method_text
+    ^ " is declared as `__NeedsConcrete` but overrides a non-`__NeedsConcrete` method. Please add `__NeedsConcrete` to the overridden method or remove it from the current method. (The `__NeedsConcrete` attribute indicates that a method requires `static` to point to a concrete class)"
+
+  let reasons
+      {
+        Typing_warning.Needs_concrete_override.pos;
+        parent_pos;
+        method_name_for_method_defined_outside_class = _;
+      } =
+    [
+      ( pos,
+        "It is unsafe to declare this method as `__NeedsConcrete`, since it overrides a non-`__NeedsConcrete` method"
+      );
+      (parent_pos, "Previously defined here");
+    ]
+
+  let quickfixes _ = []
+end
+
 let module_of (type a x) (kind : (x, a) Typing_warning.kind) :
     (module Warning with type t = x) =
   match kind with
@@ -730,6 +769,7 @@ let module_of (type a x) (kind : (x, a) Typing_warning.kind) :
     (module Abstract_access_via_static)
   | Typing_warning.Uninstantiable_class_via_static ->
     (module Uninstantiable_class_via_static)
+  | Typing_warning.Needs_concrete_override -> (module Needs_concrete_override)
 
 let module_of_migrated
     (type x) (kind : (x, Typing_warning.migrated) Typing_warning.kind) :

hphp/hack/test/typecheck/needs_concrete/needs_concrete_override_1.php

@@ -0,0 +1,10 @@
+<?hh
+
+class Base {
+  public function foo(): void {}
+}
+
+class Child extends Base {
+  <<__NeedsConcrete>>
+  public function foo(): void {}
+}

hphp/hack/test/typecheck/needs_concrete/needs_concrete_override_1.php.exp

@@ -0,0 +1,6 @@
+WARN: File "needs_concrete_override_1.php", line 9, characters 19-21:
+This method is declared as `__NeedsConcrete` but overrides a non-`__NeedsConcrete` method. Please add `__NeedsConcrete` to the overridden method or remove it from the current method. (The `__NeedsConcrete` attribute indicates that a method requires `static` to point to a concrete class) (Warn[12027])
+  File "needs_concrete_override_1.php", line 9, characters 19-21:
+  It is unsafe to declare this method as `__NeedsConcrete`, since it overrides a non-`__NeedsConcrete` method
+  File "needs_concrete_override_1.php", line 4, characters 19-21:
+  Previously defined here