From d2ce09e49b4d0d82bacba749db9303ecbdc9fc8c Mon Sep 17 00:00:00 2001 From: Jens Geyer Date: Thu, 5 Nov 2026 09:32:50 +0100 Subject: [PATCH] THRIFT-6493: Compare and hash netstd union values by the member they hold Client: netstd The classes of the union members compared only the values in Equals, so values of different members could be equal, and GetHashCode() ignored the member. Equals now checks which member is set, and the hash code adds its field ID. Co-Authored-By: Claude Opus 5.5 --- .../src/thrift/generate/t_netstd_generator.cc | 35 +++++++-- .../Thrift.Compile.net10.csproj | 1 + .../Thrift.Compile.net8.csproj | 1 + .../Thrift.Compile.net9.csproj | 1 + .../Thrift.Compile.netstd2.csproj | 1 + .../Thrift.Compile.Tests/Thrift6493.thrift | 28 +++++++ .../Thrift.Tests/DataModel/UnionEquality.cs | 76 +++++++++++++++++++ 7 files changed, 136 insertions(+), 7 deletions(-) create mode 100644 lib/netstd/Tests/Thrift.Compile.Tests/Thrift6493.thrift create mode 100644 lib/netstd/Tests/Thrift.Tests/DataModel/UnionEquality.cs diff --git a/compiler/cpp/src/thrift/generate/t_netstd_generator.cc b/compiler/cpp/src/thrift/generate/t_netstd_generator.cc index 92b0f6830df..c26a794fad7 100644 --- a/compiler/cpp/src/thrift/generate/t_netstd_generator.cc +++ b/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,6 +2142,8 @@ 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'; @@ -2138,7 +2151,15 @@ void t_netstd_generator::generate_netstd_union_class(ostream& out, t_struct* tun 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'; diff --git a/lib/netstd/Tests/Thrift.Compile.Tests/Thrift.Compile.net10/Thrift.Compile.net10.csproj b/lib/netstd/Tests/Thrift.Compile.Tests/Thrift.Compile.net10/Thrift.Compile.net10.csproj index f51faf6d4a9..f36eb20fc77 100644 --- a/lib/netstd/Tests/Thrift.Compile.Tests/Thrift.Compile.net10/Thrift.Compile.net10.csproj +++ b/lib/netstd/Tests/Thrift.Compile.Tests/Thrift.Compile.net10/Thrift.Compile.net10.csproj @@ -87,6 +87,7 @@ + diff --git a/lib/netstd/Tests/Thrift.Compile.Tests/Thrift.Compile.net8/Thrift.Compile.net8.csproj b/lib/netstd/Tests/Thrift.Compile.Tests/Thrift.Compile.net8/Thrift.Compile.net8.csproj index 4beeefdf8bf..204d2c145d1 100644 --- a/lib/netstd/Tests/Thrift.Compile.Tests/Thrift.Compile.net8/Thrift.Compile.net8.csproj +++ b/lib/netstd/Tests/Thrift.Compile.Tests/Thrift.Compile.net8/Thrift.Compile.net8.csproj @@ -88,6 +88,7 @@ + diff --git a/lib/netstd/Tests/Thrift.Compile.Tests/Thrift.Compile.net9/Thrift.Compile.net9.csproj b/lib/netstd/Tests/Thrift.Compile.Tests/Thrift.Compile.net9/Thrift.Compile.net9.csproj index 555be8ff80a..99606756d08 100644 --- a/lib/netstd/Tests/Thrift.Compile.Tests/Thrift.Compile.net9/Thrift.Compile.net9.csproj +++ b/lib/netstd/Tests/Thrift.Compile.Tests/Thrift.Compile.net9/Thrift.Compile.net9.csproj @@ -88,6 +88,7 @@ + diff --git a/lib/netstd/Tests/Thrift.Compile.Tests/Thrift.Compile.netstd2/Thrift.Compile.netstd2.csproj b/lib/netstd/Tests/Thrift.Compile.Tests/Thrift.Compile.netstd2/Thrift.Compile.netstd2.csproj index f7dea3c525a..66fa830f929 100644 --- a/lib/netstd/Tests/Thrift.Compile.Tests/Thrift.Compile.netstd2/Thrift.Compile.netstd2.csproj +++ b/lib/netstd/Tests/Thrift.Compile.Tests/Thrift.Compile.netstd2/Thrift.Compile.netstd2.csproj @@ -79,6 +79,7 @@ + diff --git a/lib/netstd/Tests/Thrift.Compile.Tests/Thrift6493.thrift b/lib/netstd/Tests/Thrift.Compile.Tests/Thrift6493.thrift new file mode 100644 index 00000000000..85aec968bac --- /dev/null +++ b/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 +} diff --git a/lib/netstd/Tests/Thrift.Tests/DataModel/UnionEquality.cs b/lib/netstd/Tests/Thrift.Tests/DataModel/UnionEquality.cs new file mode 100644 index 00000000000..04ec51b6848 --- /dev/null +++ b/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 { 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); + } + } +}