[nullsafe] Error reporting: recommend non-nullable alternarives for known nullable methods

Summary:
This will help making error reporting more actionable.

Often methods that are nullable in general (like View.findViewById) are used as not-nullable due to app-invariants. In such cases suggesting a non-nullable alternative that does an assertion under the hood makes the error report more actionable and provides necessary guidance with respect to coding best practices

Follow up will include adding more methods to models.
If this goes well, we might support it in user-defined area (nullability
repository)

Reviewed By: artempyanykh

Differential Revision: D20001416

fbshipit-source-id: 46f03467c

infer/models/java/src/android/view/View.java

@@ -0,0 +1,19 @@
+/*
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+package android.view;
+
+public class View {
+  public <T extends View> T findViewById(int id) {
+    // STUB: we need signature for tests; implementation is not tested
+    return null;
+  }
+
+  public void setId(int id) {
+    // STUB: we need signature for tests; implementation is not tested
+  }
+}

infer/src/nullsafe/AssignmentRule.ml

@@ -65,7 +65,7 @@ let pp_param_name fmt mangled =
 
 let mk_description_for_bad_param_passed
     {model_source; param_signature; actual_param_expression; param_position; function_procname}
-    ~param_nullability nullability_evidence =
+    ~param_nullability ~nullability_evidence =
   let nullability_evidence_as_suffix =
     Option.value_map nullability_evidence ~f:(fun evidence -> ": " ^ evidence) ~default:""
   in
@@ -161,11 +161,22 @@ let violation_description {nullsafe_mode; rhs} ~assignment_location assignment_t
         Option.value_map nullability_evidence ~f:(fun evidence -> ": " ^ evidence) ~default:""
       in
       let module MF = MarkupFormatter in
+      let alternative_method_description =
+        ErrorRenderingUtils.find_alternative_nonnull_method_description rhs_origin
+      in
+      let alternative_recommendation =
+        Option.value_map alternative_method_description
+          ~f:(fun descr ->
+            Format.asprintf " If you don't expect null, use %a instead." MF.pp_monospaced descr )
+          ~default:""
+      in
       let error_message =
         match assignment_type with
         | PassingParamToFunction function_info ->
-            mk_description_for_bad_param_passed function_info nullability_evidence
-              ~param_nullability:rhs
+            Format.sprintf "%s%s"
+              (mk_description_for_bad_param_passed function_info ~nullability_evidence
+                 ~param_nullability:rhs)
+              alternative_recommendation
         | AssigningToField field_name ->
             let rhs_description =
               Nullability.(
@@ -180,9 +191,9 @@ let violation_description {nullsafe_mode; rhs} ~assignment_location assignment_t
                        nullability %a"
                       Nullability.pp other)
             in
-            Format.asprintf "%a is declared non-nullable but is assigned %s%s." MF.pp_monospaced
+            Format.asprintf "%a is declared non-nullable but is assigned %s%s.%s" MF.pp_monospaced
               (Fieldname.get_field_name field_name)
-              rhs_description nullability_evidence_as_suffix
+              rhs_description nullability_evidence_as_suffix alternative_recommendation
         | ReturningFromFunction function_proc_name ->
             let return_description =
               Nullability.(
@@ -198,10 +209,11 @@ let violation_description {nullsafe_mode; rhs} ~assignment_location assignment_t
                        nullability %a"
                       Nullability.pp other)
             in
-            Format.asprintf "%a: return type is declared non-nullable but the method returns %s%s."
+            Format.asprintf
+              "%a: return type is declared non-nullable but the method returns %s%s.%s"
               MF.pp_monospaced
               (Procname.to_simplified_string ~withclass:false function_proc_name)
-              return_description nullability_evidence_as_suffix
+              return_description nullability_evidence_as_suffix alternative_recommendation
       in
       let issue_type = get_issue_type assignment_type in
       (error_message, issue_type, assignment_location)

infer/src/nullsafe/DereferenceRule.ml

@@ -79,21 +79,30 @@ let violation_description {nullsafe_mode; nullability} ~dereference_location der
         | ArrayLengthAccess ->
             "accessing its length"
       in
-      let suffix =
+      let origin_descr =
         get_origin_opt ~nullable_object_descr nullable_object_origin
         |> Option.bind ~f:(fun origin -> TypeOrigin.get_description origin)
         |> Option.value_map ~f:(fun origin -> ": " ^ origin) ~default:""
       in
+      let alternative_method_description =
+        ErrorRenderingUtils.find_alternative_nonnull_method_description nullable_object_origin
+      in
+      let alternative_recommendation =
+        Option.value_map alternative_method_description
+          ~f:(fun descr ->
+            Format.asprintf " If this is intentional, use %a instead." MF.pp_monospaced descr )
+          ~default:""
+      in
       let description =
         match nullability with
         | Nullability.Null ->
             Format.sprintf
               "NullPointerException will be thrown at this line! %s is `null` and is dereferenced \
                via %s%s."
-              what_is_dereferred_str action_descr suffix
+              what_is_dereferred_str action_descr origin_descr
         | Nullability.Nullable ->
-            Format.sprintf "%s is nullable and is not locally checked for null when %s%s."
-              what_is_dereferred_str action_descr suffix
+            Format.sprintf "%s is nullable and is not locally checked for null when %s%s.%s"
+              what_is_dereferred_str action_descr origin_descr alternative_recommendation
         | other ->
             Logging.die InternalError
               "violation_description:: invariant violation: unexpected nullability %a"

infer/src/nullsafe/ErrorRenderingUtils.ml

@@ -215,3 +215,20 @@ let mk_special_nullsafe_issue ~nullsafe_mode ~bad_nullability ~bad_usage_locatio
       bad_usage_location.Location.line recommendation
   in
   (description, issue_type, object_loc)
+
+
+let find_alternative_nonnull_method_description nullable_origin =
+  let open IOption.Let_syntax in
+  match nullable_origin with
+  | TypeOrigin.MethodCall {pname= Procname.Java java_pname as pname} ->
+      let* ModelTables.{package_name; class_name; method_name} =
+        Models.find_nonnullable_alternative pname
+      in
+      let+ original_package_name = Procname.Java.get_package java_pname in
+      if String.equal original_package_name package_name then
+        (* The same package that is from origin - omit name for simplicity *)
+        class_name ^ "." ^ method_name ^ "()"
+      else (* Fully qualified name *)
+        package_name ^ "." ^ class_name ^ "." ^ method_name ^ "()"
+  | _ ->
+      None

infer/src/nullsafe/ErrorRenderingUtils.mli

@@ -23,3 +23,8 @@ val mk_special_nullsafe_issue :
     strict and local mode. Returns a tuple (error message, issue type, error location). NOTE:
     Location of the error will be NOT in the place when the value is used (that is
     [bad_usage_location]), but where the value is first obtained from. *)
+
+val find_alternative_nonnull_method_description : TypeOrigin.t -> string option
+(** If type origin is the result of a nullable method call that have a known nonnullable alternative
+    (the one that does the check inside), return the string representation of that alternative
+    suitable for error messaging. *)

infer/src/nullsafe/modelTables.ml

@@ -224,12 +224,33 @@ let mapPut_list =
   ; (cp, "java.util.Map.put(java.lang.Object,java.lang.Object):java.lang.Object") ]
 
 
-(** Models for nullability *)
-let annotated_list_nullability =
-  check_not_null_list @ check_state_list @ check_argument_list @ true_on_null_list
-  @ [ ( o1
-      , "android.text.SpannableString.valueOf(java.lang.CharSequence):android.text.SpannableString"
-      )
+type nonnull_alternative_method = {package_name: string; class_name: string; method_name: string}
+
+(* Nullable methods that have non-nullable signatures. 
+   Format is a triple: (<nullability>, <method>, <alternative>), *)
+let nullable_methods_with_nonnull_alternatives_list =
+  [ ( (n, [o])
+    , "android.view.View.findViewById(int):android.view.View"
+    , {package_name= "android.view"; class_name= "View"; method_name= "requireViewById"} )
+  ; ( (n, [o])
+    , "android.app.Activity.findViewById(int):android.view.View"
+    , {package_name= "android.app"; class_name= "Activity"; method_name= "requireViewById"} ) ]
+
+
+let nullable_method_with_nonnull_alternatives_nullability_list =
+  let result =
+    List.map nullable_methods_with_nonnull_alternatives_list
+      ~f:(fun (nullability, method_descr, _) -> (nullability, method_descr))
+  in
+  List.iter result ~f:(fun ((ret_nullability, _param_nullability), _) ->
+      if not (ret_nullability == n) then
+        Logging.die Logging.InternalError "Function is expected to be nullable" ) ;
+  result
+
+
+(* Methods with signatures that are not special enough to be described in one of lists above *)
+let annotated_list_nullability_other =
+  [ (o1, "android.text.SpannableString.valueOf(java.lang.CharSequence):android.text.SpannableString")
   ; (o1, "android.app.AlarmManager.cancel(android.app.PendingIntent):void")
   ; (o1, "android.net.Uri.parse(java.lang.String):android.net.Uri")
   ; (n1, "android.os.Parcel.writeList(java.util.List):void")
@@ -455,8 +476,7 @@ let annotated_list_nullability =
        javax.lang.model.element.Element):boolean" )
   ; ( o3
     , "javax.lang.model.util.Elements.overrides(javax.lang.model.element.ExecutableElement, \
-         javax.lang.model.element.ExecutableElement, javax.lang.model.element.TypeElement):boolean"
-      )
+       javax.lang.model.element.ExecutableElement, javax.lang.model.element.TypeElement):boolean" )
   ; ( o1
     , "javax.lang.model.util.Types.asElement(javax.lang.model.type.TypeMirror):javax.lang.model.element.Element"
     )
@@ -573,9 +593,8 @@ let annotated_list_nullability =
     , "com.sun.source.util.Trees.getOriginalType(javax.lang.model.type.ErrorType):javax.lang.model.type.TypeMirror"
     )
   ; ( (o, [o; o; o; o])
-      , "com.sun.source.util.Trees.printMessage(javax.tools.Diagnostic.Kind, \
-         java.lang.CharSequence, com.sun.source.tree.Tree, \
-         com.sun.source.tree.CompilationUnitTree):void" )
+    , "com.sun.source.util.Trees.printMessage(javax.tools.Diagnostic.Kind, java.lang.CharSequence, \
+       com.sun.source.tree.Tree, com.sun.source.tree.CompilationUnitTree):void" )
   ; ( o1
     , "com.sun.source.util.Trees.getLub(com.sun.source.tree.CatchTree):javax.lang.model.type.TypeMirror"
     )
@@ -593,6 +612,19 @@ let annotated_list_nullability =
   ; (ng, "java.util.concurrent.atomic.AtomicReference.get():java.lang.Object") ]
 
 
+(** Models for nullability *)
+let annotated_list_nullability =
+  let result =
+    check_not_null_list @ check_state_list @ check_argument_list @ true_on_null_list
+    @ nullable_method_with_nonnull_alternatives_nullability_list @ annotated_list_nullability_other
+  in
+  List.find_a_dup result ~compare:(fun (_, descr1) (_, descr2) -> String.compare descr1 descr2)
+  |> Option.iter ~f:(fun (_, duplicate_method_descr) ->
+         Logging.die Logging.InternalError "Nullability table contains a duplicate %s"
+           duplicate_method_descr ) ;
+  result
+
+
 (** Models for methods that do not return *)
 let noreturn_list = [((o, [o]), "java.lang.System.exit(int):void")]
 
@@ -621,3 +653,10 @@ let mapPut_table = mk_table mapPut_list
 let noreturn_table = mk_table noreturn_list
 
 let true_on_null_table = mk_table true_on_null_list
+
+let nonnull_alternatives_table =
+  let method_descr_to_alternative =
+    List.map nullable_methods_with_nonnull_alternatives_list
+      ~f:(fun (_, method_descr, alternative) -> (alternative, method_descr))
+  in
+  mk_table method_descr_to_alternative

infer/src/nullsafe/modelTables.mli

@@ -34,3 +34,12 @@ val mapPut_table : model_table_t
 val noreturn_table : model_table_t
 
 val true_on_null_table : model_table_t
+
+(** Used to describe a method complementary to a given one. Contains information needed for
+    reporting (hence does not describe the whole signature). *)
+type nonnull_alternative_method = {package_name: string; class_name: string; method_name: string}
+
+val nonnull_alternatives_table : (string, nonnull_alternative_method) Caml.Hashtbl.t
+(** The key is a string representation of a [@Nullable] method. The value is the description of
+    non-nullable alternative: a method does the same, but never returns null (does a null check
+    inside). *)

infer/src/nullsafe/models.ml

@@ -144,3 +144,11 @@ let is_containsKey proc_name = table_has_procedure containsKey_table proc_name
 
 (** Check if the procedure is Map.put(). *)
 let is_mapPut proc_name = table_has_procedure mapPut_table proc_name
+
+(** Check if a (nullable) method has a non-nullable alternative: A method that does the same as
+    [proc_name] but asserts the result is not null before returning to the caller. *)
+let find_nonnullable_alternative proc_name =
+  (* NOTE: For now we fetch this info from internal models.
+     It is a good idea to support this feature in a user-facing third party repository. *)
+  let proc_id = Procname.to_unique_id proc_name in
+  Hashtbl.find_opt nonnull_alternatives_table proc_id

infer/tests/codetoanalyze/java/nullsafe-default/AlternativeRecommendations.java

@@ -0,0 +1,38 @@
+/*
+ * Copyright (c) Facebook, Inc. and its affiliates.
+ *
+ * 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.nullsafe_default;
+
+import android.annotation.SuppressLint;
+import android.view.View;
+
+/**
+ * Test to ensure we have special messaging when misusing known nullable methods that have
+ * non-nullable alternatives.
+ */
+public class AlternativeRecommendations {
+  @SuppressLint("eradicate-field-not-initialized")
+  View field;
+
+  static void dereference_ShouldSuggestAlternative(View view) {
+    view.findViewById(2).setId(3);
+  }
+
+  static void passingParam_ShouldSuggestAlternative(View view) {
+    acceptsNonnullView(view.findViewById(2));
+  }
+
+  static View returnValue_ShouldSuggestAlternative(View view) {
+    return view.findViewById(2);
+  }
+
+  void assigningField_ShouldSuggestAlternative(View view) {
+    field = view.findViewById(2);
+  }
+
+  static void acceptsNonnullView(View view) {}
+}

