Add implicit operator ROS<string?> for StringValues #101457
Conversation
|
Note regarding the |
|
Tagging subscribers to this area: @dotnet/area-extensions-primitives |
| @@ -85,6 +85,26 @@ public StringValues(string?[]? values) | |||
| return value.GetArrayValue(); | |||
| } | |||
|
|
|||
| #if NET9_0_OR_GREATER | |||
There was a problem hiding this comment.
@dotnet/area-extensions-primitives please advice if you want to expose this to ns2.0 too, it can't be allocation-free there.
| /// Defines an implicit conversion of a given <see cref="StringValues"/> to a string span. | ||
| /// </summary> | ||
| /// <param name="value">A <see cref="StringValues"/> to implicitly convert.</param> | ||
| #pragma warning disable CS3006 // Overloaded method 'StringValues.implicit operator ReadOnlySpan<string?>(in StringValues)' differing only in ref or out, or in array rank, is not CLS-compliant. |
There was a problem hiding this comment.
This looks like an analyzer bug, see this sharplab sample too.
| #pragma warning disable CS3006 // Overloaded method 'StringValues.implicit operator ReadOnlySpan<string?>(in StringValues)' differing only in ref or out, or in array rank, is not CLS-compliant. | ||
| public static implicit operator System.ReadOnlySpan<string?> (in Microsoft.Extensions.Primitives.StringValues value) { throw null; } | ||
| #pragma warning restore CS3006 | ||
| #endif |
There was a problem hiding this comment.
We generally don't use ifdefs in ref files. You'll want to create a separate Microsoft.Extensions.Primitives.netcoreapp.cs next to this one with just the new API, e.g. https://github.com/dotnet/runtime/blob/main/src/libraries/System.IO.Hashing/ref/System.IO.Hashing.netcoreapp.cs
| { | ||
| if (value._values is string) | ||
| { | ||
| return new ReadOnlySpan<string?>(in Unsafe.As<object, string?>(ref Unsafe.AsRef(in value._values))); |
There was a problem hiding this comment.
Isn't this extremely unsafe?
Other threads (or even the same thread) could overwrite the reference.
The rest of StringValues is careful to always do Unsafe only on locals that it already type checked.
_field = new("foo");
ReadOnlySpan<string> span = _field;
_field = new(["bar"]);
if (span[0].GetType() != typeof(string))
{
Console.WriteLine("Oh no");
}There was a problem hiding this comment.
That's a very good catch.
And I don't see a good way to achieve what the method needs to achieve and still be able to make it safe.
I think that means we need to scrap this API.
There was a problem hiding this comment.
we need to scrap this API.
@stephentoub don't we want change to the allocating form?
if (value._values is string s)
{
return new ReadOnlySpan<string?>([s]);
}There was a problem hiding this comment.
we need to scrap this API.
@stephentoub don't we want change to the allocation form?
I'd be ok with that in this case, but there were several folks who voiced opposition to having a span-returning API like this that allocated.
stephentoub
added
the
NO-MERGE
Contributes to #101261.