From 7a41ed57e7597c9f40ce7d8727833132d05c7d6a Mon Sep 17 00:00:00 2001
From: Yitzhak Mandelbaum <yitzhakm@google.com>
Date: Tue, 2 Jan 2024 19:27:21 +0000
Subject: [PATCH] [clang][dataflow] Fix bug in `Value` comparison.

Makes value equivalence require that the values have no properties, except in
the case of equivalence by pointer equality (if the pointers are equal, nothing
else is checked).

Fixes issue #76459.
---
 clang/lib/Analysis/FlowSensitive/Value.cpp    | 14 +++++++--
 .../Analysis/FlowSensitive/ValueTest.cpp      | 30 +++++++++++++++++--
 2 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/clang/lib/Analysis/FlowSensitive/Value.cpp b/clang/lib/Analysis/FlowSensitive/Value.cpp
index 349f873f1e6c4d..7fad6deb0e918f 100644
--- a/clang/lib/Analysis/FlowSensitive/Value.cpp
+++ b/clang/lib/Analysis/FlowSensitive/Value.cpp
@@ -27,9 +27,17 @@ static bool areEquivalentIndirectionValues(const Value &Val1,
 }
 
 bool areEquivalentValues(const Value &Val1, const Value &Val2) {
-  return &Val1 == &Val2 || (Val1.getKind() == Val2.getKind() &&
-                            (isa<TopBoolValue>(&Val1) ||
-                             areEquivalentIndirectionValues(Val1, Val2)));
+  if (&Val1 == &Val2)
+    return true;
+  if (Val1.getKind() != Val2.getKind())
+    return false;
+  // If values are distinct and have properties, we don't consider them equal,
+  // leaving equality up to the user model.
+  if (!Val1.properties().empty() || !Val2.properties().empty())
+    return false;
+  if (isa<TopBoolValue>(&Val1))
+    return true;
+  return areEquivalentIndirectionValues(Val1, Val2);
 }
 
 raw_ostream &operator<<(raw_ostream &OS, const Value &Val) {
diff --git a/clang/unittests/Analysis/FlowSensitive/ValueTest.cpp b/clang/unittests/Analysis/FlowSensitive/ValueTest.cpp
index c5d18ba74c3ed6..b07328d2a56215 100644
--- a/clang/unittests/Analysis/FlowSensitive/ValueTest.cpp
+++ b/clang/unittests/Analysis/FlowSensitive/ValueTest.cpp
@@ -45,7 +45,20 @@ TEST(ValueTest, TopsEquivalent) {
   EXPECT_TRUE(areEquivalentValues(V2, V1));
 }
 
-TEST(ValueTest, EquivalentValuesWithDifferentPropsEquivalent) {
+// The framework does not (currently) consider equivalence for values with
+// properties, leaving such to individual analyses.
+TEST(ValueTest, ValuesWithSamePropsDifferent) {
+  Arena A;
+  TopBoolValue Prop(A.makeAtomRef(Atom(0)));
+  TopBoolValue V1(A.makeAtomRef(Atom(2)));
+  TopBoolValue V2(A.makeAtomRef(Atom(3)));
+  V1.setProperty("foo", Prop);
+  V2.setProperty("foo", Prop);
+  EXPECT_FALSE(areEquivalentValues(V1, V2));
+  EXPECT_FALSE(areEquivalentValues(V2, V1));
+}
+
+TEST(ValueTest, ValuesWithDifferentPropsDifferent) {
   Arena A;
   TopBoolValue Prop1(A.makeAtomRef(Atom(0)));
   TopBoolValue Prop2(A.makeAtomRef(Atom(1)));
@@ -53,8 +66,19 @@ TEST(ValueTest, EquivalentValuesWithDifferentPropsEquivalent) {
   TopBoolValue V2(A.makeAtomRef(Atom(3)));
   V1.setProperty("foo", Prop1);
   V2.setProperty("bar", Prop2);
-  EXPECT_TRUE(areEquivalentValues(V1, V2));
-  EXPECT_TRUE(areEquivalentValues(V2, V1));
+  EXPECT_FALSE(areEquivalentValues(V1, V2));
+  EXPECT_FALSE(areEquivalentValues(V2, V1));
+}
+
+TEST(ValueTest, ValuesWithDifferentNumberPropsDifferent) {
+  Arena A;
+  TopBoolValue Prop(A.makeAtomRef(Atom(0)));
+  TopBoolValue V1(A.makeAtomRef(Atom(2)));
+  TopBoolValue V2(A.makeAtomRef(Atom(3)));
+  // Only set a property on `V1`.
+  V1.setProperty("foo", Prop);
+  EXPECT_FALSE(areEquivalentValues(V1, V2));
+  EXPECT_FALSE(areEquivalentValues(V2, V1));
 }
 
 TEST(ValueTest, DifferentKindsNotEquivalent) {