infer/tests/codetoanalyze/java/nullsafe-default/issues.exp

@@ -1,3 +1,7 @@
+codetoanalyze/java/nullsafe-default/AlternativeRecommendations.java, codetoanalyze.java.nullsafe_default.AlternativeRecommendations.assigningField_ShouldSuggestAlternative(android.view.View):void, 0, ERADICATE_FIELD_NOT_NULLABLE, no_bucket, WARNING, [`field` is declared non-nullable but is assigned a nullable: call to View.findViewById(...) at line 34 (nullable according to nullsafe internal models). If you don't expect null, use `View.requireViewById()` instead.]
+codetoanalyze/java/nullsafe-default/AlternativeRecommendations.java, codetoanalyze.java.nullsafe_default.AlternativeRecommendations.dereference_ShouldSuggestAlternative(android.view.View):void, 0, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`view.findViewById(...)` is nullable and is not locally checked for null when calling `setId(...)`: call to View.findViewById(...) at line 22 (nullable according to nullsafe internal models). If this is intentional, use `View.requireViewById()` instead.]
+codetoanalyze/java/nullsafe-default/AlternativeRecommendations.java, codetoanalyze.java.nullsafe_default.AlternativeRecommendations.passingParam_ShouldSuggestAlternative(android.view.View):void, 0, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`AlternativeRecommendations.acceptsNonnullView(...)`: parameter #1(`view`) is declared non-nullable but the argument `view.findViewById(...)` is nullable: call to View.findViewById(...) at line 26 (nullable according to nullsafe internal models). If you don't expect null, use `View.requireViewById()` instead.]
+codetoanalyze/java/nullsafe-default/AlternativeRecommendations.java, codetoanalyze.java.nullsafe_default.AlternativeRecommendations.returnValue_ShouldSuggestAlternative(android.view.View):android.view.View, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [`returnValue_ShouldSuggestAlternative(...)`: return type is declared non-nullable but the method returns a nullable value: call to View.findViewById(...) at line 30 (nullable according to nullsafe internal models). If you don't expect null, use `View.requireViewById()` instead.]
 codetoanalyze/java/nullsafe-default/ButterKnife.java, codetoanalyze.java.nullsafe_default.ButterKnife$TestNotInitialized.<init>(codetoanalyze.java.nullsafe_default.ButterKnife), 0, ERADICATE_FIELD_NOT_INITIALIZED, no_bucket, WARNING, [Field `notInitializedNormalIsBAD` is declared non-nullable, so it should be initialized in the constructor or in an `@Initializer` method]
 codetoanalyze/java/nullsafe-default/ButterKnife.java, codetoanalyze.java.nullsafe_default.ButterKnife.<init>(), 0, ERADICATE_FIELD_OVER_ANNOTATED, no_bucket, WARNING, [Field `ButterKnife.nullable` is always initialized in the constructor but is declared `@Nullable`]
 codetoanalyze/java/nullsafe-default/ButterKnife.java, codetoanalyze.java.nullsafe_default.ButterKnife.assignNullToNormalIsBAD():void, 0, ERADICATE_FIELD_NOT_NULLABLE, no_bucket, WARNING, [`normal` is declared non-nullable but is assigned `null`: null constant at line 76.]