Skip to content

[release/7.0-staging] Fix Item4 is missing in some ValueTuples' IStructuralEquatable.Equals #91471

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

Conversation

github-actions[bot]
Copy link
Contributor

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

Backport of #91461 to release/7.0-staging

/cc @jeffhandley @hamarb123 @skyoxZ

Customer Impact

When ValueTuple has 5 or more items, the bool IStructuralEquatable.Equals(object? other, IEqualityComparer comparer) implementation skips over Item4. This leads to instances returning as equal when they are not.

using System.Collections;

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

// Any comparer will result in the buggy behavior
var comparer = StructuralComparisons.StructuralEqualityComparer;
var equal = ((IStructuralEquatable)a).Equals(b, comparer);

This was reported by a customer in #91457 and fixed in .NET 8 with #91461. This latent behavioral bug could be silently affecting customer applications with potential data loss and data corruption issues. The bug only surfaces when explicitly using IStructuralEquatable.Equals with a comparer provided. That usage pattern would be most prevalent within framework and library usage where it could be hard for customers to diagnose or work around the issue if encountered.

A GitHub code search revealed signal of such usage patterns being used. Here are a couple examples: 1, 2

Testing

The ValueTuple implementation was scanned for other errors like this and they were all fixed together. Unit tests were added to ensure each item of the tuple affects equality.

Risk

Medium. This is a behavioral correction to a latent bug. It's plausible that data has been persisted based on the incorrect behavior; in such a case, the behavioral correction might surface for customers.

@ghost ghost 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
@jeffhandley jeffhandley added this to the 7.0.x milestone 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/7.0-staging

/cc @jeffhandley @hamarb123

Customer Impact

Testing

Risk

IMPORTANT: If this backport is for a servicing release, please verify that:

  • The PR target branch is release/X.0-staging, not release/X.0.

  • If the change touches code that ships in a NuGet package, you have added the necessary package authoring and gotten it explicitly reviewed.

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

area-System.Runtime

Milestone: -

@carlossanlop
Copy link
Contributor

@jeffhandley @stephentoub today is Code Complete for the October release. This backport does not have the servicing-approved label yet. If you want this included in the October release, please get Tactics approval and merge it in the next few hours, otherwise it will have to wait until the November release.

Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

This has my support for servicing into 7.0. It's a latent functional bug that could lead to notable customer impact.

Adding @artl93 for review/approval.

Copy link
Member

@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.

@carlossanlop
Copy link
Contributor

From the email conversation, we will wait for this to cook a bit in 8.0, then we will service this to 7.0 and 6.0 in the next servicing release.

@Saibamen
Copy link

So why are you closed this PR?
@carlossanlop said it should be merged after some time. Please reopen this, so nobody will forget about this

@jkotas jkotas deleted the backport/pr-91461-to-release/7.0-staging branch September 16, 2023 13:20
@ghost ghost locked as resolved and limited conversation to collaborators Oct 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants