[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

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

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

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

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

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} ->

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

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. *)

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 "";
+  }
 }

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