From 027ff479d1ffdb59dbd421b37f97741c0b75d6c7 Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Thu, 7 Nov 2019 04:12:44 -0800 Subject: [PATCH] [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 | 2 + infer/src/IR/Typ.mli | 7 +- infer/src/nullsafe/ThirdPartyMethod.ml | 23 ++++++ infer/src/nullsafe/ThirdPartyMethod.mli | 2 + infer/src/nullsafe/models.ml | 40 +++++++++- .../java/nullsafe-default/.inferconfig | 2 +- .../java/nullsafe-default/Makefile | 2 +- .../StrictModeForThirdParty.java | 78 +++++++++++++++++++ .../java/nullsafe-default/issues.exp | 5 ++ .../third-party-annots/some.test.pckg.sig | 2 - .../other.test.pckg.sig | 0 .../third-party-signatures/some.test.pckg.sig | 3 + .../some/test/pckg/ThirdPartyTestClass.java | 39 ++++++++++ 13 files changed, 196 insertions(+), 9 deletions(-) create mode 100644 infer/tests/codetoanalyze/java/nullsafe-default/StrictModeForThirdParty.java delete mode 100644 infer/tests/codetoanalyze/java/nullsafe-default/third-party-annots/some.test.pckg.sig rename infer/tests/codetoanalyze/java/nullsafe-default/{third-party-annots => third-party-signatures}/other.test.pckg.sig (100%) create mode 100644 infer/tests/codetoanalyze/java/nullsafe-default/third-party-signatures/some.test.pckg.sig create mode 100644 infer/tests/codetoanalyze/java/nullsafe-default/third-party-test-code/some/test/pckg/ThirdPartyTestClass.java diff --git a/infer/src/IR/Typ.ml b/infer/src/IR/Typ.ml index 7d7fe8f1a..2161589ef 100644 --- a/infer/src/IR/Typ.ml +++ b/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 diff --git a/infer/src/IR/Typ.mli b/infer/src/IR/Typ.mli index 808403376..9322f2b4a 100644 --- a/infer/src/IR/Typ.mli +++ b/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. *) diff --git a/infer/src/nullsafe/ThirdPartyMethod.ml b/infer/src/nullsafe/ThirdPartyMethod.ml index 6251d4d39..b2e2b9c72 100644 --- a/infer/src/nullsafe/ThirdPartyMethod.ml +++ b/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" diff --git a/infer/src/nullsafe/ThirdPartyMethod.mli b/infer/src/nullsafe/ThirdPartyMethod.mli index 1b17e32a7..d04d42382 100644 --- a/infer/src/nullsafe/ThirdPartyMethod.mli +++ b/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} diff --git a/infer/src/nullsafe/models.ml b/infer/src/nullsafe/models.ml index b6f975657..f164ca654 100644 --- a/infer/src/nullsafe/models.ml +++ b/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. *) diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/.inferconfig b/infer/tests/codetoanalyze/java/nullsafe-default/.inferconfig index 61cf77b67..95a493917 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/.inferconfig +++ b/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" } diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/Makefile b/infer/tests/codetoanalyze/java/nullsafe-default/Makefile index 881f0b3a1..2b8cd760d 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/Makefile +++ b/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 diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/StrictModeForThirdParty.java b/infer/tests/codetoanalyze/java/nullsafe-default/StrictModeForThirdParty.java new file mode 100644 index 000000000..f4702ea93 --- /dev/null +++ b/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()); + } +} diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp index a488b80dd..61254ccd7 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp +++ b/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.] diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/third-party-annots/some.test.pckg.sig b/infer/tests/codetoanalyze/java/nullsafe-default/third-party-annots/some.test.pckg.sig deleted file mode 100644 index c50e04614..000000000 --- a/infer/tests/codetoanalyze/java/nullsafe-default/third-party-annots/some.test.pckg.sig +++ /dev/null @@ -1,2 +0,0 @@ -some.test.pckg.SomeClass#getNullable() @Nullable -some.test.pckg.SomeClass#getNonnull() diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/third-party-annots/other.test.pckg.sig b/infer/tests/codetoanalyze/java/nullsafe-default/third-party-signatures/other.test.pckg.sig similarity index 100% rename from infer/tests/codetoanalyze/java/nullsafe-default/third-party-annots/other.test.pckg.sig rename to infer/tests/codetoanalyze/java/nullsafe-default/third-party-signatures/other.test.pckg.sig diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/third-party-signatures/some.test.pckg.sig b/infer/tests/codetoanalyze/java/nullsafe-default/third-party-signatures/some.test.pckg.sig new file mode 100644 index 000000000..639b5f5ac --- /dev/null +++ b/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) diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/third-party-test-code/some/test/pckg/ThirdPartyTestClass.java b/infer/tests/codetoanalyze/java/nullsafe-default/third-party-test-code/some/test/pckg/ThirdPartyTestClass.java new file mode 100644 index 000000000..5c0981058 --- /dev/null +++ b/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) {} +}