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

Added AsReadOnly() extension to ISet<T> #106037

Merged
merged 2 commits into from
Nov 25, 2024

Conversation

Mrxx99
Copy link
Contributor

@Mrxx99 Mrxx99 commented Aug 6, 2024

this adds the asReadOnly extension method to ISet
unfortunately CollectionExtensions does not find ReadOnlySet, I guess it either has to be moved to System.Private.CoreLib and/or type forwarders have to be added. But I don't know how to do that.

fixes #29387

Copy link

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.

1 similar comment
Copy link

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.

@Mrxx99 Mrxx99 marked this pull request as draft August 6, 2024 19:11
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Aug 6, 2024
@stephentoub
Copy link
Member

I guess it either has to be moved to System.Private.CoreLib

Yes, the implementation of ReadOnlySet<T> would need to be moved down to corelib. From a reference assembly perspective, it's in System.Collections.dll along with CollectionExtensions, so it's only an implementation issue. You can just move the implementation down.

@Mrxx99
Copy link
Contributor Author

Mrxx99 commented Aug 6, 2024

You can just move the implementation down.

Unfortunately I still get the following error:

C:\Code\Other\dotnet\runtime\src\libraries\System.Private.CoreLib\src\System\Collections\Generic\CollectionExtensions.cs(76
,23): error CS0246: The type or namespace name 'ReadOnlySet<>' could not be found (are you missing a using directive or an
assembly reference?) [C:\Code\Other\dotnet\runtime\src\coreclr\nativeaot\System.Private.CoreLib\src\System.Private.CoreLib.
csproj]

@stephentoub
Copy link
Member

Unfortunately I still get the following error:

This is editing the wrong System.Private.CoreLib project file. You instead want to add it to https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System.Private.CoreLib.Shared.projitems.

CoreLib for coreclr, mono, and native aot is slightly different for each. Most of the source is the same, and that shared source is listed in the above projitems file. The file you edited is the one that contains just the additional source specific to coreclr.

@eiriktsarpalis
Copy link
Member

@stephentoub @artl93 can we make an exception and slot this API into .NET 9? It's low risk and complements the newly introduced ReadOnlySet<T>.

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks

@GerardSmit
Copy link

GerardSmit commented Sep 6, 2024

Sorry if I'm misunderstanding this, but if the type is moved from System.Collections to System.Private.CoreLib and this is landing in .NET 10 instead of .NET 9 (or for libraries built for .NET 9.0 preview), shouldn't there be a TypeForward?

@Mrxx99
Copy link
Contributor Author

Mrxx99 commented Oct 22, 2024

@stephentoub Is there something missing that is holding this back?

@eiriktsarpalis
Copy link
Member

This looks ready to merge. I've updated the PR branch to give this another run with more recent bits.

@stephentoub
Copy link
Member

This looks ready to merge. I've updated the PR branch to give this another run with more recent bits.

I believe it will need to updated with type forwards, as suggested in #106037 (comment).

@eiriktsarpalis
Copy link
Member

@Mrxx99 when possible, could you take a look at the outstanding issue?

@Mrxx99
Copy link
Contributor Author

Mrxx99 commented Nov 18, 2024

@Mrxx99 when possible, could you take a look at the outstanding issue?

I can take a look but I am not familiar with the type forwarders so that could be much work and still would need to be reviewed by one knowledgeable person anyway. Maybe it would be easier if some expert would do it directly.

@stephentoub stephentoub force-pushed the feature/ISetAsReadOnlyExtension branch from 6a0f7b3 to ee5d655 Compare November 25, 2024 16:45
@stephentoub stephentoub force-pushed the feature/ISetAsReadOnlyExtension branch from ed8bfb1 to b7d1e0e Compare November 25, 2024 16:47
@stephentoub
Copy link
Member

/ba-g failures are unrelated

@stephentoub stephentoub merged commit ad1f8db into dotnet:main Nov 25, 2024
134 of 138 checks passed
@stephentoub
Copy link
Member

Thanks

@Mrxx99
Copy link
Contributor Author

Mrxx99 commented Nov 25, 2024

@stephentoub Thanks for finishing the PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Collections community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add an ISet<T>.AsReadOnly() extension method
4 participants