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); + } + } +}