Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[release/8.0] Fix Item4 is missing in some ValueTuples' IStructuralEquatable.Equals #91470

Merged
merged 2 commits into from
Sep 12, 2023

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Sep 1, 2023

Backport of #91461 to release/8.0

Customer Impact

Some ValueTuples' IStructuralEquatable.Equals methods skip comparing Item4. This could lead to data loss or other functional issues in customer applications where this equality check drives application logic or persistence of data.

bool IStructuralEquatable.Equals(object? other, IEqualityComparer comparer) =>
other is ValueTuple<T1, T2, T3, T4, T5> vt &&
comparer.Equals(Item1, vt.Item1) &&
comparer.Equals(Item2, vt.Item2) &&
comparer.Equals(Item3, vt.Item3) &&
comparer.Equals(Item5, vt.Item5);

Search vt.Item3) && in that file to find more.

Reproduction Steps

using System.Collections;

IStructuralEquatable a = ValueTuple.Create(1, 2, 3, 4, 5);
IStructuralEquatable b = ValueTuple.Create(1, 2, 3, 0, 5);

Console.WriteLine(a.Equals(b, EqualityComparer<object>.Default));
await Task.Delay(-1);

Expected behavior

False

Actual behavior

True

Testing

Unit test coverage gaps were filled and other ValueTuple items were checked to ensure they're being properly compared.

Risk

Low. This will correct a functional bug in a way that will alter application behavior, but it will correct the behavior.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 1, 2023
@jeffhandley jeffhandley self-assigned this Sep 1, 2023
@jeffhandley jeffhandley added area-System.Runtime and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 1, 2023
@ghost
Copy link

ghost commented Sep 1, 2023

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Backport of #91461 to release/8.0

Customer Impact

Some ValueTuples' IStructuralEquatable.Equals methods skip comparing Item4. This could lead to data loss or other functional issues in customer applications where this equality check drives application logic or persistence of data.

bool IStructuralEquatable.Equals(object? other, IEqualityComparer comparer) =>
other is ValueTuple<T1, T2, T3, T4, T5> vt &&
comparer.Equals(Item1, vt.Item1) &&
comparer.Equals(Item2, vt.Item2) &&
comparer.Equals(Item3, vt.Item3) &&
comparer.Equals(Item5, vt.Item5);

Search vt.Item3) && in that file to find more.

Reproduction Steps

using System.Collections;

IStructuralEquatable a = ValueTuple.Create(1, 2, 3, 4, 5);
IStructuralEquatable b = ValueTuple.Create(1, 2, 3, 0, 5);

Console.WriteLine(a.Equals(b, EqualityComparer<object>.Default));
await Task.Delay(-1);

Expected behavior

False

Actual behavior

True

Testing

Unit test coverage gaps were filled and other ValueTuple items were checked to ensure they're being properly compared.

Risk

Low. This will correct a functional bug in a way that will alter application behavior, but it will correct the behavior.

Author: github-actions[bot]
Assignees: jeffhandley
Labels:

area-System.Runtime

Milestone: -

@jeffhandley jeffhandley added this to the 8.0.0 milestone Sep 1, 2023
@jeffhandley jeffhandley added the Servicing-consider Issue for next servicing release review label Sep 1, 2023
@carlossanlop
Copy link
Member

8.0 only requires M2 approval until the day before the RC2 snap. @jeffhandley / @artl93 do you approve?

Copy link
Contributor

@artl93 artl93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

M2 Approved. I'm sure @jeffhandley would agree. 

@artl93 artl93 added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 11, 2023
@carlossanlop
Copy link
Member

Merging this now. We will merge the 7.0 and 6.0 equivalent PRs in a later servicing release.

@carlossanlop carlossanlop merged commit 4adb83e into release/8.0 Sep 12, 2023
172 of 177 checks passed
@carlossanlop carlossanlop deleted the backport/pr-91461-to-release/8.0 branch September 12, 2023 15:27
@radical radical mentioned this pull request Sep 26, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Runtime Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants