From 0f1187a3a39cd26e596dd0d698e18186c4e8ab85 Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Thu, 24 Oct 2019 06:47:28 -0700 Subject: [PATCH] [nullsafe] Make Strict mode respect static methods Summary: The wrong function was used when we tried to see if the class is annotated with NullsafeStrict. This made it work only for non-static methods. Now we use the proper way. Reviewed By: ngorogiannis Differential Revision: D18113848 fbshipit-source-id: 02b7555be --- infer/src/absint/PatternMatch.ml | 4 +- infer/src/absint/PatternMatch.mli | 4 +- infer/src/backend/reporting.ml | 4 +- infer/src/concurrency/starvation.ml | 2 +- infer/src/nullsafe/eradicateChecks.ml | 5 +- infer/src/nullsafe/models.ml | 5 +- infer/src/nullsafe/typeCheck.ml | 2 +- .../java/nullsafe-default/StrictMode.java | 49 +++++++++++++++++++ .../java/nullsafe-default/issues.exp | 28 ++++++----- 9 files changed, 78 insertions(+), 25 deletions(-) diff --git a/infer/src/absint/PatternMatch.ml b/infer/src/absint/PatternMatch.ml index 65afd3825..2577b5136 100644 --- a/infer/src/absint/PatternMatch.ml +++ b/infer/src/absint/PatternMatch.ml @@ -106,7 +106,7 @@ let implements_jackson class_name = implements ("com.fasterxml.jackson." ^ class let implements_org_json class_name = implements ("org.json." ^ class_name) (** The type the method is invoked on *) -let get_this_type proc_attributes = +let get_this_type_nonstatic_methods_only proc_attributes = match proc_attributes.ProcAttributes.formals with (_, t) :: _ -> Some t | _ -> None @@ -283,7 +283,7 @@ let type_has_initializer (tenv : Tenv.t) (t : Typ.t) : bool = (** Check if the method is one of the known initializer methods. *) let method_is_initializer (tenv : Tenv.t) (proc_attributes : ProcAttributes.t) : bool = - match get_this_type proc_attributes with + match get_this_type_nonstatic_methods_only proc_attributes with | Some this_type -> if type_has_initializer tenv this_type then match proc_attributes.ProcAttributes.proc_name with diff --git a/infer/src/absint/PatternMatch.mli b/infer/src/absint/PatternMatch.mli index 99ab84283..495c656c0 100644 --- a/infer/src/absint/PatternMatch.mli +++ b/infer/src/absint/PatternMatch.mli @@ -9,8 +9,8 @@ open! IStd (** Module for Pattern matching. *) -val get_this_type : ProcAttributes.t -> Typ.t option -(** Get the this type of a procedure *) +val get_this_type_nonstatic_methods_only : ProcAttributes.t -> Typ.t option +(** Get the `this` type of a procedure. Should not be called on non-static methods, otherwise it can return a wrong type *) val get_type_name : Typ.t -> string (** Get the name of a type *) diff --git a/infer/src/backend/reporting.ml b/infer/src/backend/reporting.ml index 051291683..fd69b9d33 100644 --- a/infer/src/backend/reporting.ml +++ b/infer/src/backend/reporting.ml @@ -143,7 +143,7 @@ let is_suppressed ?(field_name = None) tenv proc_desc kind = annotation_matches in let is_field_suppressed () = - match (field_name, PatternMatch.get_this_type proc_attributes) with + match (field_name, PatternMatch.get_this_type_nonstatic_methods_only proc_attributes) with | Some field_name, Some t -> ( match Typ.Struct.get_field_type_and_annotation ~lookup field_name t with | Some (_, ia) -> @@ -154,7 +154,7 @@ let is_suppressed ?(field_name = None) tenv proc_desc kind = false in let is_class_suppressed () = - match PatternMatch.get_this_type proc_attributes with + match PatternMatch.get_this_type_nonstatic_methods_only proc_attributes with | Some t -> ( match PatternMatch.type_get_annotation tenv t with | Some ia -> diff --git a/infer/src/concurrency/starvation.ml b/infer/src/concurrency/starvation.ml index 0b92d439f..045e30a81 100644 --- a/infer/src/concurrency/starvation.ml +++ b/infer/src/concurrency/starvation.ml @@ -25,7 +25,7 @@ let is_nonblocking tenv proc_desc = Annotations.pdesc_has_return_annot proc_desc Annotations.ia_is_nonblocking in let is_class_suppressed = - PatternMatch.get_this_type proc_attributes + PatternMatch.get_this_type_nonstatic_methods_only proc_attributes |> Option.bind ~f:(PatternMatch.type_get_annotation tenv) |> Option.exists ~f:Annotations.ia_is_nonblocking in diff --git a/infer/src/nullsafe/eradicateChecks.ml b/infer/src/nullsafe/eradicateChecks.ml index 8662fa283..5441c8daa 100644 --- a/infer/src/nullsafe/eradicateChecks.ml +++ b/infer/src/nullsafe/eradicateChecks.ml @@ -234,7 +234,10 @@ let check_constructor_initialization tenv find_canonical_duplicate curr_construc ~typestates_for_all_constructors_incl_current loc : unit = State.set_node start_node ; if Typ.Procname.is_constructor curr_constructor_pname then - match PatternMatch.get_this_type (Procdesc.get_attributes curr_constructor_pdesc) with + match + PatternMatch.get_this_type_nonstatic_methods_only + (Procdesc.get_attributes curr_constructor_pdesc) + with | Some {desc= Tptr (({desc= Tstruct name} as ts), _)} -> ( match Tenv.lookup tenv name with | Some {fields} -> diff --git a/infer/src/nullsafe/models.ml b/infer/src/nullsafe/models.ml index c7c9984d4..b6f975657 100644 --- a/infer/src/nullsafe/models.ml +++ b/infer/src/nullsafe/models.ml @@ -48,10 +48,7 @@ let get_modelled_annotated_signature_for_biabduction proc_attributes = let get_modelled_annotated_signature tenv proc_attributes = let proc_name = proc_attributes.ProcAttributes.proc_name in let is_strict_mode = - PatternMatch.get_this_type proc_attributes - |> Option.bind ~f:(fun this_class -> PatternMatch.type_get_annotation tenv this_class) - |> Option.exists ~f:(fun annotations_for_this -> - Annotations.ia_is_nullsafe_strict annotations_for_this ) + PatternMatch.check_current_class_attributes Annotations.ia_is_nullsafe_strict tenv proc_name in let annotated_signature = AnnotatedSignature.get ~is_strict_mode proc_attributes in let proc_id = Typ.Procname.to_unique_id proc_name in diff --git a/infer/src/nullsafe/typeCheck.ml b/infer/src/nullsafe/typeCheck.ml index 7827120a3..0c9e3f263 100644 --- a/infer/src/nullsafe/typeCheck.ml +++ b/infer/src/nullsafe/typeCheck.ml @@ -339,7 +339,7 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p let drop_unchecked_params calls_this proc_attributes params = let pname = proc_attributes.ProcAttributes.proc_name in if Typ.Procname.is_constructor pname then - match PatternMatch.get_this_type proc_attributes with + match PatternMatch.get_this_type_nonstatic_methods_only proc_attributes with | Some _ -> constructor_check_calls_this calls_this pname ; (* Drop reference parameters to this and outer objects. *) diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/StrictMode.java b/infer/tests/codetoanalyze/java/nullsafe-default/StrictMode.java index 498178050..7562ac44b 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/StrictMode.java +++ b/infer/tests/codetoanalyze/java/nullsafe-default/StrictMode.java @@ -25,6 +25,14 @@ class Strict { return nonnull; } + public static @Nullable String staticNullable() { + return null; + } + + public static String staticNonnull() { + return ""; + } + // 1. Inside the same class, we trust annotations. private void sameClass_dereferenceNullableMethodIsBad() { @@ -35,6 +43,14 @@ class Strict { getNonnull().toString(); } + private void sameClass_dereferenceNullableStaticMethodIsBad() { + staticNullable().toString(); + } + + private void sameClass_dereferenceNonnullStaticMethodIsOK() { + staticNonnull().toString(); + } + private void sameClass_dereferenceNullableFieldIsBad() { nullable.toString(); } @@ -61,6 +77,14 @@ class Strict { (new OtherStrict()).getNonnull().toString(); } + private void strictClass_dereferenceNullableStaticMethodIsBad() { + OtherStrict.staticNullable().toString(); + } + + private void strictClass_dereferenceNonnullStaticMethodIsOK() { + OtherStrict.staticNonnull().toString(); + } + private void strictClass_dereferenceNullableFieldIsBad() { (new OtherStrict()).nullable.toString(); } @@ -89,6 +113,15 @@ class Strict { (new NonStrict()).getNonnull().toString(); } + private void nonStrictClass_dereferenceNullableStaticMethodIsBad() { + NonStrict.staticNullable().toString(); + } + + private void nonStrictClass_dereferenceNonnullStaticMethodIsBad() { + // even that it is declared as nonnull, can not dereference it without checking before + NonStrict.staticNonnull().toString(); + } + private void nonStrictClass_dereferenceNullableFieldIsBad() { (new NonStrict()).nullable.toString(); } @@ -173,6 +206,14 @@ class OtherStrict { public String getNonnull() { return nonnull; } + + public static @Nullable String staticNullable() { + return null; + } + + public static String staticNonnull() { + return ""; + } } class NonStrict { @@ -186,4 +227,12 @@ class NonStrict { public String getNonnull() { return nonnull; } + + public static @Nullable String staticNullable() { + return null; + } + + public static String staticNonnull() { + return ""; + } } diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp index b8fdbeec7..8991c6eb1 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp +++ b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp @@ -216,18 +216,22 @@ codetoanalyze/java/nullsafe-default/ReturnNotNullable.java, codetoanalyze.java.n codetoanalyze/java/nullsafe-default/ReturnNotNullable.java, codetoanalyze.java.nullsafe_default.ReturnNotNullable.return_null_in_catch():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `return_null_in_catch()` may return null but it is not annotated with `@Nullable`. (Origin: null constant at line 160)] codetoanalyze/java/nullsafe-default/ReturnNotNullable.java, codetoanalyze.java.nullsafe_default.ReturnNotNullable.return_null_in_catch_after_throw():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `return_null_in_catch_after_throw()` may return null but it is not annotated with `@Nullable`. (Origin: null constant at line 172)] codetoanalyze/java/nullsafe-default/ReturnNotNullable.java, codetoanalyze.java.nullsafe_default.ReturnNotNullable.tryWithResourcesReturnNullable(java.lang.String):java.lang.Object, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `tryWithResourcesReturnNullable(...)` may return null but it is not annotated with `@Nullable`. (Origin: call to nullToNullableIsOK() at line 142)] -codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.nonStrictClass_convertingNonnullToNonnullIsBad():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `nonStrictClass_convertingNonnullToNonnullIsBad()` may return null but it is not annotated with `@Nullable`. (Origin: call to getNonnull() at line 107)] +codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.nonStrictClass_convertingNonnullToNonnullIsBad():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `nonStrictClass_convertingNonnullToNonnullIsBad()` may return null but it is not annotated with `@Nullable`. (Origin: call to getNonnull() at line 140)] codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.nonStrictClass_convertingNonnullToNullableIsOK():java.lang.String, 0, ERADICATE_RETURN_OVER_ANNOTATED, no_bucket, WARNING, [Method `nonStrictClass_convertingNonnullToNullableIsOK()` is annotated with `@Nullable` but never returns null.] -codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.nonStrictClass_convertingNullableToNonnullIsBad():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `nonStrictClass_convertingNullableToNonnullIsBad()` may return null but it is not annotated with `@Nullable`. (Origin: call to getNullable() at line 102)] +codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.nonStrictClass_convertingNullableToNonnullIsBad():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `nonStrictClass_convertingNullableToNonnullIsBad()` may return null but it is not annotated with `@Nullable`. (Origin: call to getNullable() at line 135)] codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.nonStrictClass_dereferenceNonnullFieldAfterCheckIsOK():void, 2, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition NonStrict.nonnull is always true according to the existing annotations.] -codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.nonStrictClass_dereferenceNonnullFieldIsBad():void, 2, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`__new(...).nonnull` is nullable and is not locally checked for null when calling `toString()`. (Origin: field NonStrict.nonnull at line 98)] +codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.nonStrictClass_dereferenceNonnullFieldIsBad():void, 2, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`__new(...).nonnull` is nullable and is not locally checked for null when calling `toString()`. (Origin: field NonStrict.nonnull at line 131)] codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.nonStrictClass_dereferenceNonnullMethodAfterCheckIsOK():void, 2, ERADICATE_CONDITION_REDUNDANT, no_bucket, WARNING, [The condition lang.String(o)V is always true according to the existing annotations.] -codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.nonStrictClass_dereferenceNonnullMethodIsBad():void, 2, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`__new(...).getNonnull()` is nullable and is not locally checked for null when calling `toString()`. (Origin: call to getNonnull() at line 89)] -codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.nonStrictClass_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 NonStrict.nullable at line 93)] -codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.nonStrictClass_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 84)] -codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.sameClass_convertingNullableToNonnullIsBad():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `sameClass_convertingNullableToNonnullIsBad()` may return null but it is not annotated with `@Nullable`. (Origin: call to getNullable() at line 47)] -codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.sameClass_dereferenceNullableFieldIsBad():void, 1, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`Strict.nullable` is nullable and is not locally checked for null when calling `toString()`. (Origin: field Strict.nullable at line 39)] -codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.sameClass_dereferenceNullableMethodIsBad():void, 1, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`getNullable()` is nullable and is not locally checked for null when calling `toString()`. (Origin: call to getNullable() at line 31)] -codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.strictClass_convertingNullableToNonnullIsBad():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `strictClass_convertingNullableToNonnullIsBad()` may return null but it is not annotated with `@Nullable`. (Origin: call to getNullable() at line 73)] -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 65)] -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 57)] +codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.nonStrictClass_dereferenceNonnullMethodIsBad():void, 2, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`__new(...).getNonnull()` is nullable and is not locally checked for null when calling `toString()`. (Origin: call to getNonnull() at line 113)] +codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.nonStrictClass_dereferenceNonnullStaticMethodIsBad():void, 2, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`staticNonnull()` is nullable and is not locally checked for null when calling `toString()`. (Origin: call to staticNonnull() at line 122)] +codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.nonStrictClass_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 NonStrict.nullable at line 126)] +codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.nonStrictClass_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 108)] +codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.nonStrictClass_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 117)] +codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.sameClass_convertingNullableToNonnullIsBad():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `sameClass_convertingNullableToNonnullIsBad()` may return null but it is not annotated with `@Nullable`. (Origin: call to getNullable() at line 63)] +codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.sameClass_dereferenceNullableFieldIsBad():void, 1, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`Strict.nullable` is nullable and is not locally checked for null when calling `toString()`. (Origin: field Strict.nullable at line 55)] +codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.sameClass_dereferenceNullableMethodIsBad():void, 1, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`getNullable()` is nullable and is not locally checked for null when calling `toString()`. (Origin: call to getNullable() at line 39)] +codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.sameClass_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 47)] +codetoanalyze/java/nullsafe-default/StrictMode.java, codetoanalyze.java.nullsafe_default.Strict.strictClass_convertingNullableToNonnullIsBad():java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [Method `strictClass_convertingNullableToNonnullIsBad()` may return null but it is not annotated with `@Nullable`. (Origin: call to getNullable() at line 97)] +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)]