From 21d3450ef5644968ad9305e3f7e5f05647ef9ad9 Mon Sep 17 00:00:00 2001 From: Mitya Lyubarskiy Date: Wed, 18 Sep 2019 02:58:21 -0700 Subject: [PATCH] [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 | 3 ++- infer/src/base/IssueType.mli | 2 -- infer/src/nullsafe/eradicateChecks.ml | 16 +++------------- infer/src/nullsafe/typeCheck.ml | 3 +-- infer/src/nullsafe/typeErr.ml | 12 ++++-------- infer/src/nullsafe/typeErr.mli | 2 +- 6 files changed, 11 insertions(+), 27 deletions(-) diff --git a/infer/src/base/IssueType.ml b/infer/src/base/IssueType.ml index 44638213b..116e04c9d 100644 --- a/infer/src/base/IssueType.ml +++ b/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" diff --git a/infer/src/base/IssueType.mli b/infer/src/base/IssueType.mli index 5e558fad3..52920ad2f 100644 --- a/infer/src/base/IssueType.mli +++ b/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 diff --git a/infer/src/nullsafe/eradicateChecks.ml b/infer/src/nullsafe/eradicateChecks.ml index 27b6bb56b..186c68a10 100644 --- a/infer/src/nullsafe/eradicateChecks.ml +++ b/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) - && true_branch && ((not is_temp) || nonnull) && PatternMatch.type_is_class typ + && Config.eradicate_condition_redundant && true_branch && (not is_temp) + && 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 diff --git a/infer/src/nullsafe/typeCheck.ml b/infer/src/nullsafe/typeCheck.ml index 34e9f2e66..eb2636783 100644 --- a/infer/src/nullsafe/typeCheck.ml +++ b/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 - (true, EradicateChecks.explain_expr tenv node cond, false)) + (TypeErr.Condition_redundant (true, EradicateChecks.explain_expr tenv node cond)) (Some instr_ref) loc curr_pdesc ) ; TypeState.add pvar (t, TypeAnnotation.const_nullable false TypeOrigin.ONone, [loc]) diff --git a/infer/src/nullsafe/typeErr.ml b/infer/src/nullsafe/typeErr.ml index 070b3f13b..57497d611 100644 --- a/infer/src/nullsafe/typeErr.ml +++ b/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) -> - let name = - if nonnull then IssueType.eradicate_condition_redundant_nonnull - else IssueType.eradicate_condition_redundant - in - ( name + | Condition_redundant (is_always_true, s_opt) -> + ( IssueType.eradicate_condition_redundant , 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 = diff --git a/infer/src/nullsafe/typeErr.mli b/infer/src/nullsafe/typeErr.mli index 6c0077281..cbfb88455 100644 --- a/infer/src/nullsafe/typeErr.mli +++ b/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