[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

@@ -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 =
+  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
-  | [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
+        | [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
+      (* 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 =

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

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;

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