From 0f7b6db07caf46119e428dfb2c8390af42ef8af4 Mon Sep 17 00:00:00 2001 From: Artem Pianykh Date: Tue, 30 Jun 2020 04:00:37 -0700 Subject: [PATCH] [nullsafe] Fix handling of assignment expressions in prune Summary: We already had a heuristic to deal with assignment expressions, but it relied on the very previous CFG node to have a non-empty list of instrs. In some cases, however, this previous node is a Join_node with no instrs, so we need to take one more step back to find what we're looking for. I've also added a bit more logging around this functionality, so it's easier to debug/tune in future. Reviewed By: ngorogiannis Differential Revision: D22282930 fbshipit-source-id: 024eec145 --- infer/src/nullsafe/typeCheck.ml | 52 ++++++++++++++----- .../java/nullsafe/NullMethodCall.java | 2 +- .../java/nullsafe/ReturnNotNullable.java | 3 +- .../codetoanalyze/java/nullsafe/issues.exp | 7 +-- 4 files changed, 43 insertions(+), 21 deletions(-) diff --git a/infer/src/nullsafe/typeCheck.ml b/infer/src/nullsafe/typeCheck.ml index 9f7c100cb..70180c211 100644 --- a/infer/src/nullsafe/typeCheck.ml +++ b/infer/src/nullsafe/typeCheck.ml @@ -673,22 +673,48 @@ let do_map_put ({IntraproceduralAnalysis.proc_desc= curr_pdesc; tenv; _} as anal (* Handle assignment fron a temp pvar in a condition. - This recognizes the handling of temp variables in ((x = ...) != null) *) + This recognizes the handling of temp variables in ((x = ...) != null) + + The main idea is to take a quick look back in the CFG for any assignments + from [pvar]. *) let handle_assignment_in_condition_for_sil_prune idenv node pvar = - match Procdesc.Node.get_preds node with - | [prev_node] -> - let found = ref None in - let do_instr i = - match i with - | Sil.Store {e1= e; e2= e'} when Exp.equal (Exp.Lvar pvar) (IDEnv.expand_expr idenv e') -> - found := Some e + L.d_with_indent ~pp_result:(Pp.option Exp.pp) ~name:"handle_assignment_in_condition_for_sil_prune" + (fun () -> + L.d_printfln "Pvar being pruned: %a" (Pvar.pp Pp.text) pvar ; + (* We need to find the first *unique* immediate predecessor with non-empty + list of instructions. Since some nodes like Join_node can have empty list of instrs, + we need to traverse CFG a bit. *) + let rec find_pred_node_with_instrs node = + match Procdesc.Node.get_preds node with + | [pred] -> + if Instrs.is_empty (Procdesc.Node.get_instrs pred) then find_pred_node_with_instrs pred + else Some pred | _ -> - () + None in - Instrs.iter ~f:do_instr (Procdesc.Node.get_instrs prev_node) ; - !found - | _ -> - None + (* Inspect instructions within the node to find assignments from [pvar] *) + let find_aliased_var node = + let inspect_instr instr = + match instr with + | Sil.Store {e1= e; e2= e'} -> ( + let expanded_rhs = IDEnv.expand_expr idenv e' in + L.d_printfln "Found store instr: %a; RHS expands to: %a" + (Sil.pp_instr ~print_types:false Pp.text) + instr Exp.pp expanded_rhs ; + match expanded_rhs with Exp.Lvar v when Pvar.equal v pvar -> Some e | _ -> None ) + | _ -> + None + in + (* Here we check last instructions first. IDK if it makes a difference, but + it is at least compatible with the previous behaviour *) + Instrs.find_map (Procdesc.Node.get_instrs node |> Instrs.reverse_order) ~f:inspect_instr + in + match find_pred_node_with_instrs node with + | Some prev_node -> + L.d_printfln "Found non-empty unique predecessor node: #%a" Procdesc.Node.pp prev_node ; + find_aliased_var prev_node + | _ -> + None ) let rec normalize_cond_for_sil_prune_rec idenv ~node ~original_node cond = diff --git a/infer/tests/codetoanalyze/java/nullsafe/NullMethodCall.java b/infer/tests/codetoanalyze/java/nullsafe/NullMethodCall.java index 535f50c35..55b844a04 100644 --- a/infer/tests/codetoanalyze/java/nullsafe/NullMethodCall.java +++ b/infer/tests/codetoanalyze/java/nullsafe/NullMethodCall.java @@ -349,7 +349,7 @@ public class NullMethodCall { } } - void testInAssignmentFP(@Nullable Object object) { + void testInAssignmentOK(@Nullable Object object) { Object t; while ((t = getNullable()) != null) { t.toString(); // Should not warn here diff --git a/infer/tests/codetoanalyze/java/nullsafe/ReturnNotNullable.java b/infer/tests/codetoanalyze/java/nullsafe/ReturnNotNullable.java index a67e80270..0e91d5fdd 100644 --- a/infer/tests/codetoanalyze/java/nullsafe/ReturnNotNullable.java +++ b/infer/tests/codetoanalyze/java/nullsafe/ReturnNotNullable.java @@ -216,8 +216,7 @@ public class ReturnNotNullable { } static class AssignmentResultCheck { - // T58407328 - public Throwable nullCheckAssignmentResultAsNonnull_FP(Throwable error) { + public Throwable nullCheckAssignmentResultAsNonnullOk(Throwable error) { Throwable cause; while ((cause = error.getCause()) != null) { error = cause; diff --git a/infer/tests/codetoanalyze/java/nullsafe/issues.exp b/infer/tests/codetoanalyze/java/nullsafe/issues.exp index a6d1a9799..c9de7da4a 100644 --- a/infer/tests/codetoanalyze/java/nullsafe/issues.exp +++ b/infer/tests/codetoanalyze/java/nullsafe/issues.exp @@ -176,7 +176,7 @@ codetoanalyze/java/nullsafe/NullFieldAccess.java, codetoanalyze.java.nullsafe.Nu codetoanalyze/java/nullsafe/NullFieldAccess.java, codetoanalyze.java.nullsafe.NullFieldAccess.testInterface():void, 2, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`bad` is nullable and is not locally checked for null when calling `toString()`: field nullable at line 52.] codetoanalyze/java/nullsafe/NullFieldAccess.java, codetoanalyze.java.nullsafe.NullFieldAccess.testNonStaticFields():void, 2, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`bad` is nullable and is not locally checked for null when calling `toString()`: field nullable at line 36.] codetoanalyze/java/nullsafe/NullFieldAccess.java, codetoanalyze.java.nullsafe.NullFieldAccess.testStatic():void, 2, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`bad` is nullable and is not locally checked for null when calling `toString()`: field nullableStatic at line 44.] -codetoanalyze/java/nullsafe/NullMethodCall.java, Linters_dummy_method, 22, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], NullMethodCall, codetoanalyze.java.nullsafe, issues: 24, curr_mode: "Default" +codetoanalyze/java/nullsafe/NullMethodCall.java, Linters_dummy_method, 22, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], NullMethodCall, codetoanalyze.java.nullsafe, issues: 23, curr_mode: "Default" codetoanalyze/java/nullsafe/NullMethodCall.java, codetoanalyze.java.nullsafe.NullMethodCall$Inner.outerField():int, 2, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`s` is nullable and is not locally checked for null when calling `length()`: field fld at line 69.] codetoanalyze/java/nullsafe/NullMethodCall.java, codetoanalyze.java.nullsafe.NullMethodCall$Inner.outerPrivateField():int, 2, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`s` is nullable and is not locally checked for null when calling `length()`: field pfld at line 80.] codetoanalyze/java/nullsafe/NullMethodCall.java, codetoanalyze.java.nullsafe.NullMethodCall.FP_propagatesNonNullAfterComparisonFieldOkay(java.lang.Object):void, 2, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`NullMethodCall.nullableField` is nullable and is not locally checked for null when calling `toString()`.] @@ -188,7 +188,6 @@ codetoanalyze/java/nullsafe/NullMethodCall.java, codetoanalyze.java.nullsafe.Nul codetoanalyze/java/nullsafe/NullMethodCall.java, codetoanalyze.java.nullsafe.NullMethodCall.nullabilityStoredInBooleanFP():void, 3, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`getNullable()` is nullable and is not locally checked for null when calling `toString()`.] codetoanalyze/java/nullsafe/NullMethodCall.java, codetoanalyze.java.nullsafe.NullMethodCall.testExceptionPerInstruction(int):void, 6, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [NullPointerException will be thrown at this line! `s` is `null` and is dereferenced via calling `length()`: null constant at line 181.] codetoanalyze/java/nullsafe/NullMethodCall.java, codetoanalyze.java.nullsafe.NullMethodCall.testFieldAssignmentIfThenElse(java.lang.String):void, 2, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`s` is nullable and is not locally checked for null when calling `length()`: null constant at line 172.] -codetoanalyze/java/nullsafe/NullMethodCall.java, codetoanalyze.java.nullsafe.NullMethodCall.testInAssignmentFP(java.lang.Object):void, 3, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`t` is nullable and is not locally checked for null when calling `toString()`: call to getNullable() at line 354.] codetoanalyze/java/nullsafe/NullMethodCall.java, codetoanalyze.java.nullsafe.NullMethodCall.testMapGetBad(java.util.Map,java.util.HashMap,java.util.concurrent.ConcurrentHashMap):void, 2, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`m.get(...)` is nullable and is not locally checked for null when calling `toString()`: call to Map.get(...) at line 260 (nullable according to nullsafe internal models).] codetoanalyze/java/nullsafe/NullMethodCall.java, codetoanalyze.java.nullsafe.NullMethodCall.testMapGetBad(java.util.Map,java.util.HashMap,java.util.concurrent.ConcurrentHashMap):void, 3, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`hm.get(...)` is nullable and is not locally checked for null when calling `toString()`: call to HashMap.get(...) at line 261 (nullable according to nullsafe internal models).] codetoanalyze/java/nullsafe/NullMethodCall.java, codetoanalyze.java.nullsafe.NullMethodCall.testMapGetBad(java.util.Map,java.util.HashMap,java.util.concurrent.ConcurrentHashMap):void, 4, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`chm.get(...)` is nullable and is not locally checked for null when calling `toString()`: call to ConcurrentHashMap.get(...) at line 262 (nullable according to nullsafe internal models).] @@ -302,9 +301,7 @@ codetoanalyze/java/nullsafe/PropagatesNullable.java, codetoanalyze.java.nullsafe codetoanalyze/java/nullsafe/PropagatesNullable.java, codetoanalyze.java.nullsafe.TestPropagatesNullable$TestSecondParameter.test(java.lang.String,java.lang.String):void, 7, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`nullable(...)` is nullable and is not locally checked for null when calling `length()`.] codetoanalyze/java/nullsafe/PropagatesNullable.java, codetoanalyze.java.nullsafe.TestPropagatesNullable$TestSecondParameter.test(java.lang.String,java.lang.String):void, 11, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`nullable(...)` is nullable and is not locally checked for null when calling `length()`.] codetoanalyze/java/nullsafe/PropagatesNullable.java, codetoanalyze.java.nullsafe.TestPropagatesNullable$TestSecondParameter.test(java.lang.String,java.lang.String):void, 15, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`nullable(...)` is nullable and is not locally checked for null when calling `length()`.] -codetoanalyze/java/nullsafe/ReturnNotNullable.java, Linters_dummy_method, 19, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], ReturnNotNullable, codetoanalyze.java.nullsafe, issues: 11, curr_mode: "Default" -codetoanalyze/java/nullsafe/ReturnNotNullable.java, codetoanalyze.java.nullsafe.ReturnNotNullable$AssignmentResultCheck.nullCheckAssignmentResultAsNonnull_FP(java.lang.Throwable):java.lang.Throwable, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [`nullCheckAssignmentResultAsNonnull_FP(...)`: return type is declared non-nullable but the method returns a nullable value: call to Throwable.getCause() at line 222 (declared nullable in nullsafe/third-party-signatures/java.sig at line 2).] -codetoanalyze/java/nullsafe/ReturnNotNullable.java, codetoanalyze.java.nullsafe.ReturnNotNullable$AssignmentResultCheck.nullCheckAssignmentResultAsNonnull_FP(java.lang.Throwable):java.lang.Throwable, 2, ERADICATE_NULLABLE_DEREFERENCE, no_bucket, WARNING, [`error` is nullable and is not locally checked for null when calling `getCause()`: call to Throwable.getCause() at line 222 (declared nullable in nullsafe/third-party-signatures/java.sig at line 2).] +codetoanalyze/java/nullsafe/ReturnNotNullable.java, Linters_dummy_method, 19, ERADICATE_META_CLASS_NEEDS_IMPROVEMENT, no_bucket, INFO, [], ReturnNotNullable, codetoanalyze.java.nullsafe, issues: 9, curr_mode: "Default" codetoanalyze/java/nullsafe/ReturnNotNullable.java, codetoanalyze.java.nullsafe.ReturnNotNullable$ConditionalAssignment.test(boolean):java.lang.Object, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [`test(...)`: return type is declared non-nullable but the method returns a nullable value: field f1 at line 199.] codetoanalyze/java/nullsafe/ReturnNotNullable.java, codetoanalyze.java.nullsafe.ReturnNotNullable.constantToNullableIsOverannotated():java.lang.String, 0, ERADICATE_RETURN_OVER_ANNOTATED, no_bucket, ADVICE, [Method `constantToNullableIsOverannotated()` is annotated with `@Nullable` but never returns null.] codetoanalyze/java/nullsafe/ReturnNotNullable.java, codetoanalyze.java.nullsafe.ReturnNotNullable.getResourceNullable(java.lang.Class,java.lang.String):java.net.URL, 0, ERADICATE_RETURN_NOT_NULLABLE, no_bucket, WARNING, [`getResourceNullable(...)`: return type is declared non-nullable but the method returns a nullable value: call to Class.getResource(...) at line 177 (nullable according to nullsafe internal models).]