[nullsafe] 3rd party annotations from the repo are respected in nullsafe

Summary: Follow ups will include error messaging that makes the choice clear

Reviewed By: artempyanykh

Differential Revision: D18347664

fbshipit-source-id: b6f005726

infer/src/IR/Typ.ml

@@ -764,6 +764,8 @@ module Procname = struct
       ; kind= Static }
 
 
+    let is_constructor {method_name} = String.equal method_name constructor_method_name
+
     let is_anonymous_inner_class_constructor {class_name} =
       Name.Java.is_anonymous_inner_class_name class_name
 

infer/src/IR/Typ.mli

@@ -356,13 +356,13 @@ module Procname : sig
     (** Replace the method of a java procname. *)
 
     val get_class_name : t -> string
-    (** Return the class name of a java procedure name. *)
+    (** Return the fully qualified class name of a java procedure name (package + class name) *)
 
     val get_class_type_name : t -> Name.t
     (** Return the class name as a typename of a java procedure name. *)
 
     val get_simple_class_name : t -> string
-    (** Return the simple class name of a java procedure name. *)
+    (** Return the simple class name of a java procedure name (i.e. name without the package info). *)
 
     val get_package : t -> string option
     (** Return the package name of a java procedure name. *)
@@ -376,6 +376,9 @@ module Procname : sig
     val get_return_typ : t -> typ
     (** Return the return type of [pname_java]. return Tvoid if there's no return type *)
 
+    val is_constructor : t -> bool
+    (** Whether the method is constructor *)
+
     val is_access_method : t -> bool
     (** Check if the procedure name is an acess method (e.g. access$100 used to
           access private members from a nested class. *)

infer/src/nullsafe/ThirdPartyMethod.ml

@@ -48,6 +48,29 @@ let string_of_parsing_error = function
 
 let pp_unique_repr fmt signature = Sexp.pp fmt (sexp_of_unique_repr signature)
 
+let java_type_to_string java_type =
+  let package = Typ.Name.Java.Split.package java_type in
+  let type_name = Typ.Name.Java.Split.type_name java_type in
+  match package with
+  | None ->
+      (* Primitive type *)
+      type_name
+  | Some package ->
+      package ^ "." ^ type_name
+
+
+let unique_repr_of_java_proc_name java_proc_name =
+  let class_name = Typ.Procname.Java.get_class_name java_proc_name in
+  let method_name =
+    if Typ.Procname.Java.is_constructor java_proc_name then Constructor
+    else Method (Typ.Procname.Java.get_method java_proc_name)
+  in
+  let param_types =
+    Typ.Procname.Java.get_parameters java_proc_name |> List.map ~f:java_type_to_string
+  in
+  {class_name; method_name; param_types}
+
+
 let pp_nullability fmt nullability = Sexp.pp fmt (sexp_of_nullability nullability)
 
 let nullable_annotation = "@Nullable"

infer/src/nullsafe/ThirdPartyMethod.mli

@@ -27,6 +27,8 @@ type unique_repr =
 
 and method_name = Constructor | Method of string
 
+val unique_repr_of_java_proc_name : Typ.Procname.Java.t -> unique_repr
+
 val pp_unique_repr : Format.formatter -> unique_repr -> unit
 
 type nullability = {ret_nullability: type_nullability; param_nullability: type_nullability list}

infer/src/nullsafe/models.ml

@@ -44,7 +44,26 @@ let get_modelled_annotated_signature_for_biabduction proc_attributes =
   annotated_signature |> lookup_models_nullable
 
 
-(** Return the annotated signature of the procedure, taking into account models. *)
+let get_unique_repr proc_name =
+  let java_proc_name =
+    match proc_name with Typ.Procname.Java java_proc_name -> Some java_proc_name | _ -> None
+  in
+  Option.map java_proc_name ~f:ThirdPartyMethod.unique_repr_of_java_proc_name
+
+
+let to_modelled_nullability ThirdPartyMethod.{ret_nullability; param_nullability} =
+  let is_nullable = function
+    | ThirdPartyMethod.Nullable ->
+        true
+    | ThirdPartyMethod.Nonnull ->
+        false
+  in
+  (is_nullable ret_nullability, List.map param_nullability ~f:is_nullable)
+
+
+(** Return the annotated signature of the procedure, taking into account models.
+    External models take precedence over internal ones.
+ *)
 let get_modelled_annotated_signature tenv proc_attributes =
   let proc_name = proc_attributes.ProcAttributes.proc_name in
   let is_strict_mode =
@@ -52,13 +71,28 @@ let get_modelled_annotated_signature tenv proc_attributes =
   in
   let annotated_signature = AnnotatedSignature.get ~is_strict_mode proc_attributes in
   let proc_id = Typ.Procname.to_unique_id proc_name in
-  let lookup_models_nullable ann_sig =
+  (* Look in the infer internal models *)
+  let correct_by_internal_models ann_sig =
     try
       let modelled_nullability = Hashtbl.find annotated_table_nullability proc_id in
       AnnotatedSignature.set_modelled_nullability proc_name ann_sig modelled_nullability
     with Caml.Not_found -> ann_sig
   in
-  annotated_signature |> lookup_models_nullable
+  (* Look at external models *)
+  let correct_by_external_models ann_sig =
+    get_unique_repr proc_name
+    |> Option.bind
+         ~f:
+           (ThirdPartyAnnotationInfo.find_nullability_info
+              (ThirdPartyAnnotationGlobalRepo.get_repo ()))
+    |> Option.map ~f:to_modelled_nullability
+    |> Option.value_map
+       (* If we found information in third-party repo, overwrite annotated signature *)
+         ~f:(AnnotatedSignature.set_modelled_nullability proc_name ann_sig)
+         ~default:ann_sig
+  in
+  (* External models overwrite internal ones *)
+  annotated_signature |> correct_by_internal_models |> correct_by_external_models
 
 
 (** Return true when the procedure has been modelled for nullability. *)

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

@@ -3,5 +3,5 @@
     "external."
   ],
   "nullsafe-strict-containers": true,
-  "nullsafe-third-party-signatures": "third-party-annots"
+  "nullsafe-third-party-signatures": "third-party-signatures"
 }

