Implicit conversation from string to params ROS<char> should be disallowed

[hez2010]

We have some APIs like string.Trim which originally receives a parameter params char[]. To make them efficient, we are introducing params ROS<char> overload to such APIs.

But it immediately runs into an issue where now strings are being implicitly converted to ROS<char> which makes the API extremely confusing.

"ab123ba".Trim('a', 'b') will go with the overload taking a params ROS<char> and return 123.
But with strings being converted to ROS<char> implicitly, "ab123ba".Trim("ab") will still go with the one taking a params ROS<char>, but developers would expect it returning 123ba instead of 123.

I think we should disallow implicit conversion from string to params ROS<char>, and maybe disallow u8 to params ROS<byte> as well.

See the context: dotnet/runtime#102822

/cc: @stephentoub

[stephentoub]

I think we should disallow implicit conversion from string to params ROS, and maybe disallow u8 to params ROS as well.

The string syntax is a very concise way to express a list of characters, e.g.

" \r\n\t"

instead of:

[' ', '\r', '\n', '\t']

or with params:

' ', '\r', '\n', '\t'

and as a result folks use it in that way, including us. For example, here's a use with SearchValues.Create. This overload takes a ReadOnlySpan<char> (and in .NET 9 that argument has been made params):
https://github.com/dotnet/runtime/blob/9fca0c3dbd3874ed0245b1bdb10547d0ba769d66/src/libraries/System.Private.Uri/src/System/DomainNameHelper.cs#L36-L37

private static readonly SearchValues<char> s_validChars =
    SearchValues.Create("-0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz.");

It becomes a stylistic choice whether you prefer that syntax, or if we banned the cited conversion:

private static readonly SearchValues<char> s_validChars = SearchValues.Create(
    '-', '0', '1', '2', '3', '4', '5', '6', '7', '8', '9',
    'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', 'N', 'O', 'P', 'Q', 'R', 'S', 'T', 'U', 'V', 'W', 'X', 'Y', 'Z',
    '_',
    'a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j', 'k', 'l', 'm', 'n', 'o', 'p', 'q', 'r', 's', 't', 'u', 'v', 'w', 'x', 'y', 'z',
    '.');

If we banned the use of string, either we'd be taking a source breaking change, where every use of such an API would no longer compile, or we wouldn't be able to add params to this overload, effectively dictating that developers can't use the individual character syntax if they prefer that unless they use collection expressions.

I understand and can appreciate the goal of the suggestion, but I don't think it's desirable.

[hez2010]

If the overload is simply taking a ROS<char>, it's okay to use string to be the concise way of creating a list of chars. But for params ROS<char>, I would instead be wanting to let users know they are passing a list of chars explicitly, otherwise it will lead to huge confusion.

BTW this is also blocking us adding overloads for Trim(string) in the future which trims a string segment instead of array of chars from a string.

It becomes a stylistic choice whether you prefer that syntax, or if we banned the cited conversion:

You can simply write SearchValues.Create("-0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz.".AsSpan()); instead.

[jnm2]

The methods could be called TrimSequence(string) or something like that.

[stephentoub]

If the overload is simply taking a ROS, it's okay to use string to be the concise way of creating a list of chars. But for params ROS, I would instead be wanting to let users know they are passing a list of chars explicitly.

My SearchValues.Create example is a params ReadOnlySpan<char>.

BTW this is also blocking us adding overloads for Trim(string) in the future.

That's the case regardless of params. Trim(ReadOnlySpan<char>) is being called like this:
https://github.com/dotnet/runtime/blob/fd4da9aaa16261d2b8d7104be0b8c77db991dff0/src/libraries/System.Private.Xml.Linq/src/System/Xml/Linq/XNodeReader.cs#L431

switch (a.Value.AsSpan().Trim(" \t\n\r"))

Let's say that wasn't params ReadOnlySpan<char> and just ReadOnlySpan<char>. It would still be a breaking change to add a Trim(string) overload where the string is interpreted to be a single unit, as that call site would then be reinterpreted to mean something completely differently than it currently does.

[jaredpar]

Taking this suggestion would mean that application of params is a breaking change. Essentially taking an API that has a single ReadOnlySpan<char> parameter, then adding params would be a source breaking change.

[CyrusNajmabadi]

Yeah, this seems like a non-starter on that aspect alone.

[TahirAhmadov]

The string syntax is a very concise way to express a list of characters, e.g.

I disagree with this. When I see a string like " \r\n\t", I immediately think that something will happen with that string as a whole, not with its individual characters. If there is indeed a long list of characters, why not just do "-0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZ_abcdefghijklmnopqrstuvwxyz.".AsSpan()? Then we get the "compressed" size of not having to type all those single quotes and an indication that we're actually operating with the characters, not the string.

I understand the breaking change aspect, though.

[stephentoub]

and an indication that we're actually operating with the characters, not the string

I don't see how that's an indication. Some APIs take a ReadOnlySpan<char> as a string-like thing, and some APIs take a ReadOnlySpan<char> as a list of characters.

[TahirAhmadov]

Yeah that's a little confusing. Add this to the list of things to clean up when we re-create .NET and C# from scratch 🤣

[TahirAhmadov]

I think when it's params ROS<char>, it can be a good way to "communicate" that this API deals with characters, as opposed to when it's ROS<char> without params, which means it's string-like. Which is why the OP kind of makes sense to me. (Breaking change questions aside.)