[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

@@ -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)

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
+    Language.curr_language_is Java && check_suppress (Procdesc.get_attributes proc_desc) issue_type
-    && Annotations.ia_is_suppress_lint
-         (Procdesc.get_attributes proc_desc).ProcAttributes.method_annotation.return
   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 is_suppressed = Suppression.is_suppressed
-  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 ()

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

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

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();

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, []

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.
+    // Should suppress both field not initialized warning,
-    // But actually suppresses all nullsafe issues.
+    // 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

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();

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.<init>(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.<init>(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.<init>(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.<init>(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.<init>(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.<init>(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