[nullsafe] Remove special treatment of @Nonnull for "Condition Redundant" check

Summary:
CONDITION_REDUNDANT_NONNULL was an attempt to reduce number of false
positives for condition redundant. (It is the most popular check as of
now).

The root case for most of false positives is that a lot of code is
simply not annotated (but should have been), so blaming developers for defense programming is
not actionable.

In attempt to solve the problem, a special issue type (for case when the
code is explicitly annotated with Nonnull) was introduced.

In follow up diffs we are going to introduce a generic way of doing the
same, not limited to this particular check only.

Namely, we will introduce notion of unknown nullability, so it will be
possible to distinguish not annotated yet (hence no warnings) and already
annotated (hence warnings) parts of code.

This piece of logic is incompatible with the aforementioned work, hence
we need to remove it.

Reviewed By: jvillard

Differential Revision: D17398768

fbshipit-source-id: 8bddf10e5

infer/src/base/IssueType.ml

@@ -223,7 +223,8 @@ let eradicate_condition_redundant =
   register_from_string "ERADICATE_CONDITION_REDUNDANT" ~hum:"Condition Redundant"
 
 
-let eradicate_condition_redundant_nonnull =
+(* TODO(T54070503) remove condition redundant nonnull *)
+let _ =
   register_from_string "ERADICATE_CONDITION_REDUNDANT_NONNULL" ~hum:"Condition Redundant Non-Null"
 
 

infer/src/base/IssueType.mli

@@ -144,8 +144,6 @@ val empty_vector_access : t
 
 val eradicate_condition_redundant : t
 
-val eradicate_condition_redundant_nonnull : t
-
 val eradicate_field_not_initialized : t
 
 val eradicate_field_not_nullable : t

infer/src/nullsafe/eradicateChecks.ml

@@ -69,14 +69,6 @@ let equal_from_call = [%compare.equal: from_call]
 (** Check the normalized "is zero" or "is not zero" condition of a prune instruction. *)
 let check_condition tenv case_zero find_canonical_duplicate curr_pdesc node e typ ta true_branch
     from_call idenv linereader loc instr_ref : unit =
-  let is_fun_nonnull ta =
-    match TypeAnnotation.get_origin ta with
-    | TypeOrigin.Proc proc_origin ->
-        let ia, _ = proc_origin.TypeOrigin.annotated_signature.AnnotatedSignature.ret in
-        Annotations.ia_is_nonnull ia
-    | _ ->
-        false
-  in
   let contains_instanceof_throwable pdesc node =
     (* Check if the current procedure has a catch Throwable. *)
     (* That always happens in the bytecode generated by try-with-resources. *)
@@ -113,20 +105,18 @@ let check_condition tenv case_zero find_canonical_duplicate curr_pdesc node e ty
         false
   in
   let is_temp = Idenv.exp_is_temp idenv e in
-  let nonnull = is_fun_nonnull ta in
   let should_report =
     (not (TypeAnnotation.is_nullable ta))
-    && (Config.eradicate_condition_redundant || nonnull)
+    && Config.eradicate_condition_redundant && true_branch && (not is_temp)
-    && true_branch && ((not is_temp) || nonnull) && PatternMatch.type_is_class typ
+    && PatternMatch.type_is_class typ
     && (not (from_try_with_resources ()))
     && equal_from_call from_call From_condition
     && not (TypeAnnotation.origin_is_fun_library ta)
   in
   let is_always_true = not case_zero in
-  let nonnull = is_fun_nonnull ta in
   if should_report then
     report_error tenv find_canonical_duplicate
-      (TypeErr.Condition_redundant (is_always_true, explain_expr tenv node e, nonnull))
+      (TypeErr.Condition_redundant (is_always_true, explain_expr tenv node e))
       (Some instr_ref) loc curr_pdesc
 
 

infer/src/nullsafe/typeCheck.ml

@@ -551,8 +551,7 @@ let typecheck_instr tenv calls_this checks (node : Procdesc.Node.t) idenv curr_p
               ( if checks.eradicate && should_report then
                 let cond = Exp.BinOp (Binop.Ne, Exp.Lvar pvar, Exp.null) in
                 EradicateChecks.report_error tenv find_canonical_duplicate
-                  (TypeErr.Condition_redundant
+                  (TypeErr.Condition_redundant (true, EradicateChecks.explain_expr tenv node cond))
-                     (true, EradicateChecks.explain_expr tenv node cond, false))
                   (Some instr_ref) loc curr_pdesc ) ;
               TypeState.add pvar
                 (t, TypeAnnotation.const_nullable false TypeOrigin.ONone, [loc])

infer/src/nullsafe/typeErr.ml

@@ -75,7 +75,7 @@ type parameter_not_nullable =
 
 (** Instance of an error *)
 type err_instance =
-  | Condition_redundant of (bool * string option * bool)
+  | Condition_redundant of (bool * string option)
   | Inconsistent_subclass_return_annotation of Typ.Procname.t * Typ.Procname.t
   | Inconsistent_subclass_parameter_annotation of string * int * Typ.Procname.t * Typ.Procname.t
   | Field_not_initialized of Typ.Fieldname.t * Typ.Procname.t
@@ -220,14 +220,10 @@ let report_error_now tenv (st_report_error : st_report_error) err_instance loc p
   let nullable_annotation = "@Nullable" in
   let kind, description, field_name =
     match err_instance with
-    | Condition_redundant (b, s_opt, nonnull) ->
+    | Condition_redundant (is_always_true, s_opt) ->
-        let name =
+        ( IssueType.eradicate_condition_redundant
-          if nonnull then IssueType.eradicate_condition_redundant_nonnull
-          else IssueType.eradicate_condition_redundant
-        in
-        ( name
         , P.sprintf "The condition %s is always %b according to the existing annotations."
-            (Option.value s_opt ~default:"") b
+            (Option.value s_opt ~default:"") is_always_true
         , None )
     | Field_not_initialized (fn, pn) ->
         let constructor_name =

infer/src/nullsafe/typeErr.mli

@@ -46,7 +46,7 @@ type parameter_not_nullable =
 
 (** Instance of an error *)
 type err_instance =
-  | Condition_redundant of (bool * string option * bool)
+  | Condition_redundant of (bool * string option)
   | Inconsistent_subclass_return_annotation of Typ.Procname.t * Typ.Procname.t
   | Inconsistent_subclass_parameter_annotation of string * int * Typ.Procname.t * Typ.Procname.t
   | Field_not_initialized of Typ.Fieldname.t * Typ.Procname.t