From 816af4a35563a3e9e2b1302dbdee7474db482f25 Mon Sep 17 00:00:00 2001 From: Artem Pianykh Date: Wed, 4 Nov 2020 12:23:19 -0800 Subject: [PATCH] [java][reporting] @SuppressLint now blocks _only_ specified issue types Summary: The problem in Reporting.ml:log_issue_from_summary is that it merely checks the presence of `SuppressLint` annotation on method's body to decide whether to log or not the issue. This means that regardless of issue types specified in `SuppressLint`, all issues on such method will get blocked. Here we fix that. Reviewed By: ngorogiannis, mityal Differential Revision: D24726604 fbshipit-source-id: c9cae3833 --- infer/man/man1/infer-full.txt | 6 + infer/src/absint/Reporting.ml | 127 ++++++++++-------- infer/src/base/Config.ml | 9 ++ infer/src/base/Config.mli | 2 + .../java/biabduction/SuppressLintExample.java | 4 +- .../codetoanalyze/java/checkers/issues.exp | 1 + .../java/nullsafe/FieldNotInitialized.java | 6 +- .../nullsafe/SyntheticErrorSuppressions.java | 2 +- .../codetoanalyze/java/nullsafe/issues.exp | 1 + 9 files changed, 97 insertions(+), 61 deletions(-) diff --git a/infer/man/man1/infer-full.txt b/infer/man/man1/infer-full.txt index 7f1c0d0b6..39f86a494 100644 --- a/infer/man/man1/infer-full.txt +++ b/infer/man/man1/infer-full.txt @@ -1904,6 +1904,12 @@ INTERNAL OPTIONS --summaries-caches-max-size int The maximum amount of elements the summaries LRU caches can hold + --suppress-lint-ignore-types + Activates: [DEPRECATED] Check only the presence of @SuppressLint + but not the issues types specified as parameters to the + annotations when deciding to suppress issues. Use for backwards + compatibility only! (Conversely: --no-suppress-lint-ignore-types) + --symops-per-iteration int Set the number of symbolic operations per iteration (see --iterations) diff --git a/infer/src/absint/Reporting.ml b/infer/src/absint/Reporting.ml index 271450c1b..d3b83a7ff 100644 --- a/infer/src/absint/Reporting.ml +++ b/infer/src/absint/Reporting.ml @@ -10,6 +10,68 @@ open! IStd type log_t = ?ltr:Errlog.loc_trace -> ?extras:Jsonbug_t.extra -> Checker.t -> IssueType.t -> string -> unit +module Suppression = struct + let does_annotation_suppress_issue (kind : IssueType.t) (annot : Annot.t) = + let normalize str = Str.global_replace (Str.regexp "[_-]") "" (String.lowercase str) in + let drop_prefix str = Str.replace_first (Str.regexp "^[A-Za-z]+_") "" str in + let normalized_equal s1 a2 = + Annot.( + has_matching_str_value a2.value ~pred:(fun s -> String.equal (normalize s1) (normalize s))) + in + let is_parameter_suppressed () = + String.is_suffix annot.class_name ~suffix:Annotations.suppress_lint + && List.exists ~f:(normalized_equal kind.IssueType.unique_id) annot.parameters + in + let is_annotation_suppressed () = + String.is_suffix + ~suffix:(normalize (drop_prefix kind.IssueType.unique_id)) + (normalize annot.class_name) + in + is_parameter_suppressed () || is_annotation_suppressed () + + + let is_method_suppressed proc_attributes kind = + Annotations.ma_has_annotation_with proc_attributes.ProcAttributes.method_annotation + (does_annotation_suppress_issue kind) + + + let is_field_suppressed field_name tenv proc_attributes kind = + let lookup = Tenv.lookup tenv in + match (field_name, PatternMatch.get_this_type_nonstatic_methods_only proc_attributes) with + | Some field_name, Some t -> ( + match Struct.get_field_type_and_annotation ~lookup field_name t with + | Some (_, ia) -> + Annotations.ia_has_annotation_with ia (does_annotation_suppress_issue kind) + | None -> + false ) + | _ -> + false + + + let is_class_suppressed tenv proc_attributes kind = + match PatternMatch.get_this_type_nonstatic_methods_only proc_attributes with + | Some t -> ( + match PatternMatch.type_get_annotation tenv t with + | Some ia -> + Annotations.ia_has_annotation_with ia (does_annotation_suppress_issue kind) + | None -> + false ) + | None -> + false + + + let is_suppressed ?(field_name = None) tenv proc_attributes kind = + (* Errors can be suppressed with annotations. An error of kind CHECKER_ERROR_NAME can be + suppressed with the following annotations: + - @android.annotation.SuppressLint("checker-error-name") + - @some.PrefixErrorName + where the kind matching is case - insensitive and ignores '-' and '_' characters. *) + let is_method_suppressed () = is_method_suppressed proc_attributes kind in + let is_field_suppressed () = is_field_suppressed field_name tenv proc_attributes kind in + let is_class_suppressed () = is_class_suppressed tenv proc_attributes kind in + is_method_suppressed () || is_field_suppressed () || is_class_suppressed () +end + let log_issue_from_errlog ?severity_override err_log ~loc ~node ~session ~ltr ~access ~extras checker (issue_to_report : IssueToReport.t) = let issue_type = issue_to_report.issue_type in @@ -28,6 +90,7 @@ let log_frontend_issue errlog ~loc ~node_key ~ltr exn = let log_issue_from_summary ?severity_override proc_desc err_log ~node ~session ~loc ~ltr ?extras checker exn = let procname = Procdesc.get_proc_name proc_desc in + let issue_type = exn.IssueToReport.issue_type in let is_java_generated_method = match procname with | Procname.Java java_pname -> @@ -42,10 +105,15 @@ let log_issue_from_summary ?severity_override proc_desc err_log ~node ~session ~ | _ -> false in + let check_suppress proc_attributes issue_type = + if Config.suppress_lint_ignore_types then + (* This is for backwards compatibility only! + We should really honor the issues types specified as params to @SuppressLint *) + Annotations.ia_is_suppress_lint proc_attributes.ProcAttributes.method_annotation.return + else Suppression.is_method_suppressed proc_attributes issue_type + in let should_suppress_lint = - Language.curr_language_is Java - && Annotations.ia_is_suppress_lint - (Procdesc.get_attributes proc_desc).ProcAttributes.method_annotation.return + Language.curr_language_is Java && check_suppress (Procdesc.get_attributes proc_desc) issue_type in if should_suppress_lint || is_java_generated_method || is_java_external_package then Logging.debug Analysis Medium "Reporting is suppressed!@\n" (* Skip the reporting *) @@ -79,55 +147,4 @@ let log_issue_external procname ~issue_log ?severity_override ~loc ~ltr ?access issue_log -let is_suppressed ?(field_name = None) tenv proc_attributes kind = - let lookup = Tenv.lookup tenv in - (* Errors can be suppressed with annotations. An error of kind CHECKER_ERROR_NAME can be - suppressed with the following annotations: - - @android.annotation.SuppressLint("checker-error-name") - - @some.PrefixErrorName - where the kind matching is case - insensitive and ignores '-' and '_' characters. *) - let annotation_matches (a : Annot.t) = - let normalize str = Str.global_replace (Str.regexp "[_-]") "" (String.lowercase str) in - let drop_prefix str = Str.replace_first (Str.regexp "^[A-Za-z]+_") "" str in - let normalized_equal s1 a2 = - Annot.( - has_matching_str_value a2.value ~pred:(fun s -> String.equal (normalize s1) (normalize s))) - in - let is_parameter_suppressed () = - String.is_suffix a.class_name ~suffix:Annotations.suppress_lint - && List.exists ~f:(normalized_equal kind.IssueType.unique_id) a.parameters - in - let is_annotation_suppressed () = - String.is_suffix - ~suffix:(normalize (drop_prefix kind.IssueType.unique_id)) - (normalize a.class_name) - in - is_parameter_suppressed () || is_annotation_suppressed () - in - let is_method_suppressed () = - Annotations.ma_has_annotation_with proc_attributes.ProcAttributes.method_annotation - annotation_matches - in - let is_field_suppressed () = - match (field_name, PatternMatch.get_this_type_nonstatic_methods_only proc_attributes) with - | Some field_name, Some t -> ( - match Struct.get_field_type_and_annotation ~lookup field_name t with - | Some (_, ia) -> - Annotations.ia_has_annotation_with ia annotation_matches - | None -> - false ) - | _ -> - false - in - let is_class_suppressed () = - match PatternMatch.get_this_type_nonstatic_methods_only proc_attributes with - | Some t -> ( - match PatternMatch.type_get_annotation tenv t with - | Some ia -> - Annotations.ia_has_annotation_with ia annotation_matches - | None -> - false ) - | None -> - false - in - is_method_suppressed () || is_field_suppressed () || is_class_suppressed () +let is_suppressed = Suppression.is_suppressed diff --git a/infer/src/base/Config.ml b/infer/src/base/Config.ml index 94848e39a..f68581afc 100644 --- a/infer/src/base/Config.ml +++ b/infer/src/base/Config.ml @@ -2205,6 +2205,13 @@ and starvation_whole_program = "Run whole-program starvation analysis" +and suppress_lint_ignore_types = + CLOpt.mk_bool ~long:"suppress-lint-ignore-types" ~default:false + "[DEPRECATED] Check only the presence of @SuppressLint but not the issues types specified as \ + parameters to the annotations when deciding to suppress issues. Use for backwards \ + compatibility only!" + + and sqlite_cache_size = CLOpt.mk_int ~long:"sqlite-cache-size" ~default:2000 ~in_help: @@ -3215,6 +3222,8 @@ and subtype_multirange = !subtype_multirange and summaries_caches_max_size = !summaries_caches_max_size +and suppress_lint_ignore_types = !suppress_lint_ignore_types + and custom_symbols = (* Convert symbol lists to regexps just once, here *) match !custom_symbols with diff --git a/infer/src/base/Config.mli b/infer/src/base/Config.mli index 039640ca2..581acd88e 100644 --- a/infer/src/base/Config.mli +++ b/infer/src/base/Config.mli @@ -577,6 +577,8 @@ val subtype_multirange : bool val summaries_caches_max_size : int +val suppress_lint_ignore_types : bool + val symops_per_iteration : int option val test_determinator : bool diff --git a/infer/tests/codetoanalyze/java/biabduction/SuppressLintExample.java b/infer/tests/codetoanalyze/java/biabduction/SuppressLintExample.java index 246acfcbb..804e88c28 100644 --- a/infer/tests/codetoanalyze/java/biabduction/SuppressLintExample.java +++ b/infer/tests/codetoanalyze/java/biabduction/SuppressLintExample.java @@ -30,7 +30,7 @@ class SuppressAllWarnigsInTheClass { public class SuppressLintExample { - @SuppressLint("Suppressing by warning type not supported yet") + @SuppressLint("null-dereference") SuppressLintExample() { Object object = null; object.toString(); @@ -41,7 +41,7 @@ public class SuppressLintExample { object.toString(); } - @SuppressLint("Suppressing by warning type not supported yet") + @SuppressLint("null-dereference") void shouldNotReportNPE() { Object object = null; object.toString(); diff --git a/infer/tests/codetoanalyze/java/checkers/issues.exp b/infer/tests/codetoanalyze/java/checkers/issues.exp index a538fc7cb..92eedaf31 100644 --- a/infer/tests/codetoanalyze/java/checkers/issues.exp +++ b/infer/tests/codetoanalyze/java/checkers/issues.exp @@ -4,6 +4,7 @@ codetoanalyze/java/checkers/FragmentRetainsViewExample.java, codetoanalyze.java. codetoanalyze/java/checkers/ImmutableCast.java, codetoanalyze.java.checkers.ImmutableCast.badCast(com.google.common.collect.ImmutableList):java.util.List, 0, CHECKERS_IMMUTABLE_CAST, no_bucket, WARNING, [Method badCast(...) returns class com.google.common.collect.ImmutableList but the return type is class java.util.List. Make sure that users of this method do not try to modify the collection.], ImmutableCast, codetoanalyze.java.checkers codetoanalyze/java/checkers/ImmutableCast.java, codetoanalyze.java.checkers.ImmutableCast.badCastFromField():java.util.List, 0, CHECKERS_IMMUTABLE_CAST, no_bucket, WARNING, [Method badCastFromField() returns class com.google.common.collect.ImmutableList but the return type is class java.util.List. Make sure that users of this method do not try to modify the collection.], ImmutableCast, codetoanalyze.java.checkers codetoanalyze/java/checkers/PrintfArgsChecker.java, codetoanalyze.java.checkers.PrintfArgsChecker.formatStringIsNotLiteral(java.io.PrintStream):void, 2, CHECKERS_PRINTF_ARGS, no_bucket, ERROR, [] +codetoanalyze/java/checkers/PrintfArgsChecker.java, codetoanalyze.java.checkers.PrintfArgsChecker.notSuppressed(java.io.PrintStream):void, 1, CHECKERS_PRINTF_ARGS, no_bucket, ERROR, [] codetoanalyze/java/checkers/PrintfArgsChecker.java, codetoanalyze.java.checkers.PrintfArgsChecker.stringInsteadOfInteger(java.io.PrintStream):void, 1, CHECKERS_PRINTF_ARGS, no_bucket, ERROR, [] codetoanalyze/java/checkers/PrintfArgsChecker.java, codetoanalyze.java.checkers.PrintfArgsChecker.wrongNumberOfArguments(java.io.PrintStream):void, 1, CHECKERS_PRINTF_ARGS, no_bucket, ERROR, [] codetoanalyze/java/checkers/PrintfArgsChecker.java, codetoanalyze.java.checkers.SuppressedPrintfArgsChecker.classSuppressed(java.io.PrintStream):void, 1, CHECKERS_PRINTF_ARGS, no_bucket, ERROR, [] diff --git a/infer/tests/codetoanalyze/java/nullsafe/FieldNotInitialized.java b/infer/tests/codetoanalyze/java/nullsafe/FieldNotInitialized.java index 07b73a0f0..1eb010d78 100644 --- a/infer/tests/codetoanalyze/java/nullsafe/FieldNotInitialized.java +++ b/infer/tests/codetoanalyze/java/nullsafe/FieldNotInitialized.java @@ -207,11 +207,11 @@ public class FieldNotInitialized { f(null); // Expect to see "parameter not nullable" issue } - // Should suppress both field not initialized warning. - // But actually suppresses all nullsafe issues. + // Should suppress both field not initialized warning, + // but not the PARAMETER_NOT_NULLABLE @SuppressLint("eradicate-field-not-initialized") Suppressions(int a, int b) { - f(null); // FALSE NEGATIVE: this issue was unintentionally suppressed as well + f(null); } // This annotation correctly suppresses only needed issues diff --git a/infer/tests/codetoanalyze/java/nullsafe/SyntheticErrorSuppressions.java b/infer/tests/codetoanalyze/java/nullsafe/SyntheticErrorSuppressions.java index 951f168b3..3493eab1e 100644 --- a/infer/tests/codetoanalyze/java/nullsafe/SyntheticErrorSuppressions.java +++ b/infer/tests/codetoanalyze/java/nullsafe/SyntheticErrorSuppressions.java @@ -12,7 +12,7 @@ import javax.annotation.Nullable; public class SyntheticErrorSuppressions { static class Fragment { - @SuppressLint("ReturnOverAnnotated") + @SuppressLint("eradicate-return-over-annotated") @Nullable public static Object getContext() { return new Object(); diff --git a/infer/tests/codetoanalyze/java/nullsafe/issues.exp b/infer/tests/codetoanalyze/java/nullsafe/issues.exp index 383667bbd..a98f18c41 100644 --- a/infer/tests/codetoanalyze/java/nullsafe/issues.exp +++ b/infer/tests/codetoanalyze/java/nullsafe/issues.exp @@ -61,6 +61,7 @@ codetoanalyze/java/nullsafe/FieldNotInitialized.java, codetoanalyze.java.nullsaf codetoanalyze/java/nullsafe/FieldNotInitialized.java, codetoanalyze.java.nullsafe.FieldNotInitialized$Suppression.testNullifyFields():void, 7, ERADICATE_FIELD_NOT_NULLABLE, no_bucket, WARNING, [`suppressLintIsOK` is declared non-nullable but is assigned `null`: null constant at line 63.], FieldNotInitialized$Suppression, codetoanalyze.java.nullsafe codetoanalyze/java/nullsafe/FieldNotInitialized.java, codetoanalyze.java.nullsafe.FieldNotInitialized$Suppressions.(codetoanalyze.java.nullsafe.FieldNotInitialized,int), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `f2` is declared non-nullable, so it should be initialized in the constructor or in an `@Initializer` method], FieldNotInitialized$Suppressions, codetoanalyze.java.nullsafe codetoanalyze/java/nullsafe/FieldNotInitialized.java, codetoanalyze.java.nullsafe.FieldNotInitialized$Suppressions.(codetoanalyze.java.nullsafe.FieldNotInitialized,int), 2, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`FieldNotInitialized$Suppressions.f(...)`: parameter #1(`a`) is declared non-nullable but the argument is `null`.], FieldNotInitialized$Suppressions, codetoanalyze.java.nullsafe +codetoanalyze/java/nullsafe/FieldNotInitialized.java, codetoanalyze.java.nullsafe.FieldNotInitialized$Suppressions.(codetoanalyze.java.nullsafe.FieldNotInitialized,int,int), 1, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`FieldNotInitialized$Suppressions.f(...)`: parameter #1(`a`) is declared non-nullable but the argument is `null`.], FieldNotInitialized$Suppressions, codetoanalyze.java.nullsafe codetoanalyze/java/nullsafe/FieldNotInitialized.java, codetoanalyze.java.nullsafe.FieldNotInitialized$Suppressions.(codetoanalyze.java.nullsafe.FieldNotInitialized,int,int,int), 1, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`FieldNotInitialized$Suppressions.f(...)`: parameter #1(`a`) is declared non-nullable but the argument is `null`.], FieldNotInitialized$Suppressions, codetoanalyze.java.nullsafe codetoanalyze/java/nullsafe/FieldNotInitialized.java, codetoanalyze.java.nullsafe.FieldNotInitialized$WriteItselfIsBAD.(codetoanalyze.java.nullsafe.FieldNotInitialized), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `bad` is declared non-nullable, so it should be initialized in the constructor or in an `@Initializer` method], FieldNotInitialized$WriteItselfIsBAD, codetoanalyze.java.nullsafe codetoanalyze/java/nullsafe/FieldNotInitialized.java, codetoanalyze.java.nullsafe.TestInitializerAnnotation$TestFieldNotInitializedDerived.(codetoanalyze.java.nullsafe.TestInitializerAnnotation), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `field2_BAD` is declared non-nullable, so it should be initialized in the constructor or in an `@Initializer` method], TestInitializerAnnotation$TestFieldNotInitializedDerived, codetoanalyze.java.nullsafe