From ae907d80cf0b174e5c15d635a8adbed534213f5e Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Fri, 7 Feb 2020 05:11:32 -0800 Subject: [PATCH] [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 | 12 +++++ infer/src/absint/PatternMatch.mli | 4 ++ infer/src/nullsafe/models.ml | 7 +++ infer/src/nullsafe/typeCheck.ml | 25 +++++------ .../nullsafe-default/TrueFalseOnNull.java | 45 +++++++++++++++++++ 5 files changed, 79 insertions(+), 14 deletions(-) diff --git a/infer/src/absint/PatternMatch.ml b/infer/src/absint/PatternMatch.ml index 6a6c1ce69..f8035ea33 100644 --- a/infer/src/absint/PatternMatch.ml +++ b/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) diff --git a/infer/src/absint/PatternMatch.mli b/infer/src/absint/PatternMatch.mli index 5d871f81c..024aaeb00 100644 --- a/infer/src/absint/PatternMatch.mli +++ b/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 *) diff --git a/infer/src/nullsafe/models.ml b/infer/src/nullsafe/models.ml index 3e7b954ce..c7c649588 100644 --- a/infer/src/nullsafe/models.ml +++ b/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 diff --git a/infer/src/nullsafe/typeCheck.ml b/infer/src/nullsafe/typeCheck.ml index 40a5f5909..e19fe1d46 100644 --- a/infer/src/nullsafe/typeCheck.ml +++ b/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 = - match PatternMatch.lookup_attributes tenv pn with + let is_annotated_with predicate tenv procname = + 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 annotated_true_on_null () = - match PatternMatch.lookup_attributes tenv pn 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_true_on_null ret_annotation_deprecated - | None -> - false - in - Models.is_true_on_null pn || annotated_true_on_null () + let procname_is_false_on_null tenv procname = + is_annotated_with Annotations.ia_is_false_on_null tenv procname + || Models.is_false_on_null procname + + + let procname_is_true_on_null tenv procname = + is_annotated_with Annotations.ia_is_true_on_null tenv procname + || Models.is_true_on_null procname let procname_containsKey = Models.is_containsKey diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/TrueFalseOnNull.java b/infer/tests/codetoanalyze/java/nullsafe-default/TrueFalseOnNull.java index 77828901f..68a91e31b 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/TrueFalseOnNull.java +++ b/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 {