From 055a8267e8779645f90d95f01e312f25df2a0a83 Mon Sep 17 00:00:00 2001 From: Jeremy Dubreil Date: Wed, 20 Jun 2018 21:49:06 -0700 Subject: [PATCH] [eradicate] promote reports as errors for generated GraphQL source of nulls Reviewed By: mbouaziz Differential Revision: D8519147 fbshipit-source-id: 69f6364 --- infer/src/IR/Localise.ml | 7 --- infer/src/IR/Localise.mli | 3 -- infer/src/absint/Checkers.ml | 17 +++++-- infer/src/absint/Checkers.mli | 4 +- infer/src/backend/InferPrint.ml | 16 +++--- infer/src/checkers/annotations.ml | 6 +-- infer/src/checkers/annotations.mli | 4 +- infer/src/eradicate/typeErr.ml | 50 ++++++------------- infer/src/eradicate/typeErr.mli | 2 +- .../java/eradicate/GeneratedGraphQL.java | 16 ++++++ .../java/eradicate/NullMethodCall.java | 3 ++ .../eradicate/ServerSideDeserializer.java | 15 ++++++ .../codetoanalyze/java/eradicate/issues.exp | 1 + 13 files changed, 75 insertions(+), 69 deletions(-) create mode 100644 infer/tests/codetoanalyze/java/eradicate/GeneratedGraphQL.java create mode 100644 infer/tests/codetoanalyze/java/eradicate/ServerSideDeserializer.java diff --git a/infer/src/IR/Localise.ml b/infer/src/IR/Localise.ml index ff858c105..fdc904c2d 100644 --- a/infer/src/IR/Localise.ml +++ b/infer/src/IR/Localise.ml @@ -87,13 +87,6 @@ module BucketLevel = struct let b5 = "B5" end -(** takes in input a tag to extract from the given error_desc - and returns its value *) -let error_desc_extract_tag_value err_desc tag_to_extract = - let find_value tag v = match v with t, _ when String.equal t tag -> true | _ -> false in - match List.find ~f:(find_value tag_to_extract) err_desc.tags with Some (_, s) -> s | None -> "" - - (** get the bucket value of an error_desc, if any *) let error_desc_get_bucket err_desc = Tags.get err_desc.tags Tags.bucket diff --git a/infer/src/IR/Localise.mli b/infer/src/IR/Localise.mli index 0b664081c..c9b3462e2 100644 --- a/infer/src/IR/Localise.mli +++ b/infer/src/IR/Localise.mli @@ -41,9 +41,6 @@ module BucketLevel : sig (** lowest likelihood *) end -val error_desc_extract_tag_value : error_desc -> string -> string -(** returns the value of a tag or the empty string *) - val error_desc_get_bucket : error_desc -> string option (** get the bucket value of an error_desc, if any *) diff --git a/infer/src/absint/Checkers.ml b/infer/src/absint/Checkers.ml index c50a299c3..a387654b2 100644 --- a/infer/src/absint/Checkers.ml +++ b/infer/src/absint/Checkers.ml @@ -6,15 +6,24 @@ *) open! IStd +module L = Logging (** Module for user-defined checkers. *) module ST = struct let report_error tenv proc_name proc_desc kind loc ?(field_name= None) ?(origin_loc= None) - ?(exception_kind= fun k d -> Exceptions.Checkers (k, d)) ?(always_report= false) description = - let localized_description = - Localise.custom_desc description [("always_report", string_of_bool always_report)] + ?(exception_kind= fun k d -> Exceptions.Checkers (k, d)) ?(severity= Exceptions.Kwarning) + description = + let log = + match severity with + | Exceptions.Kwarning -> + Reporting.log_warning_deprecated + | Exceptions.Kerror -> + Reporting.log_error_deprecated + | _ -> + L.(die InternalError) "Severity not supported" in + let localized_description = Localise.custom_desc description [] in let exn = exception_kind kind localized_description in let suppressed = Reporting.is_suppressed tenv proc_desc kind ~field_name in let trace = @@ -27,5 +36,5 @@ module ST = struct in origin_elements @ [Errlog.make_trace_element 0 loc description []] in - if not suppressed then Reporting.log_warning_deprecated proc_name ~loc ~ltr:trace exn + if not suppressed then log proc_name ~loc ~ltr:trace exn end diff --git a/infer/src/absint/Checkers.mli b/infer/src/absint/Checkers.mli index 8abf7b3ae..43dfad8de 100644 --- a/infer/src/absint/Checkers.mli +++ b/infer/src/absint/Checkers.mli @@ -13,7 +13,7 @@ module ST : sig val report_error : Tenv.t -> Typ.Procname.t -> Procdesc.t -> IssueType.t -> Location.t -> ?field_name:Typ.Fieldname.t option -> ?origin_loc:Location.t option - -> ?exception_kind:(IssueType.t -> Localise.error_desc -> exn) -> ?always_report:bool -> string - -> unit + -> ?exception_kind:(IssueType.t -> Localise.error_desc -> exn) -> ?severity:Exceptions.err_kind + -> string -> unit (** Report an error. *) end diff --git a/infer/src/backend/InferPrint.ml b/infer/src/backend/InferPrint.ml index 02f547f1a..166299ea2 100644 --- a/infer/src/backend/InferPrint.ml +++ b/infer/src/backend/InferPrint.ml @@ -230,8 +230,7 @@ module IssuesJson = struct not (SourceFile.is_infer_model source_file) || Config.debug_mode || Config.debug_exceptions in if - key.in_footprint && error_filter source_file key.err_desc key.err_name - && should_report_source_file + key.in_footprint && error_filter source_file key.err_name && should_report_source_file && should_report key.err_kind key.err_name key.err_desc err_data.err_class then ( let kind = Exceptions.err_kind_string key.err_kind in @@ -380,7 +379,7 @@ module IssuesTxt = struct err_data.loc.Location.file in if - key.in_footprint && error_filter source_file key.err_desc key.err_name + key.in_footprint && error_filter source_file key.err_name && (not Config.filtering || String.is_empty (censored_reason key.err_name source_file)) then Exceptions.pp_err ~node_key:err_data.node_id_key.node_key err_data.loc key.err_kind @@ -472,7 +471,7 @@ module Stats = struct let found_errors = ref false in let process_row (key: Errlog.err_key) (err_data: Errlog.err_data) = let type_str = key.err_name.IssueType.unique_id in - if key.in_footprint && error_filter key.err_desc key.err_name then + if key.in_footprint && error_filter key.err_name then match key.err_kind with | Exceptions.Kerror -> found_errors := true ; @@ -646,13 +645,10 @@ module Issue = struct issues' end -let error_filter filters proc_name file error_desc error_name = - let always_report () = - String.equal (Localise.error_desc_extract_tag_value error_desc "always_report") "true" - in +let error_filter filters proc_name file error_name = (Config.write_html || not (IssueType.(equal skip_function) error_name)) - && (filters.Inferconfig.path_filter file || always_report ()) - && filters.Inferconfig.error_filter error_name && filters.Inferconfig.proc_filter proc_name + && filters.Inferconfig.path_filter file && filters.Inferconfig.error_filter error_name + && filters.Inferconfig.proc_filter proc_name type report_kind = Issues | Procs | Stats | Summary [@@deriving compare] diff --git a/infer/src/checkers/annotations.ml b/infer/src/checkers/annotations.ml index 6c9a0a615..f89e37efe 100644 --- a/infer/src/checkers/annotations.ml +++ b/infer/src/checkers/annotations.ml @@ -85,7 +85,7 @@ let returns_ownership = "ReturnsOwnership" let synchronized_collection = "SynchronizedCollection" -let strict = "com.facebook.infer.annotation.Strict" +let generated_graphql = "GeneratedGraphQL" let suppress_lint = "SuppressLint" @@ -132,8 +132,6 @@ let ia_ends_with ia ann_name = List.exists ~f:(fun (a, _) -> annot_ends_with a a let ia_contains ia ann_name = List.exists ~f:(class_name_matches ann_name) ia -let ia_get ia ann_name = List.find ~f:(class_name_matches ann_name) ia |> Option.map ~f:fst - let pdesc_get_return_annot pdesc = fst (Procdesc.get_attributes pdesc).ProcAttributes.method_annotation @@ -219,8 +217,6 @@ let ia_is_field_injector_readwrite ia = let ia_is_mutable ia = ia_ends_with ia mutable_ -let ia_get_strict ia = ia_get ia strict - let ia_is_verify ia = ia_contains ia verify_annotation let ia_is_expensive ia = ia_ends_with ia expensive diff --git a/infer/src/checkers/annotations.mli b/infer/src/checkers/annotations.mli index 7abae9561..f7a04bf1b 100644 --- a/infer/src/checkers/annotations.mli +++ b/infer/src/checkers/annotations.mli @@ -45,6 +45,8 @@ val ui_thread : string val visibleForTesting : string +val generated_graphql : string + val annot_ends_with : Annot.t -> string -> bool (** [annot_ends_with annot ann_name] returns true if the class name of [annot], without the package, is equal to [ann_name] *) @@ -54,8 +56,6 @@ val ia_ends_with : Annot.Item.t -> string -> bool val ia_has_annotation_with : Annot.Item.t -> (Annot.t -> bool) -> bool -val ia_get_strict : Annot.Item.t -> Annot.t option - val ia_is_false_on_null : Annot.Item.t -> bool val ia_is_initializer : Annot.Item.t -> bool diff --git a/infer/src/eradicate/typeErr.ml b/infer/src/eradicate/typeErr.ml index d7c97511d..1c1d8119a 100644 --- a/infer/src/eradicate/typeErr.ml +++ b/infer/src/eradicate/typeErr.ml @@ -211,63 +211,43 @@ let add_err find_canonical_duplicate err_instance instr_ref_opt loc = (* print now if it's not a forall check *) -module Strict = struct - let method_get_strict (signature: AnnotatedSignature.t) = - let ia, _ = signature.ret in - Annotations.ia_get_strict ia +module Severity = struct + let get_severity ia = + if Annotations.ia_ends_with ia Annotations.generated_graphql then Some Exceptions.Kerror + else None - let this_type_get_strict tenv (signature: AnnotatedSignature.t) = + let this_type_get_severity tenv (signature: AnnotatedSignature.t) = match signature.params with - | (p, _, this_type) :: _ when String.equal (Mangled.to_string p) "this" -> ( - match PatternMatch.type_get_annotation tenv this_type with - | Some ia -> - Annotations.ia_get_strict ia - | None -> - None ) + | (p, _, this_type) :: _ when String.equal (Mangled.to_string p) "this" -> + Option.bind ~f:get_severity (PatternMatch.type_get_annotation tenv this_type) | _ -> None - let signature_get_strict tenv signature = - match method_get_strict signature with - | None -> - this_type_get_strict tenv signature - | Some x -> - Some x - - - let origin_descr_get_strict tenv origin_descr = + let origin_descr_get_severity tenv origin_descr = match origin_descr with | _, _, Some signature -> - signature_get_strict tenv signature + this_type_get_severity tenv signature | _, _, None -> None - let report_on_method_arguments = false - - (* Return (Some parameters) if there is a method call on a @Nullable object,*) - (* where the origin of @Nullable in the analysis is the return value of a Strict method*) - (* with parameters. A method is Strict if it or its class are annotated @Strict. *) - let err_instance_get_strict tenv err_instance : Annot.t option = + let err_instance_get_severity tenv err_instance : Exceptions.err_kind option = match err_instance with | Call_receiver_annotation_inconsistent (AnnotatedSignature.Nullable, _, _, origin_descr) | Null_field_access (_, _, origin_descr, _) -> - origin_descr_get_strict tenv origin_descr - | Parameter_annotation_inconsistent (AnnotatedSignature.Nullable, _, _, _, _, origin_descr) - when report_on_method_arguments -> - origin_descr_get_strict tenv origin_descr + origin_descr_get_severity tenv origin_descr | _ -> None end -(* Strict *) +(* Severity *) type st_report_error = Typ.Procname.t -> Procdesc.t -> IssueType.t -> Location.t -> ?field_name:Typ.Fieldname.t option -> ?origin_loc:Location.t option -> ?exception_kind:(IssueType.t -> Localise.error_desc -> exn) - -> ?always_report:bool -> string -> unit + -> ?severity:Exceptions.err_kind -> string -> unit (** Report an error right now. *) let report_error_now tenv (st_report_error: st_report_error) err_instance loc pdesc : unit = @@ -447,10 +427,10 @@ let report_error_now tenv (st_report_error: st_report_error) err_instance loc pd , None , None ) in - let always_report = Strict.err_instance_get_strict tenv err_instance <> None in + let severity = Severity.err_instance_get_severity tenv err_instance in st_report_error pname pdesc kind loc ~field_name ~origin_loc ~exception_kind:(fun k d -> Exceptions.Eradicate (k, d)) - ~always_report description + ?severity description (** Report an error unless is has been reported already, or unless it's a forall error diff --git a/infer/src/eradicate/typeErr.mli b/infer/src/eradicate/typeErr.mli index 9e51d90ff..02a113bb1 100644 --- a/infer/src/eradicate/typeErr.mli +++ b/infer/src/eradicate/typeErr.mli @@ -67,7 +67,7 @@ val node_reset_forall : Procdesc.Node.t -> unit type st_report_error = Typ.Procname.t -> Procdesc.t -> IssueType.t -> Location.t -> ?field_name:Typ.Fieldname.t option -> ?origin_loc:Location.t option -> ?exception_kind:(IssueType.t -> Localise.error_desc -> exn) - -> ?always_report:bool -> string -> unit + -> ?severity:Exceptions.err_kind -> string -> unit val report_error : Tenv.t -> st_report_error -> (Procdesc.Node.t -> Procdesc.Node.t) -> err_instance diff --git a/infer/tests/codetoanalyze/java/eradicate/GeneratedGraphQL.java b/infer/tests/codetoanalyze/java/eradicate/GeneratedGraphQL.java new file mode 100644 index 000000000..e62d0ec3e --- /dev/null +++ b/infer/tests/codetoanalyze/java/eradicate/GeneratedGraphQL.java @@ -0,0 +1,16 @@ +/* + * Copyright (c) 2018-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ +package codetoanalyze.java.eradicate; + +import java.lang.annotation.ElementType; +import java.lang.annotation.Retention; +import java.lang.annotation.RetentionPolicy; +import java.lang.annotation.Target; + +@Retention(RetentionPolicy.CLASS) +@Target(ElementType.TYPE) +public @interface GeneratedGraphQL {} diff --git a/infer/tests/codetoanalyze/java/eradicate/NullMethodCall.java b/infer/tests/codetoanalyze/java/eradicate/NullMethodCall.java index 9893258c2..b42a0dc5d 100644 --- a/infer/tests/codetoanalyze/java/eradicate/NullMethodCall.java +++ b/infer/tests/codetoanalyze/java/eradicate/NullMethodCall.java @@ -305,4 +305,7 @@ public class NullMethodCall { manager.cancel(intent); } + String callingSeverSideNullableGetter(ServerSideDeserializer deserializer) { + return deserializer.nullableGetter().toString(); + } } diff --git a/infer/tests/codetoanalyze/java/eradicate/ServerSideDeserializer.java b/infer/tests/codetoanalyze/java/eradicate/ServerSideDeserializer.java new file mode 100644 index 000000000..11d13c43b --- /dev/null +++ b/infer/tests/codetoanalyze/java/eradicate/ServerSideDeserializer.java @@ -0,0 +1,15 @@ +/* + * Copyright (c) 2018-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ +package codetoanalyze.java.eradicate; + +import javax.annotation.Nullable; + +@GeneratedGraphQL +public interface ServerSideDeserializer { + + public @Nullable Object nullableGetter(); +} diff --git a/infer/tests/codetoanalyze/java/eradicate/issues.exp b/infer/tests/codetoanalyze/java/eradicate/issues.exp index 87b1e33bc..26daef898 100644 --- a/infer/tests/codetoanalyze/java/eradicate/issues.exp +++ b/infer/tests/codetoanalyze/java/eradicate/issues.exp @@ -40,6 +40,7 @@ codetoanalyze/java/eradicate/NullFieldAccess.java, int NullFieldAccess.arrayLeng codetoanalyze/java/eradicate/NullFieldAccess.java, int NullFieldAccess.useInterface(NullFieldAccess$I), 2, ERADICATE_NULL_FIELD_ACCESS, no_bucket, WARNING, [origin,Object `c` could be null when accessing field `NullFieldAccess$C.n`. (Origin: field NullFieldAccess$I.c at line 50)] codetoanalyze/java/eradicate/NullFieldAccess.java, int NullFieldAccess.useX(), 2, ERADICATE_NULL_FIELD_ACCESS, no_bucket, WARNING, [origin,Object `c` could be null when accessing field `NullFieldAccess$C.n`. (Origin: field NullFieldAccess.x at line 35)] codetoanalyze/java/eradicate/NullFieldAccess.java, int NullFieldAccess.useZ(), 2, ERADICATE_NULL_FIELD_ACCESS, no_bucket, WARNING, [origin,Object `c` could be null when accessing field `NullFieldAccess$C.n`. (Origin: field NullFieldAccess.z at line 45)] +codetoanalyze/java/eradicate/NullMethodCall.java, String NullMethodCall.callingSeverSideNullableGetter(ServerSideDeserializer), 1, ERADICATE_NULL_METHOD_CALL, no_bucket, ERROR, [origin,The value of `deserializer.nullableGetter()` in the call to `toString()` could be null. (Origin: call to nullableGetter() at line 309)] codetoanalyze/java/eradicate/NullMethodCall.java, int NullMethodCall$Inner.outerField(), 2, ERADICATE_NULL_METHOD_CALL, no_bucket, WARNING, [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, no_bucket, WARNING, [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, int NullMethodCall.testSystemGetenvBad(), 2, ERADICATE_NULL_METHOD_CALL, no_bucket, WARNING, [origin,The value of `envValue` in the call to `length()` could be null. (Origin: call to getenv(...) modelled in modelTables.ml at line 246)]