-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Add [OverloadResolutionPriority(1)] to MemoryMarshal.AsRef(Span<byte>) #122330
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
base: main
Are you sure you want to change the base?
Conversation
…te>) Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
|
@copilot, please add a test based on the original issue. It should fail to compile prior to the fix and compile successfully after. Also, please add a succinct comment next to the ORPA attribute in the source indicating what it's there for / what problem it's solving. |
…AsRef Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Added in 46b7492:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds the [OverloadResolutionPriority(1)] attribute to the MemoryMarshal.AsRef<T>(Span<byte>) method to prioritize it over the AsRef<T>(ReadOnlySpan<byte>) overload during overload resolution. This resolves a C# 14 regression where types with implicit Span<byte> conversions would incorrectly resolve to the ReadOnlySpan<byte> overload, preventing mutable references from being obtained.
- Added
[OverloadResolutionPriority(1)]attribute to theSpan<byte>overload in both implementation and reference assemblies - Added explanatory comments documenting the purpose of the attribute
- Added comprehensive test validating that types with implicit
Span<byte>conversion correctly obtain mutable references
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/MemoryMarshal.cs | Added [OverloadResolutionPriority(1)] attribute with explanatory comment to the AsRef<T>(Span<byte>) method |
| src/libraries/System.Runtime/ref/System.Runtime.cs | Added [OverloadResolutionPriorityAttribute(1)] to the AsRef<T>(Span<byte>) method signature in the reference assembly |
| src/libraries/System.Memory/tests/MemoryMarshal/AsRef.cs | Added test validating that types with implicit Span<byte> conversion return mutable references through AsRef |
333fred
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While in general we wanted ROS prioritized because of array covariance causing potential breaks, we're dealing with bytes here that's not an issue. Therefore, it seems fine to me.
Thanks. Not bytes necessarily, but the T is constrained to be a value type, so same argument applies. |
[OverloadResolutionPriority(1)]attribute toMemoryMarshal.AsRef<T>(Span<byte> span)in the implementation file (src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/MemoryMarshal.cs)[System.Runtime.CompilerServices.OverloadResolutionPriorityAttribute(1)]to the same method in the reference assembly file (src/libraries/System.Runtime/ref/System.Runtime.cs)Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.