-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Add CollectionBuilder attribute to some immutable collection interfaces #89459
Conversation
Note regarding the 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. |
Tagging subscribers to this area: @dotnet/area-system-collections Issue DetailsRoslyn asked that these be added. Adding these attributes to the immutable interfaces was discussed as part of #87569 (comment) and left up to the working group to decide what to do with it, which is this PR.
|
@cston @RikkiGibson @captainsafia . We shoudl have a meeting to conclude on this. If we do take this path, we will need to ensure the lang spec/impl allows for this. Specifically, where the factory method returns a different (but implicit-reference compatible) result. |
Totally. I don't know whether/how we should limit the permitted implicit conversions. For example, if the implementation type is a struct, is it ok to do a boxing conversion here? The user is applying the attribute. We might assume they know which method is being referenced in the attribute and they're OK with the semantics of converting the result to interface type. |
My presumptino is that at least an implicit conversion needs to exist.
In practice, i expect usage to be exactly like this. There is a concrete type returned and it has an implicit conversion (ideally reference, but any implicit seems fine) to the interface. I think boxing is fine as i could totally see a lightweight struct being an impl for some types, and the interface just going: yup, good enough for me. |
src/libraries/System.Collections.Immutable/ref/System.Collections.Immutable.cs
Show resolved
Hide resolved
src/libraries/System.Collections.Immutable/src/System/Collections/Immutable/IImmutableStack.cs
Show resolved
Hide resolved
C# 12 is going to support the conversion. |
Roslyn asked that these be added. Adding these attributes to the immutable interfaces was discussed as part of #87569 (comment) and left up to the working group to decide what to do with it, which is this PR.