[java] Make override resolution consider parameter types

Summary:
Previously, _override resolution_ considered only the number of
arguments. This led to many FPs in nullsafe's _Inconsistent Subclass
Annotation_ check.

Current version also checks that argument types match.  However, we
still don't handle type parameters and erasure, so in this sense the
rules are incomplete.

Reviewed By: ngorogiannis, mityal

Differential Revision: D19393201

fbshipit-source-id: a0c75b8dd

infer/src/IR/Procname.ml

@@ -22,7 +22,7 @@ module Java = struct
   [@@deriving compare]
 
   (* TODO: use Mangled.t here *)
-  type java_type = Typ.Name.Java.Split.t [@@deriving compare]
+  type java_type = Typ.Name.Java.Split.t [@@deriving compare, equal]
 
   let java_void = Typ.Name.Java.Split.make "void"
 
@@ -209,11 +209,11 @@ module Parameter = struct
   (** Type for parameters in clang procnames, [Some name] means the parameter is of type pointer to
       struct, with [name] being the name of the struct, [None] means the parameter is of some other
       type. *)
-  type clang_parameter = Typ.Name.t option [@@deriving compare]
+  type clang_parameter = Typ.Name.t option [@@deriving compare, equal]
 
   (** Type for parameters in procnames, for java and clang. *)
   type t = JavaParameter of Java.java_type | ClangParameter of clang_parameter
-  [@@deriving compare]
+  [@@deriving compare, equal]
 
   let of_typ typ =
     match typ.Typ.desc with Typ.Tptr ({desc= Tstruct name}, Pk_pointer) -> Some name | _ -> None

infer/src/IR/Procname.mli

@@ -19,14 +19,12 @@ module Java : sig
 
   type t [@@deriving compare]
 
-  type java_type = Typ.Name.Java.Split.t
+  type java_type = Typ.Name.Java.Split.t [@@deriving compare, equal]
 
   val constructor_method_name : string
 
   val class_initializer_method_name : string
 
-  val compare_java_type : java_type -> java_type -> int
-
   val make : Typ.Name.t -> java_type option -> string -> java_type list -> kind -> t
   (** Create a Java procedure name from its class_name method_name args_type_name return_type_name
       method_kind. *)
@@ -104,11 +102,11 @@ module Parameter : sig
   (** Type for parameters in clang procnames, [Some name] means the parameter is of type pointer to
       struct, with [name] being the name of the struct, [None] means the parameter is of some other
       type. *)
-  type clang_parameter = Typ.Name.t option [@@deriving compare]
+  type clang_parameter = Typ.Name.t option [@@deriving compare, equal]
 
   (** Type for parameters in procnames, for java and clang. *)
   type t = JavaParameter of Java.java_type | ClangParameter of clang_parameter
-  [@@deriving compare]
+  [@@deriving compare, equal]
 
   val of_typ : Typ.t -> clang_parameter
 end

infer/src/IR/Typ.ml

@@ -364,7 +364,7 @@ let to_string typ =
 
 
 module Name = struct
-  type t = name [@@deriving compare]
+  type t = name [@@deriving compare, equal]
 
   let equal = [%compare.equal: t]
 
@@ -447,7 +447,7 @@ module Name = struct
       (** e.g. {type_name="int"; package=None} for primitive types
       * or {type_name="PrintWriter"; package=Some "java.io"} for objects.
       *)
-      type t = {package: string option; type_name: string} [@@deriving compare]
+      type t = {package: string option; type_name: string} [@@deriving compare, equal]
 
       let make ?package type_name = {type_name; package}
 

infer/src/IR/Typ.mli

@@ -215,7 +215,7 @@ module Name : sig
 
   module Java : sig
     module Split : sig
-      type t [@@deriving compare]
+      type t [@@deriving compare, equal]
 
       val make : ?package:string -> string -> t
 

infer/src/absint/PatternMatch.ml

@@ -334,13 +334,20 @@ let proc_calls resolve_attributes pdesc filter : (Procname.t * ProcAttributes.t)
 
 
 let is_override_of proc_name =
