Skip to content

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

@ghost
Copy link

ghost commented Aug 6, 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.

1 similar comment
@ghost
Copy link

ghost commented Aug 6, 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.

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

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

mikelle-rogers pushed a commit to mikelle-rogers/runtime that referenced this pull request Dec 10, 2024
* Add AsReadOnly extension for ISet<T>

* Fix type forwarding

---------

Co-authored-by: Stephen Toub <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Dec 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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