[nullsafe] Treat parameter in overrides of `equals(Object param)` as nullable.

Summary:
We already warn about lack of nullable annotations in `equals()`, and even have a specialized error message for that.

But lack of an annotation is not as severe as direct dereference: the
latter is a plain bug which is also a time bomb: it will lead to an NPE not immediately.

This is widespread enough to be reported separately.

Reviewed By: dulmarod

Differential Revision: D19719598

fbshipit-source-id: a535d43ea

infer/src/nullsafe/ErrorRenderingUtils.ml

@@ -22,12 +22,14 @@ let is_object_nullability_self_explanatory ~object_expression (object_origin : T
   | NullConst _ ->
       (* Expect either a local variable or null literal (latter case is trivial) *)
       String.equal object_expression "null"
-  | MethodParameter {mangled} ->
+  | MethodParameter (Normal {mangled}) ->
       (* Either local variable or literally parameter. In latter case, its nullability is
          self-explanatory because the user can quickly see the current method signature.
       *)
       let param_name = Mangled.to_string mangled in
       String.equal object_expression param_name
+  | MethodParameter ObjectEqualsOverride ->
+      false
   | Field {field_name} ->
       (* Either local variable or expression like `<smth>.field_name`. Latter case is trivial:
          the user can quickly go to field_name definition and see if its annotation. *)

infer/src/nullsafe/eradicate.ml

@@ -41,7 +41,9 @@ module MkCallback (Extension : ExtensionT) : CallBackT = struct
              has very special meaning and nullability limitations (it can never be null).
           *)
           TypeOrigin.This
-        else TypeOrigin.MethodParameter param_signature
+        else if PatternMatch.is_override_of_java_lang_object_equals curr_pname then
+          TypeOrigin.MethodParameter ObjectEqualsOverride
+        else TypeOrigin.MethodParameter (Normal param_signature)
       in
       let inferred_nullability = InferredNullability.create origin in
       TypeState.add pvar (param_signature.param_annotated_type.typ, inferred_nullability) typestate

infer/src/nullsafe/typeOrigin.ml

@@ -13,7 +13,7 @@ type t =
   | NullConst of Location.t  (** A null literal in the source *)
   | NonnullConst of Location.t  (** A constant (not equal to null) in the source. *)
   | Field of field_origin  (** A field access (result of expression `some_object.some_field`) *)
