Skip to content

Conversation

@stephentoub
Copy link
Member

Closes #28934

@ghost
Copy link

ghost commented Nov 26, 2024

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Nov 26, 2024

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@stephentoub stephentoub requested a review from Copilot December 13, 2024 14:48
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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

Files not reviewed (9)
  • src/libraries/System.Memory/tests/System.Memory.Tests.csproj: Language not supported
  • src/libraries/System.Memory/tests/ReadOnlySpan/IndexOfSequence.T.cs: Evaluated as low risk
  • src/libraries/System.Memory/tests/ReadOnlySpan/LastIndexOfSequence.T.cs: Evaluated as low risk
  • src/libraries/System.Memory/tests/ReadOnlySpan/EndsWith.T.cs: Evaluated as low risk
  • src/libraries/System.Memory/tests/ReadOnlySpan/SequenceCompareTo.T.cs: Evaluated as low risk
  • src/libraries/System.Memory/tests/ReadOnlySpan/LastIndexOf.T.cs: Evaluated as low risk
  • src/libraries/System.Memory/tests/ReadOnlySpan/IndexOf.T.cs: Evaluated as low risk
  • src/libraries/System.Memory/tests/ReadOnlySpan/StartsWith.T.cs: Evaluated as low risk
  • src/libraries/System.Memory/ref/System.Memory.cs: Evaluated as low risk
Comments suppressed due to low confidence (4)

src/libraries/System.Linq/src/System/Linq/Contains.cs:21

  • The new overload for Contains using a comparer should have corresponding unit tests to ensure it behaves correctly with different comparers.
if (source.TryGetSpan(out ReadOnlySpan<TSource> span))

src/libraries/System.Linq/src/System/Linq/Contains.cs:32

  • The check for value types should be removed as it is redundant. The logic for handling null comparers should be consistent regardless of whether TSource is a value type or not.
if (typeof(TSource).IsValueType)

src/libraries/System.Diagnostics.TraceSource/src/System/Diagnostics/TraceUtils.cs:22

  • The Contains method used here does not specify a comparer, which might lead to incorrect behavior if the default equality comparer is not appropriate for the type of elements in supportedAttributes.
if (supportedAttributes is null || !supportedAttributes.Contains(key))

src/libraries/System.Memory/tests/ReadOnlySpan/Contains.T.cs:124

  • The test should include a case where the comparer is null to ensure the method handles it correctly.
Assert.All(GetDefaultEqualityComparers<string>(), comparer => Assert.False(new ReadOnlySpan<string>(Array.Empty<string>()).Contains("a", comparer)));

Tip: If you use Visual Studio Code, you can request a review from Copilot before you push from the "Source Control" tab. Learn more

@stephentoub stephentoub requested a review from Copilot January 17, 2025 03:41
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 8 out of 17 changed files in this pull request and generated 1 comment.

Files not reviewed (9)
  • src/libraries/System.Memory/tests/System.Memory.Tests.csproj: Language not supported
  • src/libraries/System.Linq/src/System/Linq/Contains.cs: Evaluated as low risk
  • src/libraries/System.Memory/ref/System.Memory.cs: Evaluated as low risk
  • src/libraries/System.Memory/tests/ReadOnlySpan/Comparers.cs: Evaluated as low risk
  • src/libraries/System.Memory/tests/ReadOnlySpan/Contains.T.cs: Evaluated as low risk
  • src/libraries/System.Memory/tests/ReadOnlySpan/Count.T.cs: Evaluated as low risk
  • src/libraries/System.Diagnostics.TraceSource/src/System/Diagnostics/TraceUtils.cs: Evaluated as low risk
  • src/libraries/System.Memory/tests/ReadOnlySpan/IndexOfSequence.T.cs: Evaluated as low risk
  • src/libraries/System.Memory/tests/ReadOnlySpan/LastIndexOfSequence.T.cs: Evaluated as low risk
Comments suppressed due to low confidence (1)

src/libraries/System.Memory/tests/ReadOnlySpan/SequenceCompareTo.T.cs:140

  • The method 'GetDefaultComparers' is not defined in the provided code. Ensure that 'GetDefaultComparers' is defined and correctly implemented to avoid compilation errors.
Assert.All(GetDefaultComparers<string>(), comparer => Assert.Equal(0, new ReadOnlySpan<string>(a, 1, 0).SequenceCompareTo<string>(new ReadOnlySpan<string>(a, 2, 0), comparer)));

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

@stephentoub stephentoub merged commit b790824 into dotnet:main Jan 17, 2025
137 of 139 checks passed
@stephentoub stephentoub deleted the equalitycompareroverloads branch January 17, 2025 13:52
grendello added a commit to grendello/runtime that referenced this pull request Jan 20, 2025
* main: (89 commits)
  Add Dispose for X509Chain instance (dotnet#110740)
  Fix XML comment on regex split enumerator (dotnet#111572)
  JIT: tolerate missing InitClass map in SPMI (dotnet#111555)
  Build ilasm/ildasm packages for the host machine (dotnet#111512)
  Unicode 16.0 Support (dotnet#111469)
  Improve performance of interface method resolution in ILC (dotnet#103066)
  Fix building the host-targeting components and packing ILC (dotnet#111552)
  Improve JSON validation perf (dotnet#111332)
  Update github-merge-flow.jsonc to autoflow 9.0 to 9.0-staging (dotnet#111549)
  Include GPL-3 licence text in the notice (dotnet#111528)
  Remove explicit __compact_unwind entries from x64 assembler (dotnet#111530)
  Add MemoryExtensions overloads with comparer (dotnet#110197)
  Avoid capturing the ExecutionContext for the whole HTTP connection lifetime (dotnet#111475)
  Forward DefaultArtifactVisibility down from the VMR orchestrator (dotnet#111513)
  Fix relocs errors on riscv64 (dotnet#111317)
  Added JITDUMP_USE_ARCH_TIMESTAMP support. (dotnet#111359)
  add rcl/rcr tp and latency info (dotnet#111442)
  Fix stack overflow in compiler-generated state (dotnet#109207)
  Produce a package with the host-running ILC for repos in the VMR (dotnet#111443)
  Delete dead code in ilasm PE writer (dotnet#111218)
  ...
@roji
Copy link
Member

roji commented Jan 30, 2025

@stephentoub this unfortunately breaks some very basic EF usage: the new overload with the optional comparer parameter gets resolved by the compiler, but LINQ expression trees don't support method invocations with optional parameters (see dotnet/efcore#35547). This also unfortunately has an incredibly big blast radius, since anyone using Contains in any LINQ provider would be affected.

A short-term quick fix could be to replace the overload with two overloads, one with and one without the comparer (basically avoid using optional parameters). But longer term, I think we need to discuss evolving expression tree support, at least around this specific point.

@ChrisJollyAU
Copy link

But longer term, I think we need to discuss evolving expression tree support, at least around this specific point.

I'll second that

@github-actions github-actions bot locked and limited conversation to collaborators Mar 4, 2025
@ericstj ericstj added the breaking-change Issue or PR that represents a breaking API or functional change over a previous release. label Nov 13, 2025
@dotnet-policy-service dotnet-policy-service bot added the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Nov 13, 2025
@ericstj ericstj removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Nov 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-System.Memory breaking-change Issue or PR that represents a breaking API or functional change over a previous release. new-api-needs-documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MemoryExtensions.*(..., IEqualityComparer) overloads

5 participants