-  let method_name = Procname.get_method proc_name in
-  let parameter_length = List.length (Procname.get_parameters proc_name) in
-  Staged.stage (fun pname ->
-      (not (Procname.is_constructor pname))
-      && String.equal (Procname.get_method pname) method_name
-      (* TODO (T32979782): match parameter types, taking subtyping and type erasure into account *)
-      && Int.equal (List.length (Procname.get_parameters pname)) parameter_length )
+  let sub_method_name = Procname.get_method proc_name in
+  let sub_params = Procname.get_parameters proc_name in
+  Staged.stage (fun super_pname ->
+      let super_method_name = Procname.get_method super_pname in
+      let super_params = Procname.get_parameters super_pname in
+      (not (Procname.is_constructor super_pname))
+      && String.equal super_method_name sub_method_name
+      (* Check that parameter types match exactly (no subtyping or what not). *)
+      &&
+      match List.for_all2 sub_params super_params ~f:Procname.Parameter.equal with
+      | List.Or_unequal_lengths.Ok res ->
+          res
+      | List.Or_unequal_lengths.Unequal_lengths ->
+          false )
 
 
 let override_find ?(check_current_type = true) f tenv proc_name =

infer/src/absint/PatternMatch.mli

@@ -152,3 +152,5 @@ val find_superclasses_with_attributes :
 (** find superclasss with attributes (e.g., [@ThreadSafe]), including current class*)
 
 val is_override_of : Procname.t -> (Procname.t -> bool) Staged.t
