[nullsafe] Model Object.equals() and descentands as FalseOnNull

Summary:
Since we fixed a bug in implementation of FalseOnNull (see stack below),
we can finally ship this change.

Side note: this change is essential for the follow up diff (which adds extra check
for user-defined implementations of equal()), without it the follow
up change would introduce a lot of false positives.

Reviewed By: ngorogiannis

Differential Revision: D19771057

fbshipit-source-id: 7d7cf1ef7

infer/src/absint/PatternMatch.ml

@@ -460,3 +460,15 @@ let rec find_superclasses_with_attributes check tenv tname =
       if check struct_typ.annots then tname :: result_from_supers else result_from_supers
   | _ ->
       []
+
+
+let is_override_of_java_lang_object_equals curr_pname =
+  let is_only_param_of_object_type = function
+    | [Procname.Parameter.JavaParameter param_type]
+      when Typ.Name.Java.Split.equal param_type Typ.Name.Java.Split.java_lang_object ->
+        true
+    | _ ->
+        false
+  in
+  String.equal (Procname.get_method curr_pname) "equals"
+  && is_only_param_of_object_type (Procname.get_parameters curr_pname)

infer/src/absint/PatternMatch.mli

@@ -156,3 +156,7 @@ val find_superclasses_with_attributes :
 val has_same_signature : Procname.t -> (Procname.t -> bool) Staged.t
 (** For a given [procname] checks if the method has the same method name, number, order and types of
     parameters.) *)
+
+val is_override_of_java_lang_object_equals : Procname.t -> bool
+(** Whether the method is an override of `java.lang.Object.equals(Object)` or
+    `java.lang.Object.equals(Object)` itself *)

infer/src/nullsafe/models.ml

@@ -132,6 +132,13 @@ let is_noreturn proc_name = table_has_procedure noreturn_table proc_name
 (** Check if the procedure returns true on null. *)
 let is_true_on_null proc_name = table_has_procedure true_on_null_table proc_name
 
+(** Check if the procedure returns false on null. *)
+let is_false_on_null proc_name =
+  (* The only usecase for now - consider all overrides of `Object.equals()` correctly
+     implementing the Java specification contract (returning false on null). *)
+  PatternMatch.is_override_of_java_lang_object_equals proc_name
+
+
 (** Check if the procedure is Map.containsKey(). *)
 let is_containsKey proc_name = table_has_procedure containsKey_table proc_name
 

infer/src/nullsafe/typeCheck.ml

@@ -15,27 +15,24 @@ module DExp = DecompiledExp
 module ComplexExpressions = struct
   let procname_instanceof = Procname.equal BuiltinDecl.__instanceof
 
-  let procname_is_false_on_null tenv pn =
+  let is_annotated_with predicate tenv procname =
-    match PatternMatch.lookup_attributes tenv pn with
+    match PatternMatch.lookup_attributes tenv procname with
     | Some proc_attributes ->
         let annotated_signature = Models.get_modelled_annotated_signature tenv proc_attributes in
         let AnnotatedSignature.{ret_annotation_deprecated} = annotated_signature.ret in
-        Annotations.ia_is_false_on_null ret_annotation_deprecated
+        predicate ret_annotation_deprecated
     | None ->
         false
 
 
-  let procname_is_true_on_null tenv pn =
+  let procname_is_false_on_null tenv procname =
-    let annotated_true_on_null () =
+    is_annotated_with Annotations.ia_is_false_on_null tenv procname
-      match PatternMatch.lookup_attributes tenv pn with
+    || Models.is_false_on_null procname
-      | Some proc_attributes ->
+
-          let annotated_signature = Models.get_modelled_annotated_signature tenv proc_attributes in
+
-          let AnnotatedSignature.{ret_annotation_deprecated} = annotated_signature.ret in
+  let procname_is_true_on_null tenv procname =
-          Annotations.ia_is_true_on_null ret_annotation_deprecated
+    is_annotated_with Annotations.ia_is_true_on_null tenv procname
-      | None ->
+    || Models.is_true_on_null procname
-          false
-    in
-    Models.is_true_on_null pn || annotated_true_on_null ()
 
 
   let procname_containsKey = Models.is_containsKey

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

@@ -224,6 +224,51 @@ public class TrueFalseOnNull {
     }
   }
 
+  // nullsafe should assume Object.equals() and all overrides return false on `null` argument.
+  static class TestEqualsIsFalseOnNull {
+    // An example of class that overrides equals().
+    static class SomeObject {
+      private int mContent;
+
+      SomeObject(int content) {
+        mContent = content;
+      }
+
+      @Override
+      // No nullsafe warnings are expected
+      public boolean equals(@Nullable Object src) {
+        if (!super.equals(src)) {
+          return false;
+        }
+        // at this point src should be a non-nullable: super.equals() would return false otherwise.
+        src.toString(); // should be OK to dereference now
+        if (!(src instanceof SomeObject)) {
+          return false;
+        }
+        SomeObject asSomeObject = (SomeObject) src;
+        return mContent == asSomeObject.mContent;
+      }
+    }
+
+    private void testObjectEqualsIsFalseOnNull(Object x, @Nullable Object y) {
+      if (!x.equals(y)) {
+        return;
+      }
+
+      // OK to dereference
+      y.toString();
+    }
+
+    private void testOverrideEqualsIsFalseOnNull(SomeObject x, @Nullable Object y) {
+      if (!x.equals(y)) {
+        return;
+      }
+
+      // OK to dereference even that SomeObject.equals() is not annotated as @FalseOnNull
+      y.toString();
+    }
+  }
+
   // this is a common enough pattern to be tested separately
   class EarlyReturn {