From 592c746e6b06fb8e7bc1ed43628764f7dc4b40b2 Mon Sep 17 00:00:00 2001 From: Artem Pianykh Date: Tue, 14 Jan 2020 12:34:01 -0800 Subject: [PATCH] [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 | 6 +++--- infer/src/IR/Procname.mli | 8 +++---- infer/src/IR/Typ.ml | 4 ++-- infer/src/IR/Typ.mli | 2 +- infer/src/absint/PatternMatch.ml | 21 ++++++++++++------- infer/src/absint/PatternMatch.mli | 2 ++ .../InconsistentSubclassAnnotation.java | 17 +++++++++------ .../java/nullsafe-default/issues.exp | 6 ------ 8 files changed, 36 insertions(+), 30 deletions(-) diff --git a/infer/src/IR/Procname.ml b/infer/src/IR/Procname.ml index 6b6f151c6..cf9ac672f 100644 --- a/infer/src/IR/Procname.ml +++ b/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 diff --git a/infer/src/IR/Procname.mli b/infer/src/IR/Procname.mli index 2f9287910..2abcaa611 100644 --- a/infer/src/IR/Procname.mli +++ b/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 diff --git a/infer/src/IR/Typ.ml b/infer/src/IR/Typ.ml index 45b30b851..69ee4e359 100644 --- a/infer/src/IR/Typ.ml +++ b/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} diff --git a/infer/src/IR/Typ.mli b/infer/src/IR/Typ.mli index cc37caa72..8d9f043a1 100644 --- a/infer/src/IR/Typ.mli +++ b/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 diff --git a/infer/src/absint/PatternMatch.ml b/infer/src/absint/PatternMatch.ml index 6c3fad2f7..cc9b7dcfa 100644 --- a/infer/src/absint/PatternMatch.ml +++ b/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 = diff --git a/infer/src/absint/PatternMatch.mli b/infer/src/absint/PatternMatch.mli index a683ec33a..2eb510dda 100644 --- a/infer/src/absint/PatternMatch.mli +++ b/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. *) diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java b/infer/tests/codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java index a3ac734d5..e25623884 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java +++ b/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 diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp index e6bf0eb94..261872630 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp +++ b/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)]