From 434cfbfb1573c83b9430d8254410a427a8232e68 Mon Sep 17 00:00:00 2001 From: Cristiano Calcagno Date: Fri, 17 Mar 2017 06:17:19 -0700 Subject: [PATCH] [eradicate] Add support for new annotation @PropagatesNullable Summary: One limitation of Eradicate is that certain nullability patterns are not expressible using simply the `Nullable` annotation. One such pattern is using the knowledge that a function returns null when passed null, but returns an object otherwise. The annotation `PropagatesNullable` is a variant of `Nullable` applied to parameters when their value propagates to the return value. A method annotated ``` B m(PropagatesNullable A x) { return x == null ? x : B(x); } ``` indicates that `m` returns null if `x` is null, or an object of class `B` if the argument is not null. Examples with multiple parameters are in the test cases. This diff builds some infrastructure for annotation transformers: the example above represents the identity function on nullability annotations. Reviewed By: jvillard Differential Revision: D4705938 fbshipit-source-id: 9f6194e --- .../infer/annotation/PropagatesNullable.java | 23 ++ infer/src/IR/Annot.re | 4 +- infer/src/IR/Annot.rei | 4 +- infer/src/checkers/annotations.ml | 7 +- infer/src/checkers/annotations.mli | 1 + infer/src/eradicate/eradicate.ml | 10 +- infer/src/eradicate/eradicateChecks.ml | 107 +++---- infer/src/eradicate/typeCheck.ml | 263 +++++++++++------- infer/src/java/jAnnotation.ml | 7 +- .../java/eradicate/CustomAnnotations.java | 129 +++++++++ .../java/eradicate/NullMethodCall.java | 54 ---- .../java/eradicate/ParameterNotNullable.java | 13 + .../codetoanalyze/java/eradicate/issues.exp | 23 +- 13 files changed, 415 insertions(+), 230 deletions(-) create mode 100644 infer/annotations/src/main/java/com/facebook/infer/annotation/PropagatesNullable.java create mode 100644 infer/tests/codetoanalyze/java/eradicate/CustomAnnotations.java diff --git a/infer/annotations/src/main/java/com/facebook/infer/annotation/PropagatesNullable.java b/infer/annotations/src/main/java/com/facebook/infer/annotation/PropagatesNullable.java new file mode 100644 index 000000000..bbe99dd53 --- /dev/null +++ b/infer/annotations/src/main/java/com/facebook/infer/annotation/PropagatesNullable.java @@ -0,0 +1,23 @@ +/* + * Copyright (c) 2017 - present Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +package com.facebook.infer.annotation; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +/** + * Annotation to indicate that when the parameter is null, the result is also null. + */ +@Retention(RetentionPolicy.CLASS) +@Target({ElementType.PARAMETER}) +public @interface PropagatesNullable { +} diff --git a/infer/src/IR/Annot.re b/infer/src/IR/Annot.re index 03e51344d..67ea58061 100644 --- a/infer/src/IR/Annot.re +++ b/infer/src/IR/Annot.re @@ -15,11 +15,13 @@ let module L = Logging; let module F = Format; +type parameters = list string [@@deriving compare]; + /** Type to represent one @Annotation. */ type t = { class_name: string, /** name of the annotation */ - parameters: list string /** currently only one string parameter */ + parameters: parameters /** currently only one string parameter */ } [@@deriving compare]; diff --git a/infer/src/IR/Annot.rei b/infer/src/IR/Annot.rei index 343c2d45c..e16baa372 100644 --- a/infer/src/IR/Annot.rei +++ b/infer/src/IR/Annot.rei @@ -13,11 +13,13 @@ open! IStd; /** The Smallfoot Intermediate Language: Annotations */ let module F = Format; +type parameters = list string; + /** Type to represent one @Annotation. */ type t = { class_name: string, /** name of the annotation */ - parameters: list string /** currently only one string parameter */ + parameters: parameters /** currently only one string parameter */ } [@@deriving compare]; diff --git a/infer/src/checkers/annotations.ml b/infer/src/checkers/annotations.ml index 0dfd3e5a3..62bd97cd6 100644 --- a/infer/src/checkers/annotations.ml +++ b/infer/src/checkers/annotations.ml @@ -49,6 +49,7 @@ let performance_critical = "PerformanceCritical" let present = "Present" let privacy_source = "PrivacySource" let privacy_sink = "PrivacySink" +let propagates_nullable = "PropagatesNullable" let strict = "com.facebook.infer.annotation.Strict" let returns_ownership = "ReturnsOwnership" let suppress_lint = "SuppressLint" @@ -120,8 +121,12 @@ let struct_typ_has_annot (struct_typ : Typ.Struct.t) predicate = let ia_is_not_thread_safe ia = ia_ends_with ia not_thread_safe +let ia_is_propagates_nullable ia = + ia_ends_with ia propagates_nullable + let ia_is_nullable ia = - ia_ends_with ia nullable + ia_ends_with ia nullable || + ia_is_propagates_nullable ia let ia_is_present ia = ia_ends_with ia present diff --git a/infer/src/checkers/annotations.mli b/infer/src/checkers/annotations.mli index 5b2506d55..791ffb0da 100644 --- a/infer/src/checkers/annotations.mli +++ b/infer/src/checkers/annotations.mli @@ -61,6 +61,7 @@ val ia_is_verify : Annot.Item.t -> bool val ia_is_expensive : Annot.Item.t -> bool val ia_is_functional : Annot.Item.t -> bool val ia_is_performance_critical : Annot.Item.t -> bool +val ia_is_propagates_nullable : Annot.Item.t -> bool val ia_is_no_allocation : Annot.Item.t -> bool val ia_is_ignore_allocations : Annot.Item.t -> bool val ia_is_inject : Annot.Item.t -> bool diff --git a/infer/src/eradicate/eradicate.ml b/infer/src/eradicate/eradicate.ml index 5095b5ed1..b29f7383c 100644 --- a/infer/src/eradicate/eradicate.ml +++ b/infer/src/eradicate/eradicate.ml @@ -84,7 +84,9 @@ struct annotated_signature.AnnotatedSignature.params in (* Check the nullable flag computed for the return value and report inconsistencies. *) - let check_return find_canonical_duplicate exit_node final_typestate ret_ia ret_type loc : unit = + let check_return + find_canonical_duplicate exit_node final_typestate annotated_signature loc : unit = + let _, ret_type = annotated_signature.AnnotatedSignature.ret in let ret_pvar = Procdesc.get_ret_var curr_pdesc in let ret_range = TypeState.lookup_pvar ret_pvar final_typestate in let typ_found_opt = match ret_range with @@ -101,7 +103,7 @@ struct if checks.TypeCheck.eradicate then EradicateChecks.check_return_annotation tenv find_canonical_duplicate curr_pdesc ret_range - ret_ia ret_implicitly_nullable loc in + annotated_signature ret_implicitly_nullable loc in let do_before_dataflow initial_typestate = if Config.eradicate_verbose then @@ -110,8 +112,8 @@ struct let do_after_dataflow find_canonical_duplicate final_typestate = let exit_node = Procdesc.get_exit_node curr_pdesc in - let ia, ret_type = annotated_signature.AnnotatedSignature.ret in - check_return find_canonical_duplicate exit_node final_typestate ia ret_type proc_loc in + check_return + find_canonical_duplicate exit_node final_typestate annotated_signature proc_loc in let module DFTypeCheck = MakeDF(struct type t = Extension.extension TypeState.t diff --git a/infer/src/eradicate/eradicateChecks.ml b/infer/src/eradicate/eradicateChecks.ml index 45a58a74c..0cb852a56 100644 --- a/infer/src/eradicate/eradicateChecks.ml +++ b/infer/src/eradicate/eradicateChecks.ml @@ -347,11 +347,18 @@ let spec_make_return_nullable curr_pname = (** Check the annotations when returning from a method. *) let check_return_annotation tenv find_canonical_duplicate curr_pdesc ret_range - ret_ia ret_implicitly_nullable loc : unit = + (annotated_signature : AnnotatedSignature.t) ret_implicitly_nullable loc : unit = + let ret_ia, _ = annotated_signature.ret in let curr_pname = Procdesc.get_proc_name curr_pdesc in - let ret_annotated_nullable = Annotations.ia_is_nullable ret_ia in - let ret_annotated_present = Annotations.ia_is_present ret_ia in - let ret_annotated_nonnull = Annotations.ia_is_nonnull ret_ia in + let ret_annotated_nullable = + Annotations.ia_is_nullable ret_ia || + List.exists + ~f:(fun (_, ia, _) -> Annotations.ia_is_propagates_nullable ia) + annotated_signature.params in + let ret_annotated_present = + Annotations.ia_is_present ret_ia in + let ret_annotated_nonnull = + Annotations.ia_is_nonnull ret_ia in match ret_range with | Some (_, final_ta, _) -> let final_nullable = TypeAnnotation.get_value AnnotatedSignature.Nullable final_ta in @@ -442,62 +449,62 @@ let check_call_receiver tenv end | [] -> () -type param = { +type resolved_param = { + num : int; formal : Mangled.t * TypeAnnotation.t * Typ.t; actual : Exp.t * TypeAnnotation.t; + propagates_nullable : bool; } (** Check the parameters of a call. *) let check_call_parameters tenv find_canonical_duplicate curr_pdesc node callee_attributes - params loc instr_ref : unit = + resolved_params loc instr_ref : unit = let callee_pname = callee_attributes.ProcAttributes.proc_name in - let tot_param_num = List.length params in + let tot_param_num = List.length resolved_params in - let check i = function - | {formal = (s1, ta1, t1); actual = (orig_e2, ta2)} -> - let param_num = i + 1 in + let check + {num = param_num; formal = (s1, ta1, t1); actual = (orig_e2, ta2)} = + let report ann = + let description = + match explain_expr tenv node orig_e2 with + | Some descr -> descr + | None -> "formal parameter " ^ (Mangled.to_string s1) in + let origin_descr = TypeAnnotation.descr_origin tenv ta2 in - let report ann = - let description = - match explain_expr tenv node orig_e2 with - | Some descr -> descr - | None -> "formal parameter " ^ (Mangled.to_string s1) in - let origin_descr = TypeAnnotation.descr_origin tenv ta2 in - - let callee_loc = callee_attributes.ProcAttributes.loc in - report_error tenv - find_canonical_duplicate - (TypeErr.Parameter_annotation_inconsistent ( - ann, - description, - param_num, - callee_pname, - callee_loc, - origin_descr)) - (Some instr_ref) - loc curr_pdesc in - - let check_ann ann = - let b1 = TypeAnnotation.get_value ann ta1 in - let b2 = TypeAnnotation.get_value ann ta2 in - match ann, b1, b2 with - | AnnotatedSignature.Nullable, false, true -> - report ann; - if Models.Inference.enabled then - Models.Inference.proc_add_parameter_nullable callee_pname param_num tot_param_num - | AnnotatedSignature.Present, true, false -> - report ann - | _ -> - () in - - if PatternMatch.type_is_class t1 - then begin - check_ann AnnotatedSignature.Nullable; - if Config.eradicate_optional_present - then check_ann AnnotatedSignature.Present; - end in + let callee_loc = callee_attributes.ProcAttributes.loc in + report_error tenv + find_canonical_duplicate + (TypeErr.Parameter_annotation_inconsistent ( + ann, + description, + param_num, + callee_pname, + callee_loc, + origin_descr)) + (Some instr_ref) + loc curr_pdesc in + + let check_ann ann = + let b1 = TypeAnnotation.get_value ann ta1 in + let b2 = TypeAnnotation.get_value ann ta2 in + match ann, b1, b2 with + | AnnotatedSignature.Nullable, false, true -> + report ann; + if Models.Inference.enabled then + Models.Inference.proc_add_parameter_nullable callee_pname param_num tot_param_num + | AnnotatedSignature.Present, true, false -> + report ann + | _ -> + () in + + if PatternMatch.type_is_class t1 + then begin + check_ann AnnotatedSignature.Nullable; + if Config.eradicate_optional_present + then check_ann AnnotatedSignature.Present; + end in let should_check_parameters = if check_library_calls then true else @@ -507,7 +514,7 @@ let check_call_parameters tenv if should_check_parameters then (* left to right to avoid guessing the different lengths *) - List.iteri ~f:check params + List.iter ~f:check resolved_params (** Checks if the annotations are consistent with the inherited class or with the implemented interfaces *) diff --git a/infer/src/eradicate/typeCheck.ml b/infer/src/eradicate/typeCheck.ml index aaccdb630..872396945 100644 --- a/infer/src/eradicate/typeCheck.ml +++ b/infer/src/eradicate/typeCheck.ml @@ -621,20 +621,11 @@ let typecheck_instr let is_anonymous_inner_class_constructor = Typ.Procname.java_is_anonymous_inner_class_constructor callee_pname in - let do_return loc' typestate' = + let do_return (ret_ta, ret_typ) loc' typestate' = let mk_return_range () = - let (ia, ret_typ) = annotated_signature.AnnotatedSignature.ret in - let is_library = Specs.proc_is_library callee_attributes in - let origin = TypeOrigin.Proc - { - TypeOrigin.pname = callee_pname; - loc = loc'; - annotated_signature; - is_library; - } in ( ret_typ, - TypeAnnotation.from_item_annotation ia origin, + ret_ta, [loc'] ) in @@ -802,101 +793,161 @@ let typecheck_instr | _ -> typestate' in - let typestate2 = - let prepare_params sig_params call_params = - let rec f acc sparams cparams = match sparams, cparams with - | (s1, ia1, t1) :: sparams', ((orig_e2, e2), t2) :: cparams' -> - let param_is_this = String.equal (Mangled.to_string s1) "this" in - let acc' = - if param_is_this - then acc - else begin - let ta1 = TypeAnnotation.from_item_annotation ia1 (TypeOrigin.Formal s1) in - let (_, ta2, _) = - typecheck_expr - find_canonical_duplicate calls_this checks - tenv node instr_ref curr_pdesc typestate e2 - (t2, - TypeAnnotation.const AnnotatedSignature.Nullable false TypeOrigin.ONone, - []) - loc in - - EradicateChecks.{ - formal = (s1, ta1, t1); - actual = (orig_e2, ta2)} - :: acc - end in - f acc' sparams' cparams' - | _ -> - acc in - f [] (List.rev sig_params) (List.rev call_params) in - - if not is_anonymous_inner_class_constructor then - begin - if Config.eradicate_debug then - begin - let unique_id = Typ.Procname.to_unique_id callee_pname in - let classification = - EradicateChecks.classify_procedure callee_attributes in - L.stdout " %s unique id: %s@." classification unique_id - end; - if cflags.CallFlags.cf_virtual && checks.eradicate then - EradicateChecks.check_call_receiver tenv - find_canonical_duplicate - curr_pdesc - node - typestate1 - call_params - callee_pname - instr_ref - loc - (typecheck_expr find_canonical_duplicate calls_this checks); - if checks.eradicate then - EradicateChecks.check_call_parameters tenv - find_canonical_duplicate - curr_pdesc - node - callee_attributes - (prepare_params signature_params call_params) - loc - instr_ref; - let typestate2 = - if checks.check_extension then - let etl' = List.map ~f:(fun ((_, e), t) -> (e, t)) call_params in - let extension = TypeState.get_extension typestate1 in - let extension' = - ext.TypeState.check_instr - tenv get_proc_desc curr_pname curr_pdesc extension instr etl' in - TypeState.set_extension typestate1 extension' - else typestate1 in - let has_method pn name = match pn with - | Typ.Procname.Java pn_java -> - String.equal (Typ.Procname.java_get_method pn_java) name - | _ -> - false in - if Models.is_check_not_null callee_pname then - do_preconditions_check_not_null - (Models.get_check_not_null_parameter callee_pname) - ~is_vararg:false - typestate2 - else - if has_method callee_pname "checkNotNull" - && Typ.Procname.java_is_vararg callee_pname - then - let last_parameter = List.length call_params in - do_preconditions_check_not_null - last_parameter - ~is_vararg:true - typestate2 - else if Models.is_check_state callee_pname || - Models.is_check_argument callee_pname then - do_preconditions_check_state typestate2 - else if Models.is_mapPut callee_pname then - do_map_put typestate2 - else typestate2 - end - else typestate1 in - do_return loc typestate2 + let typestate_after_call, resolved_ret = + let resolve_param i (sparam, cparam) = + let (s1, ia1, t1) = sparam in + let ((orig_e2, e2), t2) = cparam in + let ta1 = TypeAnnotation.from_item_annotation ia1 (TypeOrigin.Formal s1) in + let (_, ta2, _) = + typecheck_expr + find_canonical_duplicate calls_this checks + tenv node instr_ref curr_pdesc typestate e2 + (t2, + TypeAnnotation.const AnnotatedSignature.Nullable false TypeOrigin.ONone, + []) + loc in + let formal = (s1, ta1, t1) in + let actual = (orig_e2, ta2) in + let num = i+1 in + let formal_is_propagates_nullable = Annotations.ia_is_propagates_nullable ia1 in + let actual_is_nullable = TypeAnnotation.get_value AnnotatedSignature.Nullable ta2 in + let propagates_nullable = formal_is_propagates_nullable && actual_is_nullable in + EradicateChecks.{num; formal; actual; propagates_nullable} in + + (* Apply a function that operates on annotations *) + let apply_annotation_transformer + resolved_ret (resolved_params : EradicateChecks.resolved_param list) = + let rec handle_params resolved_ret params = + match (params : EradicateChecks.resolved_param list) with + | param :: params' + when param.propagates_nullable -> + let (_, actual_ta) = param.actual in + let resolved_ret' = + let (ret_ta, ret_typ) = resolved_ret in + let ret_ta' = + let actual_nullable = + TypeAnnotation.get_value AnnotatedSignature.Nullable actual_ta in + let old_nullable = + TypeAnnotation.get_value AnnotatedSignature.Nullable ret_ta in + let new_nullable = + old_nullable || actual_nullable in + TypeAnnotation.set_value + AnnotatedSignature.Nullable + new_nullable + ret_ta in + (ret_ta', ret_typ) in + handle_params resolved_ret' params' + | _ :: params' -> + handle_params resolved_ret params' + | [] -> + resolved_ret in + handle_params resolved_ret resolved_params in + + let resolved_ret_ = + let (ret_ia, ret_typ) = annotated_signature.AnnotatedSignature.ret in + let is_library = Specs.proc_is_library callee_attributes in + let origin = TypeOrigin.Proc + { + TypeOrigin.pname = callee_pname; + loc; + annotated_signature; + is_library; + } in + let ret_ta = TypeAnnotation.from_item_annotation ret_ia origin in + (ret_ta, ret_typ) in + + let sig_len = List.length signature_params in + let call_len = List.length call_params in + let min_len = min sig_len call_len in + let slice l = + let len = List.length l in + if len > min_len + then List.slice l (len - min_len) 0 + else l in + let sig_slice = slice signature_params in + let call_slice = slice call_params in + let sig_call_params = + List.filter + ~f:(fun (sparam, _) -> + let (s, _, _) = sparam in + let param_is_this = + String.equal (Mangled.to_string s) "this" || + String.is_prefix ~prefix:"this$" (Mangled.to_string s) in + not param_is_this) + (List.zip_exn sig_slice call_slice) in + + let resolved_params = List.mapi ~f:resolve_param sig_call_params in + let resolved_ret = + apply_annotation_transformer resolved_ret_ resolved_params in + + let typestate_after_call = + if not is_anonymous_inner_class_constructor then + begin + if Config.eradicate_debug then + begin + let unique_id = Typ.Procname.to_unique_id callee_pname in + let classification = + EradicateChecks.classify_procedure callee_attributes in + L.stdout " %s unique id: %s@." classification unique_id + end; + if cflags.CallFlags.cf_virtual && checks.eradicate then + EradicateChecks.check_call_receiver tenv + find_canonical_duplicate + curr_pdesc + node + typestate1 + call_params + callee_pname + instr_ref + loc + (typecheck_expr find_canonical_duplicate calls_this checks); + if checks.eradicate then + EradicateChecks.check_call_parameters tenv + find_canonical_duplicate + curr_pdesc + node + callee_attributes + resolved_params + loc + instr_ref; + let typestate2 = + if checks.check_extension then + let etl' = List.map ~f:(fun ((_, e), t) -> (e, t)) call_params in + let extension = TypeState.get_extension typestate1 in + let extension' = + ext.TypeState.check_instr + tenv get_proc_desc curr_pname curr_pdesc extension instr etl' in + TypeState.set_extension typestate1 extension' + else typestate1 in + let has_method pn name = match pn with + | Typ.Procname.Java pn_java -> + String.equal (Typ.Procname.java_get_method pn_java) name + | _ -> + false in + if Models.is_check_not_null callee_pname then + do_preconditions_check_not_null + (Models.get_check_not_null_parameter callee_pname) + ~is_vararg:false + typestate2 + else + if has_method callee_pname "checkNotNull" + && Typ.Procname.java_is_vararg callee_pname + then + let last_parameter = List.length call_params in + do_preconditions_check_not_null + last_parameter + ~is_vararg:true + typestate2 + else if Models.is_check_state callee_pname || + Models.is_check_argument callee_pname then + do_preconditions_check_state typestate2 + else if Models.is_mapPut callee_pname then + do_map_put typestate2 + else typestate2 + end + else typestate1 in + typestate_after_call, resolved_ret in + do_return resolved_ret loc typestate_after_call | Sil.Call _ -> typestate | Sil.Prune (cond, loc, true_branch, _) -> diff --git a/infer/src/java/jAnnotation.ml b/infer/src/java/jAnnotation.ml index 8a5de1b81..0b4535a5a 100644 --- a/infer/src/java/jAnnotation.ml +++ b/infer/src/java/jAnnotation.ml @@ -16,11 +16,10 @@ open Javalib_pack (** Translate an annotation. *) let translate a : Annot.t = let class_name = JBasics.cn_name a.JBasics.kind in - let translate_value_pair acc (_, value) = + let rec translate_value_pair acc (x, value) = match value with - | JBasics.EVArray [JBasics.EVCstString s] -> - (* TODO (t16352423) see if this case is used *) - s :: acc + | JBasics.EVArray (JBasics.EVCstString s :: l) -> + translate_value_pair (s::acc) (x, JBasics.EVArray l) | JBasics.EVCstString s -> s :: acc | JBasics.EVCstBoolean 0 -> diff --git a/infer/tests/codetoanalyze/java/eradicate/CustomAnnotations.java b/infer/tests/codetoanalyze/java/eradicate/CustomAnnotations.java new file mode 100644 index 000000000..681f103bf --- /dev/null +++ b/infer/tests/codetoanalyze/java/eradicate/CustomAnnotations.java @@ -0,0 +1,129 @@ +/* + * Copyright (c) 2013 - present Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +package codetoanalyze.java.eradicate; + +import android.text.TextUtils; + +import javax.annotation.Nullable; +import com.facebook.infer.annotation.Assertions; +import com.facebook.infer.annotation.FalseOnNull; +import com.facebook.infer.annotation.PropagatesNullable; +import com.facebook.infer.annotation.TrueOnNull; + + +public class CustomAnnotations { + + static class MyTextUtils { + + @TrueOnNull + static boolean isEmpty(@Nullable java.lang.CharSequence s) { + return s == null || s.equals(""); + } + + @FalseOnNull + static boolean isNotEmpty(@Nullable java.lang.CharSequence s) { + return s != null && s.length() > 0; + } + } + + class TestTextUtilsIsEmpty { + void textUtilsNotIsEmpty(@Nullable CharSequence s) { + if (!TextUtils.isEmpty(s)) { + s.toString(); // OK + } + } + + void textUtilsIsEmpty(@Nullable CharSequence s) { + if (TextUtils.isEmpty(s)) { + s.toString(); // BAD + } + } + + void myTextUtilsNotIsEmpty(@Nullable CharSequence s) { + if (!MyTextUtils.isEmpty(s)) { + s.toString(); // OK + } + } + + void myTextUtilsIsEmpty(@Nullable CharSequence s) { + if (MyTextUtils.isEmpty(s)) { + s.toString(); // BAD + } + } + void myTextUtilsIsNotEmpty(@Nullable CharSequence s) { + if (MyTextUtils.isNotEmpty(s)) { + s.toString(); // OK + } + } + + void myTextUtilsNotIsNotEmpty(@Nullable CharSequence s) { + if (!MyTextUtils.isNotEmpty(s)) { + s.toString(); // BAD + } + } + } + + + // Tests for the annotation @PropagatesNullable + class TestPropagatesNullable { + + // one parameter: means return null iff s is null + String m(@PropagatesNullable String s) { + return s; + } + + void mBad() { + m(null).length(); // bad: m returns null + } + + void mGood() { + m("").length(); + } + + // limitation: we currently cannot check the body, and just trust the annotation + String cannotCheckBody(@PropagatesNullable String s) { + return null; // nothing is reported here + } + + void illustrateFalseNegativeAsCannotCheckBody() { + cannotCheckBody("").length(); // this is an NPE but is not found + } + + // second parameter: means return null iff s2 is null + String m2(@Nullable String s1, @PropagatesNullable String s2) { + return s2; + } + + void m2Bad() { + m2(null, null).length(); // bad: m2 returns null + } + + void m2Good() { + m2(null, "").length(); + } + + // two parameters: means return null iff either s1 or s2 is null + String m12(@PropagatesNullable String s1, @PropagatesNullable String s2) { + return s1 == null ? s1 : s2; + } + + void m12Bad1() { + m12(null, "").length(); // bad: m12 returns null + } + + void m12Bad2() { + m12("", null).length(); // bad: m12 returns null + } + + void m12Good() { + m12("", "").length(); + } + } +} diff --git a/infer/tests/codetoanalyze/java/eradicate/NullMethodCall.java b/infer/tests/codetoanalyze/java/eradicate/NullMethodCall.java index c2ba3ae3e..68ca0cada 100644 --- a/infer/tests/codetoanalyze/java/eradicate/NullMethodCall.java +++ b/infer/tests/codetoanalyze/java/eradicate/NullMethodCall.java @@ -9,14 +9,11 @@ package codetoanalyze.java.eradicate; -import android.text.TextUtils; import com.google.common.base.Preconditions; import java.lang.System; import javax.annotation.Nullable; import com.facebook.infer.annotation.Assertions; -import com.facebook.infer.annotation.FalseOnNull; -import com.facebook.infer.annotation.TrueOnNull; public class NullMethodCall { @@ -242,57 +239,6 @@ public class NullMethodCall { int n = s.length(); } - - static class MyTextUtils { - - @TrueOnNull - static boolean isEmpty(@Nullable java.lang.CharSequence s) { - return s == null || s.equals(""); - } - - @FalseOnNull - static boolean isNotEmpty(@Nullable java.lang.CharSequence s) { - return s != null && s.length() > 0; - } - } - - class TestTextUtilsIsEmpty { - void textUtilsNotIsEmpty(@Nullable CharSequence s) { - if (!TextUtils.isEmpty(s)) { - s.toString(); // OK - } - } - - void textUtilsIsEmpty(@Nullable CharSequence s) { - if (TextUtils.isEmpty(s)) { - s.toString(); // BAD - } - } - - void myTextUtilsNotIsEmpty(@Nullable CharSequence s) { - if (!MyTextUtils.isEmpty(s)) { - s.toString(); // OK - } - } - - void myTextUtilsIsEmpty(@Nullable CharSequence s) { - if (MyTextUtils.isEmpty(s)) { - s.toString(); // BAD - } - } - void myTextUtilsIsNotEmpty(@Nullable CharSequence s) { - if (MyTextUtils.isNotEmpty(s)) { - s.toString(); // OK - } - } - - void myTextUtilsNotIsNotEmpty(@Nullable CharSequence s) { - if (!MyTextUtils.isNotEmpty(s)) { - s.toString(); // BAD - } - } - } - class SystemExitDoesNotReturn { native boolean whoknows(); diff --git a/infer/tests/codetoanalyze/java/eradicate/ParameterNotNullable.java b/infer/tests/codetoanalyze/java/eradicate/ParameterNotNullable.java index 538d863af..1f7ab0353 100644 --- a/infer/tests/codetoanalyze/java/eradicate/ParameterNotNullable.java +++ b/infer/tests/codetoanalyze/java/eradicate/ParameterNotNullable.java @@ -85,4 +85,17 @@ public class ParameterNotNullable { threeParameters(s, null, s); threeParameters(s, s, null); } + + class ConstructorCall { + ConstructorCall(int x, String s) { + } + + ConstructorCall() { + this(3, ""); // OK + } + + ConstructorCall(int x) { + this(3, null); // NPE + } + } } diff --git a/infer/tests/codetoanalyze/java/eradicate/issues.exp b/infer/tests/codetoanalyze/java/eradicate/issues.exp index 37f3fbe18..1efb68963 100644 --- a/infer/tests/codetoanalyze/java/eradicate/issues.exp +++ b/infer/tests/codetoanalyze/java/eradicate/issues.exp @@ -1,4 +1,11 @@ codetoanalyze/java/eradicate/ActivityFieldNotInitialized.java, ActivityFieldNotInitialized$BadActivityWithOnCreate.(ActivityFieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, [Field `ActivityFieldNotInitialized$BadActivityWithOnCreate.field` is not initialized in the constructor and is not declared `@Nullable`] +codetoanalyze/java/eradicate/CustomAnnotations.java, void CustomAnnotations$TestPropagatesNullable.m12Bad1(), 1, ERADICATE_NULL_METHOD_CALL, [origin,The value of `m12(...)` in the call to `length()` could be null. (Origin: call to m12(...) at line 118)] +codetoanalyze/java/eradicate/CustomAnnotations.java, void CustomAnnotations$TestPropagatesNullable.m12Bad2(), 1, ERADICATE_NULL_METHOD_CALL, [origin,The value of `m12(...)` in the call to `length()` could be null. (Origin: call to m12(...) at line 122)] +codetoanalyze/java/eradicate/CustomAnnotations.java, void CustomAnnotations$TestPropagatesNullable.m2Bad(), 1, ERADICATE_NULL_METHOD_CALL, [origin,The value of `m2(...)` in the call to `length()` could be null. (Origin: call to m2(...) at line 105)] +codetoanalyze/java/eradicate/CustomAnnotations.java, void CustomAnnotations$TestPropagatesNullable.mBad(), 1, ERADICATE_NULL_METHOD_CALL, [origin,The value of `m(...)` in the call to `length()` could be null. (Origin: call to m(...) at line 83)] +codetoanalyze/java/eradicate/CustomAnnotations.java, void CustomAnnotations$TestTextUtilsIsEmpty.myTextUtilsIsEmpty(CharSequence), 2, ERADICATE_NULL_METHOD_CALL, [The value of `s` in the call to `toString()` could be null. (Origin: method parameter s)] +codetoanalyze/java/eradicate/CustomAnnotations.java, void CustomAnnotations$TestTextUtilsIsEmpty.myTextUtilsNotIsNotEmpty(CharSequence), 2, ERADICATE_NULL_METHOD_CALL, [The value of `s` in the call to `toString()` could be null. (Origin: method parameter s)] +codetoanalyze/java/eradicate/CustomAnnotations.java, void CustomAnnotations$TestTextUtilsIsEmpty.textUtilsIsEmpty(CharSequence), 2, ERADICATE_NULL_METHOD_CALL, [The value of `s` in the call to `toString()` could be null. (Origin: method parameter s)] codetoanalyze/java/eradicate/FieldNotInitialized.java, FieldNotInitialized$ConditionalFieldInit.(FieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, [Field `FieldNotInitialized$ConditionalFieldInit.o1` is not initialized in the constructor and is not declared `@Nullable`] codetoanalyze/java/eradicate/FieldNotInitialized.java, FieldNotInitialized$InitCircular.(FieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, [Field `FieldNotInitialized$InitCircular.s` is not initialized in the constructor and is not declared `@Nullable`] codetoanalyze/java/eradicate/FieldNotInitialized.java, FieldNotInitialized$OnlyRead.(FieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, [Field `FieldNotInitialized$OnlyRead.o` is not initialized in the constructor and is not declared `@Nullable`] @@ -30,15 +37,13 @@ codetoanalyze/java/eradicate/NullFieldAccess.java, int NullFieldAccess.arrayLeng codetoanalyze/java/eradicate/NullFieldAccess.java, int NullFieldAccess.useInterface(NullFieldAccess$I), 2, ERADICATE_NULL_FIELD_ACCESS, [origin,Object `c` could be null when accessing field `NullFieldAccess$C.n`. (Origin: field NullFieldAccess$I.c at line 52)] codetoanalyze/java/eradicate/NullFieldAccess.java, int NullFieldAccess.useX(), 2, ERADICATE_NULL_FIELD_ACCESS, [origin,Object `c` could be null when accessing field `NullFieldAccess$C.n`. (Origin: field NullFieldAccess.x at line 37)] codetoanalyze/java/eradicate/NullFieldAccess.java, int NullFieldAccess.useZ(), 2, ERADICATE_NULL_FIELD_ACCESS, [origin,Object `c` could be null when accessing field `NullFieldAccess$C.n`. (Origin: field NullFieldAccess.z at line 47)] -codetoanalyze/java/eradicate/NullMethodCall.java, int NullMethodCall$Inner.outerField(), 2, ERADICATE_NULL_METHOD_CALL, [origin,The value of `s` in the call to `length()` could be null. (Origin: field NullMethodCall.fld at line 74)] -codetoanalyze/java/eradicate/NullMethodCall.java, int NullMethodCall$Inner.outerPrivateField(), 2, ERADICATE_NULL_METHOD_CALL, [origin,The value of `s` in the call to `length()` could be null. (Origin: field NullMethodCall.pfld at line 85)] -codetoanalyze/java/eradicate/NullMethodCall.java, void NullMethodCall$TestTextUtilsIsEmpty.myTextUtilsIsEmpty(CharSequence), 2, ERADICATE_NULL_METHOD_CALL, [The value of `s` in the call to `toString()` could be null. (Origin: method parameter s)] -codetoanalyze/java/eradicate/NullMethodCall.java, void NullMethodCall$TestTextUtilsIsEmpty.myTextUtilsNotIsNotEmpty(CharSequence), 2, ERADICATE_NULL_METHOD_CALL, [The value of `s` in the call to `toString()` could be null. (Origin: method parameter s)] -codetoanalyze/java/eradicate/NullMethodCall.java, void NullMethodCall$TestTextUtilsIsEmpty.textUtilsIsEmpty(CharSequence), 2, ERADICATE_NULL_METHOD_CALL, [The value of `s` in the call to `toString()` could be null. (Origin: method parameter s)] -codetoanalyze/java/eradicate/NullMethodCall.java, void NullMethodCall.callOnNull(), 2, ERADICATE_NULL_METHOD_CALL, [origin,The value of `s` in the call to `length()` could be null. (Origin: null constant at line 25)] -codetoanalyze/java/eradicate/NullMethodCall.java, void NullMethodCall.testExceptionPerInstruction(int), 6, ERADICATE_NULL_METHOD_CALL, [origin,The value of `s` in the call to `length()` could be null. (Origin: null constant at line 186)] -codetoanalyze/java/eradicate/NullMethodCall.java, void NullMethodCall.testFieldAssignmentIfThenElse(String), 2, ERADICATE_NULL_METHOD_CALL, [origin,The value of `s` in the call to `length()` could be null. (Origin: null constant at line 177)] -codetoanalyze/java/eradicate/NullMethodCall.java, void NullMethodCall.testSystemGetPropertyReturn(), 2, ERADICATE_NULL_METHOD_CALL, [origin,The value of `s` in the call to `length()` could be null. (Origin: call to getProperty(...) modelled in eradicate/modelTables.ml at line 241)] +codetoanalyze/java/eradicate/NullMethodCall.java, int NullMethodCall$Inner.outerField(), 2, ERADICATE_NULL_METHOD_CALL, [origin,The value of `s` in the call to `length()` could be null. (Origin: field NullMethodCall.fld at line 71)] +codetoanalyze/java/eradicate/NullMethodCall.java, int NullMethodCall$Inner.outerPrivateField(), 2, ERADICATE_NULL_METHOD_CALL, [origin,The value of `s` in the call to `length()` could be null. (Origin: field NullMethodCall.pfld at line 82)] +codetoanalyze/java/eradicate/NullMethodCall.java, void NullMethodCall.callOnNull(), 2, ERADICATE_NULL_METHOD_CALL, [origin,The value of `s` in the call to `length()` could be null. (Origin: null constant at line 22)] +codetoanalyze/java/eradicate/NullMethodCall.java, void NullMethodCall.testExceptionPerInstruction(int), 6, ERADICATE_NULL_METHOD_CALL, [origin,The value of `s` in the call to `length()` could be null. (Origin: null constant at line 183)] +codetoanalyze/java/eradicate/NullMethodCall.java, void NullMethodCall.testFieldAssignmentIfThenElse(String), 2, ERADICATE_NULL_METHOD_CALL, [origin,The value of `s` in the call to `length()` could be null. (Origin: null constant at line 174)] +codetoanalyze/java/eradicate/NullMethodCall.java, void NullMethodCall.testSystemGetPropertyReturn(), 2, ERADICATE_NULL_METHOD_CALL, [origin,The value of `s` in the call to `length()` could be null. (Origin: call to getProperty(...) modelled in eradicate/modelTables.ml at line 238)] +codetoanalyze/java/eradicate/ParameterNotNullable.java, ParameterNotNullable$ConstructorCall.(ParameterNotNullable,int), 1, ERADICATE_PARAMETER_NOT_NULLABLE, [origin,`ParameterNotNullable$ConstructorCall(...)` needs a non-null value in parameter 2 but argument `null` can be null. (Origin: null constant at line 98)] codetoanalyze/java/eradicate/ParameterNotNullable.java, String ParameterNotNullable.testSystemGetPropertyArgument(), 1, ERADICATE_PARAMETER_NOT_NULLABLE, [origin,`getProperty(...)` needs a non-null value in parameter 1 but argument `null` can be null. (Origin: null constant at line 71)] codetoanalyze/java/eradicate/ParameterNotNullable.java, URL ParameterNotNullable.testClassGetResourceArgument(Class), 1, ERADICATE_PARAMETER_NOT_NULLABLE, [origin,`getResource(...)` needs a non-null value in parameter 1 but argument `null` can be null. (Origin: null constant at line 76)] codetoanalyze/java/eradicate/ParameterNotNullable.java, void ParameterNotNullable.callNull(), 2, ERADICATE_PARAMETER_NOT_NULLABLE, [origin,`test(...)` needs a non-null value in parameter 1 but argument `s` can be null. (Origin: null constant at line 38)]