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.

Sign up for GitHub

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 implicit operator ROS<string?> for StringValues #101457

Closed
wants to merge 1 commit into from
Closed

Add implicit operator ROS<string?> for StringValues #101457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/libraries/Microsoft.Extensions.Primitives/ref/Microsoft.Extensions.Primitives.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,11 @@ public void Reset() { }
public static bool operator ==(string?[]? left, Microsoft.Extensions.Primitives.StringValues right) { throw null; }
public static implicit operator string?(Microsoft.Extensions.Primitives.StringValues values) { throw null; }
public static implicit operator string?[]?(Microsoft.Extensions.Primitives.StringValues value) { throw null; }
#if NET9_0_OR_GREATER
#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
Copy link
Member

Choose a reason for hiding this comment

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

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

public static implicit operator Microsoft.Extensions.Primitives.StringValues(string? value) { throw null; }
public static implicit operator Microsoft.Extensions.Primitives.StringValues(string?[]? values) { throw null; }
public static bool operator !=(Microsoft.Extensions.Primitives.StringValues left, Microsoft.Extensions.Primitives.StringValues right) { throw null; }
Expand Down
20 changes: 20 additions & 0 deletions src/libraries/Microsoft.Extensions.Primitives/src/StringValues.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,26 @@ public static implicit operator StringValues(string?[]? values)
return value.GetArrayValue();
}

#if NET9_0_OR_GREATER
Copy link
Member Author

Choose a reason for hiding this comment

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

@dotnet/area-extensions-primitives please advice if you want to expose this to ns2.0 too, it can't be allocation-free there.

/// <summary>
/// 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.
Copy link
Member Author

Choose a reason for hiding this comment

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

This looks like an analyzer bug, see this sharplab sample too.

public static implicit operator ReadOnlySpan<string?>(in StringValues value)
#pragma warning restore CS3006
{
if (value._values is string)
{
return new ReadOnlySpan<string?>(in Unsafe.As<object, string?>(ref Unsafe.AsRef(in value._values)));
Copy link
Member

Choose a reason for hiding this comment

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

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");
}

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@jozkee jozkee Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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]); 
}

Copy link
Member

@stephentoub stephentoub Apr 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}
else
{
return new ReadOnlySpan<string?>((string?[]?)value._values);
}
}
#endif

/// <summary>
/// Gets the number of <see cref="string"/> elements contained in this <see cref="StringValues" />.
/// </summary>
Expand Down
37 changes: 37 additions & 0 deletions src/libraries/Microsoft.Extensions.Primitives/tests/StringValuesTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,43 @@ public void ImplicitStringArrayConverter_Works()
Assert.Equal<string[]>(aStringArray, stringValues);
}

#if NETCOREAPP
[Fact]
public void ImplicitReadOnlySpanStringConverter_Works()
{
// from null string
StringValues stringValues = (string)null;
ReadOnlySpan<string> span = (ReadOnlySpan<string>)stringValues;
Assert.True(span.IsEmpty);
Assert.Equal((string[])stringValues, span);

// from null string[]
stringValues = (string[])null;
span = (ReadOnlySpan<string>)stringValues;
Assert.True(span.IsEmpty);
Assert.Equal((string[])stringValues, span);

// from string
string aString = "abc";
stringValues = (string)aString;
span = (ReadOnlySpan<string>)stringValues;
Assert.Equal(1, span.Length);
Assert.Equal(aString, span[0]);
Assert.Equal((string[])stringValues, span);

// from string[]
string bString = "bcd";
string[] aStringArray = new[] { aString, bString };
stringValues = (string[])aStringArray;
span = (ReadOnlySpan<string>)stringValues;
Assert.Equal(2, span.Length);
Assert.Equal(aString, span[0]);
Assert.Equal(bString, span[1]);
Assert.Equal(aStringArray, span);
Assert.Equal((string[])stringValues, span);
}
#endif

[Theory]
[MemberData(nameof(DefaultOrNullStringValues))]
[MemberData(nameof(EmptyStringValues))]
Expand Down
Loading