[nullsafe] Show violations of @NullsafeStrict mode as errors: Part 2

Summary: This diff does it for inheritance violation issues.

Reviewed By: artempyanykh

Differential Revision: D19393748

fbshipit-source-id: 47be41e77

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

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

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
+let check_inheritance_rule_for_return find_canonical_duplicate tenv loc ~is_strict_mode
-    ~overridden_proc_name ~overridden_proc_desc ~base_nullability ~overridden_nullability =
+    ~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
+let check_inheritance_rule_for_param find_canonical_duplicate tenv loc ~is_strict_mode
-    ~base_proc_name ~overridden_proc_name ~overridden_proc_desc ~param_position ~base_nullability
+    ~overridden_param_name ~base_proc_name ~overridden_proc_name ~overridden_proc_desc
-    ~overridden_nullability =
+    ~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
+let check_inheritance_rule_for_params find_canonical_duplicate tenv loc ~is_strict_mode
-    ~overridden_proc_name ~overridden_proc_desc ~base_signature ~overridden_signature =
+    ~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
+          check_inheritance_rule_for_param find_canonical_duplicate tenv loc ~is_strict_mode
-            ~base_proc_name ~overridden_proc_name ~overridden_proc_desc
+            ~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
+let check_inheritance_rule_for_signature find_canonical_duplicate tenv loc ~is_strict_mode
-    ~overridden_proc_name ~overridden_proc_desc ~base_signature ~overridden_signature =
+    ~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
+  check_inheritance_rule_for_params find_canonical_duplicate tenv loc ~is_strict_mode
-    ~overridden_proc_name ~overridden_proc_desc ~base_signature ~overridden_signature ;
+    ~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
+      check_inheritance_rule_for_return find_canonical_duplicate tenv loc ~is_strict_mode
-        ~overridden_proc_name ~overridden_proc_desc ~base_nullability ~overridden_nullability
+        ~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
+        check_inheritance_rule_for_signature
-          ~overridden_proc_name:proc_name ~overridden_proc_desc:proc_desc ~base_signature
+          ~is_strict_mode:annotated_signature.AnnotatedSignature.is_strict_mode
-          ~overridden_signature:annotated_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
     | None ->
         (* Could not find the attributes - optimistically skipping the check *)
         (* TODO(T54687014) ensure this is not an issue in practice *)

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 _ ->
+    | Inconsistent_subclass {inheritance_violation} ->
-        (* TODO: show strict mode violations as errors *)
+        Some (InheritanceRule.violation_severity inheritance_violation)
-        None
 end
 
 (* Severity *)

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) {}
+  }
+}

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