THRIFT-6493: Compare and hash netstd union values by the member they hold

compiler/cpp/src/thrift/generate/t_netstd_generator.cc

@@ -1952,8 +1952,13 @@ void t_netstd_generator::generate_netstd_union_definition(ostream& out, t_struct
     out << indent() << "public override int GetHashCode()" << '\n';
     out << indent() << "{" << '\n';
     indent_up();
+    // as in the member classes, the ID of the field that is set goes in first
+    out << indent() << "int hashcode = 157;" << '\n';
+    out << indent() << "unchecked {" << '\n';
+    indent_up();
+    out << indent() << "hashcode = (hashcode * 397) + Isset;" << '\n';
     if(target_net_version >= 6) {
-        out << indent() << "return Isset switch" << '\n';
+        out << indent() << "hashcode = (hashcode * 397) + (Isset switch" << '\n';
         out << indent() << "{" << '\n';
         indent_up();
         for (f_iter = fields.begin(); f_iter != fields.end(); ++f_iter)
@@ -1967,7 +1972,7 @@ void t_netstd_generator::generate_netstd_union_definition(ostream& out, t_struct
         }
         out << indent() << "_ =>  (new ___undefined()).GetHashCode()" << '\n';
         indent_down();
-        out << indent() << "};" << '\n';
+        out << indent() << "});" << '\n';
     } else {
         out << indent() << "switch (Isset)" << '\n';
         out << indent() << "{" << '\n';
@@ -1977,21 +1982,27 @@ void t_netstd_generator::generate_netstd_union_definition(ostream& out, t_struct
             string null_coalesce(is_nullable_type((*f_iter)->get_type()) ? "?" : "");
             out << indent() << "case " << (*f_iter)->get_key() << ":" << '\n';
             indent_up();
-            out << indent() << "return As_" << (*f_iter)->get_name() << null_coalesce << ".GetHashCode()";
+            out << indent() << "hashcode = (hashcode * 397) + ";
             if( null_coalesce.size() > 0) {
-              out << " ?? 0";
+              out << "(As_" << (*f_iter)->get_name() << null_coalesce << ".GetHashCode() ?? 0);" << '\n';
+            } else {
+              out << "As_" << (*f_iter)->get_name() << ".GetHashCode();" << '\n';
             }
-            out << ";" << '\n';
+            out << indent() << "break;" << '\n';
             indent_down();
         }
         out << indent() << "default:" << '\n';
         indent_up();
-        out << indent() << "return (new ___undefined()).GetHashCode();" << '\n';
+        out << indent() << "hashcode = (hashcode * 397) + (new ___undefined()).GetHashCode();" << '\n';
+        out << indent() << "break;" << '\n';
         indent_down();
         indent_down();
         out << indent() << "}" << '\n';
     }
     indent_down();
+    out << indent() << "}" << '\n';
+    out << indent() << "return hashcode;" << '\n';
+    indent_down();
     out << indent() << "}" << '\n' << '\n';
 
     if( ! suppress_deepcopy) {
@@ -2131,14 +2142,24 @@ void t_netstd_generator::generate_netstd_union_class(ostream& out, t_struct* tun
     }
     out << indent() << "if (ReferenceEquals(this, other)) return true;" << '\n';
     out << '\n';
+    out << indent() << "if(this.Isset != other.Isset) return false;" << '\n';
+    out << '\n';
     out << indent() << "return Equals( _data, other.As_" << tfield->get_name() << ");" << '\n';
     indent_down();
     out << indent() << "}" << '\n' << '\n';
 
     out << indent() << "public override int GetHashCode()" << '\n';
     out << indent() << "{" << '\n';
     indent_up();
-    out << indent() << "return _data.GetHashCode();" << '\n';
+    // the field ID goes in first, so that the same value hashes differently in another member
+    out << indent() << "int hashcode = 157;" << '\n';
+    out << indent() << "unchecked {" << '\n';
+    indent_up();
+    out << indent() << "hashcode = (hashcode * 397) + " << tfield->get_key() << ";" << '\n';
+    out << indent() << "hashcode = (hashcode * 397) + _data.GetHashCode();" << '\n';
+    indent_down();
+    out << indent() << "}" << '\n';
+    out << indent() << "return hashcode;" << '\n';
     indent_down();
     out << indent() << "}" << '\n' << '\n';
 

lib/netstd/Tests/Thrift.Compile.Tests/Thrift.Compile.net10/Thrift.Compile.net10.csproj

@@ -87,6 +87,7 @@
     <Exec Command="$(PathToThrift) -gen netstd:wcf,union,serial,net10 -r ../Thrift6198.thrift" />
     <Exec Command="$(PathToThrift) -gen netstd:wcf,union,serial,net10 -r ../Thrift6199.thrift" />
     <Exec Command="$(PathToThrift) -gen netstd:wcf,union,serial,net10 -r ../Thrift6474.thrift" />
+    <Exec Command="$(PathToThrift) -gen netstd:wcf,union,serial,net10 -r ../Thrift6493.thrift" />
     <!-- special options (see ticket) -->
     <Exec Command="$(PathToThrift) -gen netstd:net10                  -r ../Thrift5794.thrift" />
   </Target>

lib/netstd/Tests/Thrift.Compile.Tests/Thrift.Compile.net8/Thrift.Compile.net8.csproj

@@ -88,6 +88,7 @@
     <Exec Command="$(PathToThrift) -gen netstd:wcf,union,serial,net8 -r ../Thrift6198.thrift" />
     <Exec Command="$(PathToThrift) -gen netstd:wcf,union,serial,net8 -r ../Thrift6199.thrift" />
     <Exec Command="$(PathToThrift) -gen netstd:wcf,union,serial,net8 -r ../Thrift6474.thrift" />
+    <Exec Command="$(PathToThrift) -gen netstd:wcf,union,serial,net8 -r ../Thrift6493.thrift" />
     <!-- special options (see ticket) -->
     <Exec Command="$(PathToThrift) -gen netstd:net8                  -r ../Thrift5794.thrift" />
   </Target>

lib/netstd/Tests/Thrift.Compile.Tests/Thrift.Compile.net9/Thrift.Compile.net9.csproj

@@ -88,6 +88,7 @@
     <Exec Command="$(PathToThrift) -gen netstd:wcf,union,serial,net9 -r ../Thrift6198.thrift" />
     <Exec Command="$(PathToThrift) -gen netstd:wcf,union,serial,net9 -r ../Thrift6199.thrift" />
     <Exec Command="$(PathToThrift) -gen netstd:wcf,union,serial,net9 -r ../Thrift6474.thrift" />
+    <Exec Command="$(PathToThrift) -gen netstd:wcf,union,serial,net9 -r ../Thrift6493.thrift" />
     <!-- special options (see ticket) -->
     <Exec Command="$(PathToThrift) -gen netstd:net9                  -r ../Thrift5794.thrift" />
   </Target>

lib/netstd/Tests/Thrift.Compile.Tests/Thrift.Compile.netstd2/Thrift.Compile.netstd2.csproj

@@ -79,6 +79,7 @@
     <Exec Command="$(PathToThrift) -gen netstd:union,serial -r ../Thrift6198.thrift" />
     <Exec Command="$(PathToThrift) -gen netstd:union,serial -r ../Thrift6199.thrift" />
     <Exec Command="$(PathToThrift) -gen netstd:union,serial -r ../Thrift6474.thrift" />
+    <Exec Command="$(PathToThrift) -gen netstd:union,serial -r ../Thrift6493.thrift" />
     <!-- special options (see ticket) -->
     <Exec Command="$(PathToThrift) -gen netstd                  -r ../Thrift5794.thrift" />
   </Target>

lib/netstd/Tests/Thrift.Compile.Tests/Thrift6493.thrift

@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+// Testcase for THRIFT-6493 netstd: generated unions compare Equals without regard to which member is set
+
+namespace * Thrift6493
+
+union U {
+  1: i32 x
+  2: i32 y
+  3: string s
+}

lib/netstd/Tests/Thrift.Tests/DataModel/UnionEquality.cs

@@ -0,0 +1,76 @@
+// Licensed to the Apache Software Foundation(ASF) under one
+// or more contributor license agreements.See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+using System.Collections.Generic;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+using Thrift6493;
+
+namespace Thrift.Tests.DataModel
+{
+    // THRIFT-6493: two values of a union are equal only if they hold the same member, with equal
+    // values, and the hash code of a value depends on which member it holds
+    [TestClass]
+    public class UnionEqualityTests
+    {
+        [TestMethod]
+        public void DifferentMembersWithDefaultValues_AreNotEqual()
+        {
+            Assert.IsFalse(new U.x(0).Equals(new U.y(0)));
+            Assert.IsFalse(new U.y(0).Equals(new U.x(0)));
+        }
+
+        [TestMethod]
+        public void DifferentMembers_AreNotEqualInEitherDirection()
+        {
+            Assert.IsFalse(new U.x(0).Equals(new U.s("text")));
+            Assert.IsFalse(new U.s("text").Equals(new U.x(0)));
+        }
+
+        [TestMethod]
+        public void DifferentMembersWithEqualValues_HashDifferently()
+        {
+            Assert.AreNotEqual(new U.x(5).GetHashCode(), new U.y(5).GetHashCode());
+        }
+
+        [TestMethod]
+        public void EqualValues_AreEqualAndHashEqually()
+        {
+            var pairs = new (U, U)[]
+            {
+                (new U.x(0), new U.x(0)),
+                (new U.x(5), new U.x(5)),
+                (new U.y(5), new U.y(5)),
+                (new U.s("text"), new U.s("text")),
+                (new U.x(5), new U.x(5).DeepCopy()),
+                (new U.___undefined(), new U.___undefined()),
+            };
+            foreach (var (first, second) in pairs)
+            {
+                Assert.IsTrue(first.Equals(second), $"{first} equals {second}");
+                Assert.IsTrue(second.Equals(first), $"{second} equals {first}");
+                Assert.AreEqual(first.GetHashCode(), second.GetHashCode(), $"hash codes of {first} and {second}");
+            }
+        }
+
+        [TestMethod]
+        public void DifferentMembers_AreKeptApartInAHashSet()
+        {
+            var set = new HashSet<U> { new U.x(0), new U.y(0), new U.x(5), new U.y(5), new U.s("text"), new U.x(5) };
+            Assert.HasCount(5, set);
+        }
+    }
+}