+(** For a given [procname] checks if it's an override of some other [procname]. NOTE: the
+    implementation is not complete with respect to Java's generics and type erasure. *)

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

@@ -30,6 +30,8 @@ interface Overloads {
   String overload(String arg1, int arg2);
 
   String overload(String arg1, String arg2);
+
+  void notOverload(@Nullable Object arg);
 }
 
 // Check return annotations
@@ -96,15 +98,14 @@ abstract class ArgValToValAndNullToNullOK implements VariousMethods {
 
 // Check overrides + overloads
 
-// This is 'good' cases (should be OK except 1 FP due to broken is_override logic)
+// These are 'good' cases with real overrides
 abstract class OverrideExistingCorrectlyOK implements Overloads {
-  // This is FP
   public String overload(int arg) {
     return "OK";
   }
 
   public String overload(@Nullable String arg) {
-    return arg;
+    return "OK";
   }
 
   public String overload(String arg1, int arg2) {
@@ -116,16 +117,20 @@ abstract class OverrideExistingCorrectlyOK implements Overloads {
   }
 }
 
-// These are FP cases that get reported due to broken is_override logic
-abstract class NoOverrideSinceDifferentTypesFP implements Overloads {
+abstract class NoOverrideSinceDifferentTypesOK implements Overloads {
   @Nullable
   public String overload(Object arg) {
-    return arg.toString();
+    return null;
   }
 
   public String overload(Double arg) {
     return arg.toString();
   }
+
+  // Although, String is a subtype of Object, this method is not an override
+  public void notOverload(String arg) {
+    return;
+  }
 }
 
 // This is just a smoke test to check that incorrect overrides of overloaded methods get reported

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

@@ -99,13 +99,7 @@ codetoanalyze/java/nullsafe-default/FieldOverAnnotated.java, codetoanalyze.java.
 codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.ArgNullToValBAD.nullableArg(java.lang.String):java.lang.String, 0, ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION, no_bucket, WARNING, [First parameter `arg` of method `ArgNullToValBAD.nullableArg(...)` is not `@Nullable` but is declared `@Nullable`in the parent class method `VariousMethods.nullableArg(...)`.]
 codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.ArgNullToValForInterfaceInAnotherFileBAD.implementInAnotherFile(java.lang.String):java.lang.String, 0, ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION, no_bucket, WARNING, [First parameter `s` of method `ArgNullToValForInterfaceInAnotherFileBAD.implementInAnotherFile(...)` is not `@Nullable` but is declared `@Nullable`in the parent class method `InconsistentSubclassAnnotationInterface.implementInAnotherFile(...)`.]
 codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.ExtendsExternalLibrary.externalMethod2(java.lang.Object):void, 0, ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION, no_bucket, WARNING, [First parameter `object` of method `ExtendsExternalLibrary.externalMethod2(...)` is not `@Nullable` but is declared `@Nullable`in the parent class method `SomeExternalClass.externalMethod2(...)`.]
-codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.NoOverrideSinceDifferentTypesFP.overload(java.lang.Double):java.lang.String, 0, ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION, no_bucket, WARNING, [First parameter `arg` of method `NoOverrideSinceDifferentTypesFP.overload(...)` is not `@Nullable` but is declared `@Nullable`in the parent class method `Overloads.overload(...)`.]
-codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.NoOverrideSinceDifferentTypesFP.overload(java.lang.Object):java.lang.String, 0, ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION, no_bucket, WARNING, [First parameter `arg` of method `NoOverrideSinceDifferentTypesFP.overload(...)` is not `@Nullable` but is declared `@Nullable`in the parent class method `Overloads.overload(...)`.]
-codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.NoOverrideSinceDifferentTypesFP.overload(java.lang.Object):java.lang.String, 0, ERADICATE_INCONSISTENT_SUBCLASS_RETURN_ANNOTATION, no_bucket, WARNING, [Method `NoOverrideSinceDifferentTypesFP.overload(...)` is annotated with `@Nullable` but overrides unannotated method `Overloads.overload(...)`.]
-codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.NoOverrideSinceDifferentTypesFP.overload(java.lang.Object):java.lang.String, 0, ERADICATE_RETURN_OVER_ANNOTATED, no_bucket, WARNING, [Method `overload(...)` is annotated with `@Nullable` but never returns null.]
 codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.OverloadExistingIncorrectBAD.overload(java.lang.String,java.lang.String):java.lang.String, 0, ERADICATE_INCONSISTENT_SUBCLASS_RETURN_ANNOTATION, no_bucket, WARNING, [Method `OverloadExistingIncorrectBAD.overload(...)` is annotated with `@Nullable` but overrides unannotated method `Overloads.overload(...)`.]
-codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.OverrideExistingCorrectlyOK.overload(int):java.lang.String, 0, ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION, no_bucket, WARNING, [First parameter `arg` of method `OverrideExistingCorrectlyOK.overload(...)` is not `@Nullable` but is declared `@Nullable`in the parent class method `Overloads.overload(...)`.]
-codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.OverrideExistingCorrectlyOK.overload(java.lang.String):java.lang.String, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [`overload(...)`: return type is declared non-nullable but the method returns a nullable value: method parameter arg.]
 codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.ReturnValToNullBAD.valBoth(java.lang.String):java.lang.String, 0, ERADICATE_INCONSISTENT_SUBCLASS_RETURN_ANNOTATION, no_bucket, WARNING, [Method `ReturnValToNullBAD.valBoth(...)` is annotated with `@Nullable` but overrides unannotated method `VariousMethods.valBoth(...)`.]
 codetoanalyze/java/nullsafe-default/LibraryCalls.java, codetoanalyze.java.nullsafe_default.LibraryCalls.badAtomicReferenceDereference(java.util.concurrent.atomic.AtomicReference):java.lang.String, 0, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`ref.get()` is nullable and is not locally checked for null when calling `toString()`: call to AtomicReference.get() at line 35 (nullable according to nullsafe internal models)]
 codetoanalyze/java/nullsafe-default/LibraryCalls.java, codetoanalyze.java.nullsafe_default.LibraryCalls.badPhantomReferenceDereference(java.lang.ref.PhantomReference):java.lang.String, 0, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`ref.get()` is nullable and is not locally checked for null when calling `toString()`: call to PhantomReference.get() at line 27 (nullable according to nullsafe internal models)]