infer/tests/codetoanalyze/java/nullsafe-default/Makefile

@@ -12,6 +12,6 @@ INFER_OPTIONS = \
   --eradicate-condition-redundant \
   --debug-exceptions
 INFERPRINT_OPTIONS = --issues-tests
-SOURCES = $(wildcard *.java) $(wildcard $(TESTS_DIR)/external/library/*.java)
+SOURCES = $(wildcard third-party-test-code/some/test/pckg/*.java) $(wildcard *.java) $(wildcard $(TESTS_DIR)/external/library/*.java)
 
 include $(TESTS_DIR)/javac.make

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

@@ -0,0 +1,78 @@
+/*
+ * 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 com.facebook.infer.annotation.NullsafeStrict;
+import javax.annotation.Nullable;
+import some.test.pckg.ThirdPartyTestClass;
+
+/**
+ * In this test, we test how Strict mode works for calls of 3rd party libraries, and how detection
+ * differs based on if the function is whitelisted or not in 3rd party signatures repository.
+ */
+@NullsafeStrict
+public class StrictModeForThirdParty {
+
+  ThirdPartyTestClass obj;
+
+  StrictModeForThirdParty() {
+    obj = new ThirdPartyTestClass();
+  }
+
+  public @Nullable String getNullable() {
+    return null;
+  }
+
+  public String getNonnull() {
+    return "";
+  }
+
+  // Return values.
+  // In strict mode, return values should be pessimistically treated as nullable
+  // if the function is unspecified, and treated according to their return annotation if
+  // the function is whitelisted in the 3rd party repo.
+
+  public void dereferenceUnspecifiedIsBAD() {
+    obj.returnUnspecified().toString();
+  }
+
+  public void dereferenceSpecifiedAsNullableIsBAD() {
+    obj.returnSpecifiedAsNullable().toString();
+  }
+
+  public void dereferenceSpecifiedAsNonnullIsOK() {
+    obj.returnSpecifiedAsNonnull().toString();
+  }
+
+  // Params.
+  // In strict mode, params should be pessimistically treated as non-nullable if the function is
+  // unspecified,
+  // and treated based on their annotation if the function is whitelisted in the 3rd party repo.
+
+  public void passingNullableParamToUnspecifiedIsBAD() {
+    obj.paramUnspecified(getNullable());
+  }
+
+  public void passingNonnullParamToUnspecifiedIsOK() {
+    obj.paramUnspecified(getNonnull());
+  }
+
+  public void passingNullableToParamSpecifiedAsNonnullIsBAD() {
+    obj.secondParamSpecifiedAsNonnull(getNonnull(), getNullable());
+  }
+
+  public void passingNullableToParamSpecifiedAsNullableIsOK() {
+    // first param is explicitly whitelisted as specified as nullable, so everything is OK
+    obj.secondParamSpecifiedAsNonnull(getNullable(), getNonnull());
+  }
+
+  public void passingNonnullToParamIsOK() {
+    // Independently of param signature, it is safe to pass non-nullables
+    obj.secondParamSpecifiedAsNonnull(getNonnull(), getNonnull());
+  }
+}

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

@@ -235,3 +235,8 @@ codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe
 codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.strictClass_dereferenceNullableFieldIsBad():void, 1, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`__new(...).nullable` is nullable and is not locally checked for null when calling `toString()`. (Origin: field OtherStrict.nullable at line 89)]
 codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.strictClass_dereferenceNullableMethodIsBad():void, 1, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`__new(...).getNullable()` is nullable and is not locally checked for null when calling `toString()`. (Origin: call to getNullable() at line 73)]
 codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.strictClass_dereferenceNullableStaticMethodIsBad():void, 1, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`staticNullable()` is nullable and is not locally checked for null when calling `toString()`. (Origin: call to staticNullable() at line 81)]
