[pulse] apply equality relation to terms to be added to the equality relation

Summary:
Extra normalization gives extra precision. This doesn't seem to
negatively impact perf.

Reviewed By: skcho

Differential Revision: D22867109

fbshipit-source-id: 5b82ec377

infer/src/pulse/PulseFormula.ml

@@ -638,14 +638,25 @@ end = struct
         acc
     | False ->
         raise Contradiction
-    | Atom (Equal (t1, t2)) ->
-        (* NOTE: this should also normalize sub-terms of [t1] and [t2] according to the equality
-           relation. We don't do that yet. *)
-        let uf = UnionFind.union uf t1 t2 in
-        (* change facts into atoms when the equality relation changes so they will be normalized
-           again later using the new equality relation *)
-        let atoms_with_facts = Atom.Set.fold (fun atom atoms -> atom :: atoms) facts atoms in
-        (uf, Atom.Set.empty, atoms_with_facts)
+    | Atom (Equal _ as atom) ->
+        (* Normalize the terms of the equality w.r.t. the equalities we have discovered so far. Note
+           that we don't go back and normalize the existing equalities w.r.t. the new atom, which is
+           dodgy. Doing so could have adverse perf implications.
+
+           Note also that other (non-[Equal]) atoms are not yet normalized, this will happen after
+           [gather_congruences_and_facts] has run. *)
+        apply_atom uf atom
+        |> Option.value_map ~default:acc ~f:(function
+             | Atom.Equal (t1, t2) ->
+                 let uf = UnionFind.union uf t1 t2 in
+                 (* change facts into atoms when the equality relation changes so they will be normalized
+                    again later using the new equality relation *)
+                 let atoms_with_facts =
+                   Atom.Set.fold (fun atom atoms -> atom :: atoms) facts atoms
+                 in
+                 (uf, Atom.Set.empty, atoms_with_facts)
+             | atom ->
+                 (uf, facts, atom :: atoms) )
     | Atom atom ->
         (uf, facts, atom :: atoms)
     | And (phi1, phi2) ->

infer/src/pulse/unit/PulseFormulaTest.ml

@@ -44,5 +44,5 @@ let%test_module _ =
 
     let%expect_test _ =
       normalize (x = y && y = z && z = i 0 && x = i 1) ;
-      [%expect "\n        [0=1=v1=v2=v3 &&\n        ]"]
+      [%expect "\n        false"]
   end )

infer/tests/codetoanalyze/cpp/pulse/issues.exp

@@ -79,7 +79,6 @@ codetoanalyze/cpp/pulse/use_after_free.cpp, double_free_global_bad, 2, USE_AFTER
 codetoanalyze/cpp/pulse/use_after_free.cpp, double_free_simple_bad, 2, USE_AFTER_FREE, no_bucket, ERROR, [invalidation part of the trace starts here,parameter `x` of double_free_simple_bad,was invalidated by call to `free()`,use-after-lifetime part of the trace starts here,parameter `x` of double_free_simple_bad,invalid access occurs here]
 codetoanalyze/cpp/pulse/use_after_free.cpp, use_after_free_simple_bad, 2, USE_AFTER_FREE, no_bucket, ERROR, [invalidation part of the trace starts here,parameter `x` of use_after_free_simple_bad,was invalidated by call to `free()`,use-after-lifetime part of the trace starts here,parameter `x` of use_after_free_simple_bad,invalid access occurs here]
 codetoanalyze/cpp/pulse/use_after_scope.cpp, access_out_of_scope_stack_ref_bad, 3, USE_AFTER_LIFETIME, no_bucket, ERROR, [invalidation part of the trace starts here,variable `p` declared here,when calling `invalidate_local_ok` here,variable `t` declared here,is the address of a stack variable `t` whose lifetime has ended,use-after-lifetime part of the trace starts here,variable `p` declared here,passed as argument to `invalidate_local_ok`,variable `t` declared here,assigned,return from call to `invalidate_local_ok`,invalid access occurs here]
-codetoanalyze/cpp/pulse/values.cpp, FP_function_empty_range_interproc_ok, 2, NULLPTR_DEREFERENCE, no_bucket, ERROR, [invalidation part of the trace starts here,when calling `StringRange::StringRange` here,assigned,is the null pointer,use-after-lifetime part of the trace starts here,passed as argument to `StringRange::StringRange`,assigned,return from call to `StringRange::StringRange`,when calling `find_first_non_space` here,parameter `x` of find_first_non_space,passed as argument to `StringRange::data`,return from call to `StringRange::data`,assigned,invalid access occurs here]
 codetoanalyze/cpp/pulse/values.cpp, error_under_true_conditionals_bad, 5, USE_AFTER_FREE, no_bucket, ERROR, [invalidation part of the trace starts here,parameter `x` of error_under_true_conditionals_bad,was invalidated by call to `free()`,use-after-lifetime part of the trace starts here,parameter `x` of error_under_true_conditionals_bad,invalid access occurs here]
 codetoanalyze/cpp/pulse/values.cpp, free_if_deref_bad, 2, USE_AFTER_FREE, no_bucket, ERROR, [invalidation part of the trace starts here,parameter `x` of free_if_deref_bad,when calling `free_if` here,parameter `x` of free_if,was invalidated by call to `free()`,use-after-lifetime part of the trace starts here,parameter `x` of free_if_deref_bad,invalid access occurs here]
 codetoanalyze/cpp/pulse/vector.cpp, FP_init_fill_then_push_back_loop_ok, 6, VECTOR_INVALIDATION, no_bucket, ERROR, [invalidation part of the trace starts here,variable `vec` declared here,was potentially invalidated by `std::vector::push_back()`,use-after-lifetime part of the trace starts here,variable `vec` declared here,passed as argument to `std::vector::at()` (modelled),return from call to `std::vector::at()` (modelled),assigned,invalid access occurs here]

infer/tests/codetoanalyze/cpp/pulse/values.cpp

@@ -111,7 +111,7 @@ void find_first_non_space(StringRange& x) {
   }
 }
 
-void FP_function_empty_range_interproc_ok() {
+void function_empty_range_interproc_ok() {
   StringRange x{};
   find_first_non_space(x);
 }