Fix Contains for Distinct/Union with custom comparer

[stephentoub]

The recently-added optimization needs to factor in a non-default comparer.

PR Overview

This PR fixes the behavior of the Contains method in the LINQ Union and Distinct implementations to correctly handle custom comparers. Key changes include:

• Updating Union.SpeedOpt.cs to explicitly return false for default comparer cases instead of default.
• Modifying Distinct.SpeedOpt.cs to use a conditional expression that differentiates between default and custom comparer handling.
• Enhancing test coverage in ContainsTests.cs for both Union and Distinct with custom comparer scenarios.

Reviewed Changes

File	Description
src/libraries/System.Linq/src/System/Linq/Union.SpeedOpt.cs	Adjusts the Contains override to return false explicitly when using the default comparer.
src/libraries/System.Linq/src/System/Linq/Distinct.SpeedOpt.cs	Updates the Contains override to choose between _source.Contains(value) and base.Contains(value) based on the comparer.
src/libraries/System.Linq/tests/ContainsTests.cs	Adds new tests to verify behavior changes when a custom comparer is specified.

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

[dotnet-policy-service]

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

[eiriktsarpalis]

I'm guessing the reason we need to check both for null and EqualityComparer<TSource>.Default even though they functionally have the same behavior is because we use the latter representation for reference types?

[stephentoub]

We don't have to check for both; we could just check for null. But the latter is functionally equivalent and is substituted in for null in various places. I can remove it, though.

[eiriktsarpalis]

Could the iterator constructor just normalize to one of the two representations?

[IDisposable]

Why do we do a check for _comparer is null || _comparer == EqualityComparer<TSource>.Default
in Union.SpeedOpt.cs but only check _comparer is null in Distinct.SpeedOpt.cs?

Seems like we should do the same test in both places.

[stephentoub]

Why do we do a check for _comparer is null || _comparer == EqualityComparer<TSource>.Default in Union.SpeedOpt.cs but only check _comparer is null in Distinct.SpeedOpt.cs?

Seems like we should do the same test in both places.

I deleted it from both but apparently didn't push the update that deleted it from one of them. Would you like to submit a pr that temoves the extra check?

[IDisposable]

I have only a slight bias toward checking for .Default in both places, but if eliding it is the preference I can delete it in .Union I welcome a decision either way from @stephentoub and/or @eiriktsarpalis :) Will be happy to add a PR then ;)

Also, it seems a bit "fragile" / "insider knowledge" in the ContainsTests.cs that the survivor will be the first value encountered (e.g. "a" will be what remains after the .Distinct or .Union on line 264 and 286). Is that something that is disclosed/guaranteed?

[IDisposable]

I have only a slight bias toward checking for .Default in both places

I realize I should explain this bias. Since having _comparer is null means the same thing as/causes the EqualityComparer<TSource>.Default to be used, it would seem checking for just is null would be enough, but what happens when the Enumerable is created with an explicit use of EqualityComparer<TSource>.Default passed in the constructor? In that case, we're not going to trigger this speedopt, which seems suboptimal.

[stephentoub]

I have only a slight bias toward checking for .Default in both places

I realize I should explain this bias. Since having _comparer is null means the same thing as/causes the EqualityComparer<TSource>.Default to be used, it would seem checking for just is null would be enough, but what happens when the Enumerable is created with an explicit use of EqualityComparer<TSource>.Default passed in the constructor? In that case, we're not going to trigger this speedopt, which seems suboptimal.

The flip side is that it's an extra check, and it's not super common for EqualityComparer.Default to be explicitly passed in. The initial code I'd added had both checks, and I got feedback to cull it back to just the null check, so that's what we'd like it to be. I just somehow missed updating both locations.

[IDisposable]

PR #112973 to remove the test of .Default