+codetoanalyze/java/nullsafe-default/StrictModeForThirdParty.java, codetoanalyze.java.nullsafe_default.StrictModeForThirdParty.dereferenceSpecifiedAsNullableIsBAD():void, 1, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`StrictModeForThirdParty.obj.returnSpecifiedAsNullable()` is nullable and is not locally checked for null when calling `toString()`. (Origin: call to returnSpecifiedAsNullable() at line 45)]
+codetoanalyze/java/nullsafe-default/StrictModeForThirdParty.java, codetoanalyze.java.nullsafe_default.StrictModeForThirdParty.dereferenceUnspecifiedIsBAD():void, 1, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`StrictModeForThirdParty.obj.returnUnspecified()` is nullable and is not locally checked for null when calling `toString()`. (Origin: call to returnUnspecified() at line 41)]
+codetoanalyze/java/nullsafe-default/StrictModeForThirdParty.java, codetoanalyze.java.nullsafe_default.StrictModeForThirdParty.passingNullableParamToUnspecifiedIsBAD():void, 1, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`ThirdPartyTestClass.paramUnspecified(...)` needs a non-null value in parameter 1 but argument `getNullable()` can be null. (Origin: call to getNullable() at line 58)]
+codetoanalyze/java/nullsafe-default/StrictModeForThirdParty.java, codetoanalyze.java.nullsafe_default.StrictModeForThirdParty.passingNullableToParamSpecifiedAsNonnullIsBAD():void, 1, ERADICATE_PARAMETER_NOT_NULLABLE, no_bucket, WARNING, [`ThirdPartyTestClass.secondParamSpecifiedAsNonnull(...)` needs a non-null value in parameter 2 but argument `getNullable()` can be null. (Origin: call to getNullable() at line 66)]
+codetoanalyze/java/nullsafe-default/third-party-test-code/some/test/pckg/ThirdPartyTestClass.java, some.test.pckg.ThirdPartyTestClass.returnSpecifiedAsNullable():java.lang.String, 0, ERADICATE_RETURN_OVER_ANNOTATED, no_bucket, WARNING, [Method `returnSpecifiedAsNullable()` is annotated with `@Nullable` but never returns null.]

infer/tests/codetoanalyze/java/nullsafe-default/third-party-annots/some.test.pckg.sig

@@ -1,2 +0,0 @@
-some.test.pckg.SomeClass#getNullable() @Nullable
-some.test.pckg.SomeClass#getNonnull()

infer/tests/codetoanalyze/java/nullsafe-default/third-party-signatures/some.test.pckg.sig

@@ -0,0 +1,3 @@
+some.test.pckg.ThirdPartyTestClass#returnSpecifiedAsNonnull()
+some.test.pckg.ThirdPartyTestClass#returnSpecifiedAsNullable() @Nullable
+some.test.pckg.ThirdPartyTestClass#secondParamSpecifiedAsNonnull(@Nullable java.lang.String, java.lang.String)

infer/tests/codetoanalyze/java/nullsafe-default/third-party-test-code/some/test/pckg/ThirdPartyTestClass.java

@@ -0,0 +1,39 @@
+/*
+ * 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 some.test.pckg;
+
+/**
+ * A test third party class. We specify its annotations outside of this class, in a third-party
+ * repository.
+ */
+public class ThirdPartyTestClass {
+
+  // Return values.
+
+  // No information in 3rd party repo
+  public String returnUnspecified() {
+    return "";
+  }
+
+  // 3rd party repo whitelists this function as returning non-nullable
+  public String returnSpecifiedAsNonnull() {
+    return "";
+  }
+
+  // 3rd party repo whitelists this function as returning nullable
+  public String returnSpecifiedAsNullable() {
+    return "";
+  }
+
+  // Params.
+
+  // No information about this function in 3rd party repo
+  public void paramUnspecified(String param) {}
+
+  public void secondParamSpecifiedAsNonnull(
+      String specifiedAsNullable, String specifiedAsNonnull) {}
+}