diff --git a/infer/src/nullsafe/InheritanceRule.ml b/infer/src/nullsafe/InheritanceRule.ml index 08f21a012..5114cf3de 100644 --- a/infer/src/nullsafe/InheritanceRule.ml +++ b/infer/src/nullsafe/InheritanceRule.ml @@ -6,7 +6,8 @@ *) open! IStd -type violation = {base: Nullability.t; overridden: Nullability.t} [@@deriving compare] +type violation = {is_strict_mode: bool; base: Nullability.t; overridden: Nullability.t} +[@@deriving compare] type type_role = Param | Ret @@ -22,7 +23,7 @@ let is_whitelisted_violation ~subtype ~supertype = false -let check type_role ~base ~overridden = +let check ~is_strict_mode type_role ~base ~overridden = let subtype, supertype = match type_role with | Ret -> @@ -34,7 +35,7 @@ let check type_role ~base ~overridden = in Result.ok_if_true (Nullability.is_subtype ~subtype ~supertype || is_whitelisted_violation ~subtype ~supertype) - ~error:{base; overridden} + ~error:{is_strict_mode; base; overridden} type violation_type = @@ -70,3 +71,7 @@ let violation_description _ violation_type ~base_proc_name ~overridden_proc_name (translate_position param_position) MF.pp_monospaced param_description MF.pp_monospaced overridden_method_descr MF.pp_monospaced nullable_annotation MF.pp_monospaced nullable_annotation MF.pp_monospaced base_method_descr + + +let violation_severity {is_strict_mode} = + if is_strict_mode then Exceptions.Error else Exceptions.Warning diff --git a/infer/src/nullsafe/InheritanceRule.mli b/infer/src/nullsafe/InheritanceRule.mli index 9304f10a0..c2841a46e 100644 --- a/infer/src/nullsafe/InheritanceRule.mli +++ b/infer/src/nullsafe/InheritanceRule.mli @@ -26,7 +26,12 @@ type violation_type = type type_role = Param | Ret -val check : type_role -> base:Nullability.t -> overridden:Nullability.t -> (unit, violation) result +val check : + is_strict_mode:bool + -> type_role + -> base:Nullability.t + -> overridden:Nullability.t + -> (unit, violation) result val violation_description : violation @@ -34,3 +39,5 @@ val violation_description : -> base_proc_name:Procname.t -> overridden_proc_name:Procname.t -> string + +val violation_severity : violation -> Exceptions.severity diff --git a/infer/src/nullsafe/eradicateChecks.ml b/infer/src/nullsafe/eradicateChecks.ml index 6a28f5994..9f36a47e2 100644 --- a/infer/src/nullsafe/eradicateChecks.ml +++ b/infer/src/nullsafe/eradicateChecks.ml @@ -481,10 +481,11 @@ let check_call_parameters ~is_strict_mode ~callee_annotated_signature tenv find_ List.iter ~f:check resolved_params -let check_inheritance_rule_for_return find_canonical_duplicate tenv loc ~base_proc_name - ~overridden_proc_name ~overridden_proc_desc ~base_nullability ~overridden_nullability = +let check_inheritance_rule_for_return find_canonical_duplicate tenv loc ~is_strict_mode + ~base_proc_name ~overridden_proc_name ~overridden_proc_desc ~base_nullability + ~overridden_nullability = Result.iter_error - (InheritanceRule.check InheritanceRule.Ret ~base:base_nullability + (InheritanceRule.check ~is_strict_mode InheritanceRule.Ret ~base:base_nullability ~overridden:overridden_nullability) ~f:(fun inheritance_violation -> report_error tenv find_canonical_duplicate (TypeErr.Inconsistent_subclass @@ -495,11 +496,11 @@ let check_inheritance_rule_for_return find_canonical_duplicate tenv loc ~base_pr None loc overridden_proc_desc ) -let check_inheritance_rule_for_param find_canonical_duplicate tenv loc ~overridden_param_name - ~base_proc_name ~overridden_proc_name ~overridden_proc_desc ~param_position ~base_nullability - ~overridden_nullability = +let check_inheritance_rule_for_param find_canonical_duplicate tenv loc ~is_strict_mode + ~overridden_param_name ~base_proc_name ~overridden_proc_name ~overridden_proc_desc + ~param_position ~base_nullability ~overridden_nullability = Result.iter_error - (InheritanceRule.check InheritanceRule.Param ~base:base_nullability + (InheritanceRule.check ~is_strict_mode InheritanceRule.Param ~base:base_nullability ~overridden:overridden_nullability) ~f:(fun inheritance_violation -> report_error tenv find_canonical_duplicate (TypeErr.Inconsistent_subclass @@ -512,8 +513,9 @@ let check_inheritance_rule_for_param find_canonical_duplicate tenv loc ~overridd None loc overridden_proc_desc ) -let check_inheritance_rule_for_params find_canonical_duplicate tenv loc ~base_proc_name - ~overridden_proc_name ~overridden_proc_desc ~base_signature ~overridden_signature = +let check_inheritance_rule_for_params find_canonical_duplicate tenv loc ~is_strict_mode + ~base_proc_name ~overridden_proc_name ~overridden_proc_desc ~base_signature + ~overridden_signature = let base_params = base_signature.AnnotatedSignature.params in let overridden_params = overridden_signature.AnnotatedSignature.params in let zipped_params = List.zip base_params overridden_params in @@ -528,8 +530,8 @@ let check_inheritance_rule_for_params find_canonical_duplicate tenv loc ~base_pr { mangled= overridden_param_name ; param_annotated_type= {nullability= annotated_nullability_overridden} } ) -> - check_inheritance_rule_for_param find_canonical_duplicate tenv loc ~overridden_param_name - ~base_proc_name ~overridden_proc_name ~overridden_proc_desc + check_inheritance_rule_for_param find_canonical_duplicate tenv loc ~is_strict_mode + ~overridden_param_name ~base_proc_name ~overridden_proc_name ~overridden_proc_desc ~param_position:(if should_index_from_zero then index else index + 1) ~base_nullability:(AnnotatedNullability.get_nullability annotated_nullability_base) ~overridden_nullability: @@ -541,11 +543,13 @@ let check_inheritance_rule_for_params find_canonical_duplicate tenv loc ~base_pr (* Check both params and return values for complying for co- and contravariance *) -let check_inheritance_rule_for_signature find_canonical_duplicate tenv loc ~base_proc_name - ~overridden_proc_name ~overridden_proc_desc ~base_signature ~overridden_signature = +let check_inheritance_rule_for_signature find_canonical_duplicate tenv loc ~is_strict_mode + ~base_proc_name ~overridden_proc_name ~overridden_proc_desc ~base_signature + ~overridden_signature = (* Check params *) - check_inheritance_rule_for_params find_canonical_duplicate tenv loc ~base_proc_name - ~overridden_proc_name ~overridden_proc_desc ~base_signature ~overridden_signature ; + check_inheritance_rule_for_params find_canonical_duplicate tenv loc ~is_strict_mode + ~base_proc_name ~overridden_proc_name ~overridden_proc_desc ~base_signature + ~overridden_signature ; (* Check return value *) match base_proc_name with (* TODO model this as unknown nullability and get rid of that check *) @@ -559,8 +563,9 @@ let check_inheritance_rule_for_signature find_canonical_duplicate tenv loc ~base AnnotatedNullability.get_nullability overridden_signature.AnnotatedSignature.ret.ret_annotated_type.nullability in - check_inheritance_rule_for_return find_canonical_duplicate tenv loc ~base_proc_name - ~overridden_proc_name ~overridden_proc_desc ~base_nullability ~overridden_nullability + check_inheritance_rule_for_return find_canonical_duplicate tenv loc ~is_strict_mode + ~base_proc_name ~overridden_proc_name ~overridden_proc_desc ~base_nullability + ~overridden_nullability | _ -> (* the analysis should not report return type inconsistencies with external code *) () @@ -576,9 +581,10 @@ let check_overridden_annotations find_canonical_duplicate tenv proc_name proc_de match PatternMatch.lookup_attributes tenv base_proc_name with | Some base_attributes -> let base_signature = Models.get_modelled_annotated_signature tenv base_attributes in - check_inheritance_rule_for_signature find_canonical_duplicate tenv loc ~base_proc_name - ~overridden_proc_name:proc_name ~overridden_proc_desc:proc_desc ~base_signature - ~overridden_signature:annotated_signature + check_inheritance_rule_for_signature + ~is_strict_mode:annotated_signature.AnnotatedSignature.is_strict_mode + find_canonical_duplicate tenv loc ~base_proc_name ~overridden_proc_name:proc_name + ~overridden_proc_desc:proc_desc ~base_signature ~overridden_signature:annotated_signature | None -> (* Could not find the attributes - optimistically skipping the check *) (* TODO(T54687014) ensure this is not an issue in practice *) diff --git a/infer/src/nullsafe/typeErr.ml b/infer/src/nullsafe/typeErr.ml index 68f18c844..69a0fc28c 100644 --- a/infer/src/nullsafe/typeErr.ml +++ b/infer/src/nullsafe/typeErr.ml @@ -193,9 +193,8 @@ module Severity = struct None | Bad_assignment {assignment_violation} -> Some (AssignmentRule.violation_severity assignment_violation) - | Inconsistent_subclass _ -> - (* TODO: show strict mode violations as errors *) - None + | Inconsistent_subclass {inheritance_violation} -> + Some (InheritanceRule.violation_severity inheritance_violation) end (* Severity *) diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/InheritanceForStrictMode.java b/infer/tests/codetoanalyze/java/nullsafe-default/InheritanceForStrictMode.java new file mode 100644 index 000000000..242a853de --- /dev/null +++ b/infer/tests/codetoanalyze/java/nullsafe-default/InheritanceForStrictMode.java @@ -0,0 +1,88 @@ +/* + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** Test for checking how @NullsafeStrict mode plays with inheritance rule */ +package codetoanalyze.java.nullsafe_default; + +import com.facebook.infer.annotation.NullsafeStrict; +import javax.annotation.Nullable; + +public class InheritanceForStrictMode { + class NonStrictBase { + public @Nullable String okToRemoveNullableInChildren() { + return null; + } + + public String badToAddNullableInChildren() { + return ""; + } + + public void params( + @Nullable String badToRemoveNullableInChildren, String okToAddNullableInChildren) {} + } + + // Exactly as NonStrictBase, except that it is marked as @NullsafeStrict + @NullsafeStrict + class StrictBase { + public @Nullable String okToRemoveNullableInChildren() { + return null; + } + + public String badToAddNullableInChildren() { + return ""; + } + + public void params( + @Nullable String badToRemoveNullableInChildren, String okToAddNullableInChildren) {} + } + + // Expecting all issues to be surfaced as ERRORs + // NOTE: we currently DON'T require the base to be strictified in order to strictify a child (see + // T60513926) + @NullsafeStrict + class StrictExtendingNonstrict extends NonStrictBase { + public @Override String okToRemoveNullableInChildren() { + return ""; + } + + public @Override @Nullable String badToAddNullableInChildren() { + return null; + } + + public @Override void params( + String badToRemoveNullableInChildren, @Nullable String okToAddNullableInChildren) {} + } + + // Expecting all issues to be surfaced as ERRORs + @NullsafeStrict + class StrictExtendingStrict extends StrictBase { + public @Override String okToRemoveNullableInChildren() { + return ""; + } + + public @Override @Nullable String badToAddNullableInChildren() { + return null; + } + + public @Override void params( + String badToRemoveNullableInChildren, @Nullable String okToAddNullableInChildren) {} + } + + // Expecting all issues to be surfaces as WARNINGs (even that we extend a strict class) + class NonStrictExtendingStrict extends StrictBase { + public @Override String okToRemoveNullableInChildren() { + return ""; + } + + public @Override @Nullable String badToAddNullableInChildren() { + return null; + } + + public @Override void params( + String badToRemoveNullableInChildren, @Nullable String okToAddNullableInChildren) {} + } +} diff --git a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp index 4bcdc8795..3e9854deb 100644 --- a/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp +++ b/infer/tests/codetoanalyze/java/nullsafe-default/issues.exp @@ -101,6 +101,12 @@ codetoanalyze/java/nullsafe-default/InconsistentSubclassAnnotation.java, codetoa 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.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.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/InheritanceForStrictMode.java, codetoanalyze.java.nullsafe_default.InheritanceForStrictMode$NonStrictExtendingStrict.badToAddNullableInChildren():java.lang.String, 0, ERADICATE_INCONSISTENT_SUBCLASS_RETURN_ANNOTATION, no_bucket, WARNING, [Method `InheritanceForStrictMode$NonStrictExtendingStrict.badToAddNullableInChildren()` is annotated with `@Nullable` but overrides unannotated method `InheritanceForStrictMode$StrictBase.badToAddNullableInChildren()`.] +codetoanalyze/java/nullsafe-default/InheritanceForStrictMode.java, codetoanalyze.java.nullsafe_default.InheritanceForStrictMode$NonStrictExtendingStrict.params(java.lang.String,java.lang.String):void, 0, ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION, no_bucket, WARNING, [First parameter `badToRemoveNullableInChildren` of method `InheritanceForStrictMode$NonStrictExtendingStrict.params(...)` is not `@Nullable` but is declared `@Nullable`in the parent class method `InheritanceForStrictMode$StrictBase.params(...)`.] +codetoanalyze/java/nullsafe-default/InheritanceForStrictMode.java, codetoanalyze.java.nullsafe_default.InheritanceForStrictMode$StrictExtendingNonstrict.badToAddNullableInChildren():java.lang.String, 0, ERADICATE_INCONSISTENT_SUBCLASS_RETURN_ANNOTATION, no_bucket, ERROR, [Method `InheritanceForStrictMode$StrictExtendingNonstrict.badToAddNullableInChildren()` is annotated with `@Nullable` but overrides unannotated method `InheritanceForStrictMode$NonStrictBase.badToAddNullableInChildren()`.] +codetoanalyze/java/nullsafe-default/InheritanceForStrictMode.java, codetoanalyze.java.nullsafe_default.InheritanceForStrictMode$StrictExtendingNonstrict.params(java.lang.String,java.lang.String):void, 0, ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION, no_bucket, ERROR, [First parameter `badToRemoveNullableInChildren` of method `InheritanceForStrictMode$StrictExtendingNonstrict.params(...)` is not `@Nullable` but is declared `@Nullable`in the parent class method `InheritanceForStrictMode$NonStrictBase.params(...)`.] +codetoanalyze/java/nullsafe-default/InheritanceForStrictMode.java, codetoanalyze.java.nullsafe_default.InheritanceForStrictMode$StrictExtendingStrict.badToAddNullableInChildren():java.lang.String, 0, ERADICATE_INCONSISTENT_SUBCLASS_RETURN_ANNOTATION, no_bucket, ERROR, [Method `InheritanceForStrictMode$StrictExtendingStrict.badToAddNullableInChildren()` is annotated with `@Nullable` but overrides unannotated method `InheritanceForStrictMode$StrictBase.badToAddNullableInChildren()`.] +codetoanalyze/java/nullsafe-default/InheritanceForStrictMode.java, codetoanalyze.java.nullsafe_default.InheritanceForStrictMode$StrictExtendingStrict.params(java.lang.String,java.lang.String):void, 0, ERADICATE_INCONSISTENT_SUBCLASS_PARAMETER_ANNOTATION, no_bucket, ERROR, [First parameter `badToRemoveNullableInChildren` of method `InheritanceForStrictMode$StrictExtendingStrict.params(...)` is not `@Nullable` but is declared `@Nullable`in the parent class method `InheritanceForStrictMode$StrictBase.params(...)`.] 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)] codetoanalyze/java/nullsafe-default/LibraryCalls.java, codetoanalyze.java.nullsafe_default.LibraryCalls.badReferenceDereference(java.lang.ref.Reference):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 Reference.get() at line 19 (nullable according to nullsafe internal models)]