From 597c7304749f864279388aed997ff99f4fd6c833 Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Fri, 7 Feb 2020 05:11:35 -0800 Subject: [PATCH] [nullsafe] Treat parameter in overrides of `equals(Object param)` as nullable. Summary: We already warn about lack of nullable annotations in `equals()`, and even have a specialized error message for that. But lack of an annotation is not as severe as direct dereference: the latter is a plain bug which is also a time bomb: it will lead to an NPE not immediately. This is widespread enough to be reported separately. Reviewed By: dulmarod Differential Revision: D19719598 fbshipit-source-id: a535d43ea --- infer/src/nullsafe/ErrorRenderingUtils.ml | 4 +++- infer/src/nullsafe/eradicate.ml | 4 +++- infer/src/nullsafe/typeOrigin.ml | 17 +++++++++++++---- infer/src/nullsafe/typeOrigin.mli | 4 +++- .../InconsistentSubclassAnnotation.java | 8 ++++++-- .../java/nullsafe-default/issues.exp | 1 + 6 files changed, 29 insertions(+), 9 deletions(-) diff --git a/infer/src/nullsafe/ErrorRenderingUtils.ml b/infer/src/nullsafe/ErrorRenderingUtils.ml index c8708ffd7..10a6523c4 100644 --- a/infer/src/nullsafe/ErrorRenderingUtils.ml +++ b/infer/src/nullsafe/ErrorRenderingUtils.ml @@ -22,12 +22,14 @@ let is_object_nullability_self_explanatory ~object_expression (object_origin : T | NullConst _ -> (* Expect either a local variable or null literal (latter case is trivial) *) String.equal object_expression "null" - | MethodParameter {mangled} -> + | MethodParameter (Normal {mangled}) -> (* Either local variable or literally parameter. In latter case, its nullability is self-explanatory because the user can quickly see the current method signature. *) let param_name = Mangled.to_string mangled in String.equal object_expression param_name + | MethodParameter ObjectEqualsOverride -> + false | Field {field_name} -> (* Either local variable or expression like `.field_name`. Latter case is trivial: the user can quickly go to field_name definition and see if its annotation. *) diff --git a/infer/src/nullsafe/eradicate.ml b/infer/src/nullsafe/eradicate.ml index 5805cf655..e35d03a6d 100644 --- a/infer/src/nullsafe/eradicate.ml +++ b/infer/src/nullsafe/eradicate.ml @@ -41,7 +41,9 @@ module MkCallback (Extension : ExtensionT) : CallBackT = struct has very special meaning and nullability limitations (it can never be null). *) TypeOrigin.This - else TypeOrigin.MethodParameter param_signature + else if PatternMatch.is_override_of_java_lang_object_equals curr_pname then + TypeOrigin.MethodParameter ObjectEqualsOverride + else TypeOrigin.MethodParameter (Normal param_signature) in let inferred_nullability = InferredNullability.create origin in TypeState.add pvar (param_signature.param_annotated_type.typ, inferred_nullability) typestate diff --git a/infer/src/nullsafe/typeOrigin.ml b/infer/src/nullsafe/typeOrigin.ml index 4f79e921d..7b169085c 100644 --- a/infer/src/nullsafe/typeOrigin.ml +++ b/infer/src/nullsafe/typeOrigin.ml @@ -13,7 +13,7 @@ type t = | NullConst of Location.t (** A null literal in the source *) | NonnullConst of Location.t (** A constant (not equal to null) in the source. *) | Field of field_origin (** A field access (result of expression `some_object.some_field`) *) - | MethodParameter of AnnotatedSignature.param_signature (** A method's parameter *) + | MethodParameter of method_parameter_origin (** A method's parameter *) | This (* `this` object. Can not be null, according to Java rules. *) | MethodCall of method_call_origin (** A result of a method call *) | CallToGetKnownToContainsKey @@ -34,6 +34,8 @@ type t = | Undef (** Undefined value before initialization *) [@@deriving compare] +and method_parameter_origin = Normal of AnnotatedSignature.param_signature | ObjectEqualsOverride + and field_origin = { object_origin: t (** field's object origin (object is before field access operator `.`) *) ; field_name: Fieldname.t @@ -70,8 +72,11 @@ let get_nullability = function Nullability.StrictNonnull | Field {field_type= {nullability}} -> AnnotatedNullability.get_nullability nullability - | MethodParameter {param_annotated_type= {nullability}} -> + | MethodParameter (Normal {param_annotated_type= {nullability}}) -> AnnotatedNullability.get_nullability nullability + | MethodParameter ObjectEqualsOverride -> + (* `Object.equals(obj)` should expect to be called with null `obj` *) + Nullability.Nullable | MethodCall {annotated_signature= {ret= {ret_annotated_type= {nullability}}}} -> AnnotatedNullability.get_nullability nullability @@ -83,9 +88,11 @@ let rec to_string = function "Const (nonnull)" | Field {object_origin; field_name} -> "Field " ^ Fieldname.to_string field_name ^ " (object: " ^ to_string object_origin ^ ")" - | MethodParameter {mangled; param_annotated_type= {nullability}} -> + | MethodParameter (Normal {mangled; param_annotated_type= {nullability}}) -> Format.asprintf "Param %s <%a>" (Mangled.to_string mangled) AnnotatedNullability.pp nullability + | MethodParameter ObjectEqualsOverride -> + "Param(ObjectEqualsOverride)" | This -> "this" | MethodCall {pname} -> @@ -149,8 +156,10 @@ let get_description origin = Some ("null constant" ^ atline loc) | Field {field_name; access_loc} -> Some ("field " ^ Fieldname.get_field_name field_name ^ atline access_loc) - | MethodParameter {mangled} -> + | MethodParameter (Normal {mangled}) -> Some ("method parameter " ^ Mangled.to_string mangled) + | MethodParameter ObjectEqualsOverride -> + Some "Object.equals() should be able to accept `null`, according to the Java specification" | MethodCall {pname; call_loc; annotated_signature} -> Some (get_method_ret_description pname call_loc annotated_signature) (* These are origins of non-nullable expressions that are result of evaluating of some rvalue. diff --git a/infer/src/nullsafe/typeOrigin.mli b/infer/src/nullsafe/typeOrigin.mli index ba14b9481..ec72ed666 100644 --- a/infer/src/nullsafe/typeOrigin.mli +++ b/infer/src/nullsafe/typeOrigin.mli @@ -11,7 +11,7 @@ type t = | NullConst of Location.t (** A null literal in the source *) | NonnullConst of Location.t (** A constant (not equal to null) in the source. *) | Field of field_origin (** A field access (result of expression `some_object.some_field`) *) - | MethodParameter of AnnotatedSignature.param_signature (** A method's parameter *) + | MethodParameter of method_parameter_origin (** A method's parameter *) | This (* `this` object. Can not be null, according to Java rules. *) | MethodCall of method_call_origin (** A result of a method call *) | CallToGetKnownToContainsKey @@ -32,6 +32,8 @@ type t = | Undef (** Undefined value before initialization *) [@@deriving compare] +and method_parameter_origin = Normal of AnnotatedSignature.param_signature | ObjectEqualsOverride + and field_origin = { object_origin: t (** field's object origin (object is before field access operator `.`) *) ; field_name: Fieldname.t diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java b/infer/tests/codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java index bc27596b1..709cf89c5 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java +++ b/infer/tests/codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java @@ -172,11 +172,15 @@ class ExtendsExternalLibrary extends SomeExternalClass { } } -// Check that we have a special error message for this method +// Check that 1) we have a special error message for lack of annotation in this method and 2) treat +// `x` as implicitly nullable class JavaLangEquals { @Override public boolean equals(Object x) { - return false; + // BAD: x can not be directly dereferenced without null comparison: + // it is implicitly nullable because Java requires `x.equals(null)` to work correctly. + // It is a common enough case to make the nullsafe support this specifically. + return x.toString() == "JavaLangEquals"; } } diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp index 7aacbe5ab..3625625d1 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp +++ b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp @@ -73,6 +73,7 @@ codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoa 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 missing `@Nullable` declaration when overriding `InconsistentSubclassAnnotationInterface.implementInAnotherFile(...)`. The parent method declared it can handle `null` for this param, so the child should also declare that.] 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 missing `@Nullable` declaration when overriding `SomeExternalClass.externalMethod2(...)`. The parent method declared it can handle `null` for this param, so the child should also declare that.] codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.JavaLangEquals.equals(java.lang.Object):boolean, 0, ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION, no_bucket, WARNING, [Parameter `x` is missing `@Nullable` declaration: according to the Java Specification, for any object `x` call `x.equals(null)` should properly return false.] +codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.JavaLangEquals.equals(java.lang.Object):boolean, 0, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`x` is nullable and is not locally checked for null when calling `toString()`: Object.equals() should be able to accept `null`, according to the Java specification] codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoanalyze.java.nullsafe_default.NullableConcreteGetterBAD.get():java.lang.String, 0, ERADICATE_INCONSISTENT_SUBCLASS_RETURN_ANNOTATION, no_bucket, WARNING, [Child method `NullableConcreteGetterBAD.get()` is not substitution-compatible with its parent: the return type is declared as nullable, but parent method `NonNullableInterfaceGetterOK.get()` is missing `@Nullable` declaration. Either mark the parent as `@Nullable` or ensure the child does not return `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, [Child method `OverloadExistingIncorrectBAD.overload(...)` is not substitution-compatible with its parent: the return type is declared as nullable, but parent method `Overloads.overload(...)` is missing `@Nullable` declaration. Either mark the parent as `@Nullable` or ensure the child does not return `null`.] 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, [Child method `ReturnValToNullBAD.valBoth(...)` is not substitution-compatible with its parent: the return type is declared as nullable, but parent method `VariousMethods.valBoth(...)` is missing `@Nullable` declaration. Either mark the parent as `@Nullable` or ensure the child does not return `null`.]