-  | MethodParameter of AnnotatedSignature.param_signature  (** A method's parameter *)
+  | MethodParameter of method_parameter_origin  (** A method's parameter *)
   | This (* `this` object. Can not be null, according to Java rules. *)
   | MethodCall of method_call_origin  (** A result of a method call *)
   | CallToGetKnownToContainsKey
@@ -34,6 +34,8 @@ type t =
   | Undef  (** Undefined value before initialization *)
 [@@deriving compare]
 
+and method_parameter_origin = Normal of AnnotatedSignature.param_signature | ObjectEqualsOverride
+
 and field_origin =
   { object_origin: t  (** field's object origin (object is before field access operator `.`) *)
   ; field_name: Fieldname.t
@@ -70,8 +72,11 @@ let get_nullability = function
       Nullability.StrictNonnull
   | Field {field_type= {nullability}} ->
       AnnotatedNullability.get_nullability nullability
-  | MethodParameter {param_annotated_type= {nullability}} ->
+  | MethodParameter (Normal {param_annotated_type= {nullability}}) ->
       AnnotatedNullability.get_nullability nullability
+  | MethodParameter ObjectEqualsOverride ->
+      (* `Object.equals(obj)` should expect to be called with null `obj` *)
+      Nullability.Nullable
   | MethodCall {annotated_signature= {ret= {ret_annotated_type= {nullability}}}} ->
       AnnotatedNullability.get_nullability nullability
 
@@ -83,9 +88,11 @@ let rec to_string = function
       "Const (nonnull)"
   | Field {object_origin; field_name} ->
       "Field " ^ Fieldname.to_string field_name ^ " (object: " ^ to_string object_origin ^ ")"
-  | MethodParameter {mangled; param_annotated_type= {nullability}} ->
+  | MethodParameter (Normal {mangled; param_annotated_type= {nullability}}) ->
       Format.asprintf "Param %s <%a>" (Mangled.to_string mangled) AnnotatedNullability.pp
         nullability
+  | MethodParameter ObjectEqualsOverride ->
+      "Param(ObjectEqualsOverride)"
   | This ->
       "this"
   | MethodCall {pname} ->
@@ -149,8 +156,10 @@ let get_description origin =
       Some ("null constant" ^ atline loc)
   | Field {field_name; access_loc} ->
       Some ("field " ^ Fieldname.get_field_name field_name ^ atline access_loc)
-  | MethodParameter {mangled} ->
+  | MethodParameter (Normal {mangled}) ->
       Some ("method parameter " ^ Mangled.to_string mangled)
+  | MethodParameter ObjectEqualsOverride ->
+      Some "Object.equals() should be able to accept `null`, according to the Java specification"
   | MethodCall {pname; call_loc; annotated_signature} ->
       Some (get_method_ret_description pname call_loc annotated_signature)
   (* These are origins of non-nullable expressions that are result of evaluating of some rvalue.

infer/src/nullsafe/typeOrigin.mli

@@ -11,7 +11,7 @@ type t =
   | NullConst of Location.t  (** A null literal in the source *)
   | NonnullConst of Location.t  (** A constant (not equal to null) in the source. *)
   | Field of field_origin  (** A field access (result of expression `some_object.some_field`) *)
-  | MethodParameter of AnnotatedSignature.param_signature  (** A method's parameter *)
+  | MethodParameter of method_parameter_origin  (** A method's parameter *)
   | This (* `this` object. Can not be null, according to Java rules. *)
   | MethodCall of method_call_origin  (** A result of a method call *)
   | CallToGetKnownToContainsKey
@@ -32,6 +32,8 @@ type t =
   | Undef  (** Undefined value before initialization *)
 [@@deriving compare]
 
+and method_parameter_origin = Normal of AnnotatedSignature.param_signature | ObjectEqualsOverride
+
 and field_origin =
   { object_origin: t  (** field's object origin (object is before field access operator `.`) *)
   ; field_name: Fieldname.t

infer/tests/codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java

@@ -172,11 +172,15 @@ class ExtendsExternalLibrary extends SomeExternalClass {
   }
 }
 
-// Check that we have a special error message for this method
+// Check that 1) we have a special error message for lack of annotation in this method and 2) treat
+// `x` as implicitly nullable
 class JavaLangEquals {
   @Override
   public boolean equals(Object x) {
-    return false;
+    // BAD: x can not be directly dereferenced without null comparison:
+    // it is implicitly nullable because Java requires `x.equals(null)` to work correctly.
+    // It is a common enough case to make the nullsafe support this specifically.
+    return x.toString() == "JavaLangEquals";
   }
 }
 

infer/tests/codetoanalyze/java/nullsafe-default/issues.exp

@@ -73,6 +73,7 @@ codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoa
 codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.ArgNullToValForInterfaceInAnotherFileBAD.implementInAnotherFile(java.lang.String):java.lang.String, 0, ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION, no_bucket, WARNING, [First parameter `s` of method `ArgNullToValForInterfaceInAnotherFileBAD.implementInAnotherFile(...)` is missing `@Nullable` declaration when overriding `InconsistentSubclassAnnotationInterface.implementInAnotherFile(...)`. The parent method declared it can handle `null` for this param, so the child should also declare that.]
 codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.ExtendsExternalLibrary.externalMethod2(java.lang.Object):void, 0, ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION, no_bucket, WARNING, [First parameter `object` of method `ExtendsExternalLibrary.externalMethod2(...)` is missing `@Nullable` declaration when overriding `SomeExternalClass.externalMethod2(...)`. The parent method declared it can handle `null` for this param, so the child should also declare that.]
 codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.JavaLangEquals.equals(java.lang.Object):boolean, 0, ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION, no_bucket, WARNING, [Parameter `x` is missing `@Nullable` declaration: according to the Java Specification, for any object `x` call `x.equals(null)` should properly return false.]
+codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.JavaLangEquals.equals(java.lang.Object):boolean, 0, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`x` is nullable and is not locally checked for null when calling `toString()`: Object.equals() should be able to accept `null`, according to the Java specification]
 codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.NullableConcreteGetterBAD.get():java.lang.String, 0, ERADICATE_INCONSISTENT_SUBCLASS_RETURN_ANNOTATION, no_bucket, WARNING, [Child method `NullableConcreteGetterBAD.get()` is not substitution-compatible with its parent: the return type is declared as nullable, but parent method `NonNullableInterfaceGetterOK.get()` is missing `@Nullable` declaration. Either mark the parent as `@Nullable` or ensure the child does not return `null`.]
 codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.OverloadExistingIncorrectBAD.overload(java.lang.String,java.lang.String):java.lang.String, 0, ERADICATE_INCONSISTENT_SUBCLASS_RETURN_ANNOTATION, no_bucket, WARNING, [Child method `OverloadExistingIncorrectBAD.overload(...)` is not substitution-compatible with its parent: the return type is declared as nullable, but parent method `Overloads.overload(...)` is missing `@Nullable` declaration. Either mark the parent as `@Nullable` or ensure the child does not return `null`.]
 codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.ReturnValToNullBAD.valBoth(java.lang.String):java.lang.String, 0, ERADICATE_INCONSISTENT_SUBCLASS_RETURN_ANNOTATION, no_bucket, WARNING, [Child method `ReturnValToNullBAD.valBoth(...)` is not substitution-compatible with its parent: the return type is declared as nullable, but parent method `VariousMethods.valBoth(...)` is missing `@Nullable` declaration. Either mark the parent as `@Nullable` or ensure the child does not return `null`.]