"..."u8 prefers byte[] when it should prefer ReadOnlySpan<byte>

[stephentoub]

Version Used:
e79be97

Steps to Reproduce:
I understand this is currently "by design", but we should revisit the design. That u8's natural type is byte[] leads to a variety of problems.

For example, this:

"abcd"u8.Length

allocates a new byte[4] just to get the length; if this were instead a span, that allocation would be avoided, and even if C# didn't want to recognize this particular pattern of constString.Length, the JIT can:
https://sharplab.io/#v2:EYLgtghglgdgNAFxBAzmAPgAQEwEYCwAUEZgMwAEO5AwkQN5HlOUWwLkCyAFAJTkC8APnIwApgHdyAJVEQAJgHkYAGwCeAZQAOEGAB5gqhKMFcxkg0YDaAXXJ1yXC6J4ByCC7gOnr4B6+HnFwBjP0cA1zkXcgBfHgA6ABlRGABzBAALAG4iaKA==
It's not uncommon to use const strings for their length like this rather than hardcoding a magic constant by manually counting the bytes/characters, e.g.

ReadOnlySpan<byte> data = ...;
if (data.Slice("prefix"u8.Length).SequenceEquals(...)) { ... }

Requiring a cast to ReadOnlySpan<byte> here to do the "right thing" is unexpected and unintuitive.

Same goes for other operations on span, like CopyTo. You want to be able to use UTF8 const strings to populate things, e.g.

byte[] data = ...;
"suffix"u8.CopyTo(data.Slice(123));

but you really don't want to be allocating a byte[] for "suffix" to achieve that.

It's also unfortunate that overload resolution picks byte[] over ReadOnlySpan<byte>:

using System;
public class C
{
    public void M() => N("abcd"u8);
    public void N(ReadOnlySpan<byte> bytes){}
    public void N(byte[] bytes){}
}

https://sharplab.io/#v2:EYLgtghglgdgPgAQEwEYCwAoBBmABM3AYUwG9NcL88EAWXAWQAoBKXAXgD5cA5RgIgjAAxgBM+AVwAczANzlKOfHV4AlAKYQRAeRgAbAJ4BlAA4QYAHmD6ALmq5XbAZ2YkAvvIqLaPRg7UBtAF1cP2c3TFcgA===
It's common to add span-based overloads to things that currently accept byte[], and even to add both overloads at the same time (in support of languages that don't support spans), and having overload resolution prefer the more expensive thing is problematic.

cc: @MadsTorgersen

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

[tannergooding]

Many similar points were raised in dotnet/csharplang#5983

[stephentoub]

Yes. Though unlike some of those comments, I do believe that allowing "..."u8 to target-type to byte[] or anything implicitly creatable from byte[] (e.g. ReadOnlyMemory<byte>) is valuable and something we should strive to keep. I just don't think it should be the natural type.

[tannergooding]

Yep, just wanted to link the related bits even if some other bits have differing opinions.

-- Notably, I'm not a huge fan of the implicit assignment to byte[] due to the hidden allocations/etc, I'd rather just .ToArray() myself. That being said, its not a "strong" dislike and I could understand allowing it if string likewise got support for char[] and ReadOnlyMemory<char>, etc. Having something new here and not providing similar functionality for string seems odd/inconsistent to me. But that'd be a separate issue.

[stephentoub]

I'd sent an email which was discussed in C# LDM today. Including it here for @MadsTorgersen to link to:

---

[...]
Here’s our recommendation for some tweaks to be made to the design:

1. The u8 suffix should be required. Allowing “…” to implicitly convert to byte-based types leads to ambiguities in overload resolution and raises questions about the encoding being employed to generate bytes.

2. ReadOnlySpan<byte> should be the natural type of “…”u8. Having the natural type be byte[] leads to very unexpected allocations, e.g. “prefix”u8.CopyTo(span) allocating, “data”u8.Length allocating, var prefix = “prefix”u8 allocating, etc. It also ends up allocating when choosing between two overloads, one based on byte[] and one based on ReadOnlySpan<byte>, and having both overloads for both types is becoming increasingly more common. And it causes problems for null termination…

3. UTF8 literals should be null terminated. For consistency with string literals, as a defense-in-depth measure, and to help with some specific interop use cases, we should ensure there’s a null byte just after the u8 data but not included in the span’s length. This is primarily valuable for niche cases where code does fixed (byte* ptr = “somestring”u8) Call(ptr);, invoking a native function that expects the UTF8 data to be null terminated. Some feel that since string literals are null terminated and can be used in a similar manner, folks doing such interop will expect this to “just work”, and we’ll be risking vulnerabilities if we don’t. We should also investigate adding an analyzer to help mitigate expectations that such a terminator might apply to any ReadOnlySpan<byte> (or ReadOnlySpan<char> for that matter).

4. Attributes should support using “…”u8 where byte[]s are expected. This is more of a nice-to-have than something that’s truly required. This is common in certain kinds of testing scenarios, for example. But we also might not want to do this if we don’t do (5) below…

We are in disagreement about one aspect:

5. Should “…”u8 target type byte[] and things implicitly convertible from byte[]? One the one hand, it makes code that needs a byte[], a ReadOnlyMemory<byte>, an ImmutableArray<byte>, etc. more concise and simpler. On the other hand, it might be surprising to folks that each such use allocates (most such uses wouldn’t be “hidden”, but some could be if there were a method that only accepted a byte[] and you called it with “…”u8 as an argument), and there’s an inconsistency around “…” not implicitly converting to char[] (but that just stems from the broader, accepted inconsistency of there being a heap-based type for UTF16 but not currently for UTF8). This should not have a significant impact on APIs we expose, as we do expose and plan to expose ReadOnlySpan<byte>-based overloads anywhere we expect performance to be an issue. It’s more of a language usability vs predictability decision, so a good discussion to have in LDM. It of course could be a moot decision as well if it’s not feasible or desirable once the natural type is ReadOnlySpan<byte> (which is the more important piece). If we don’t enable it, instead of byte[] bytes = “data”u8;, you’d write byte[] bytes = “data”u8.ToArray();, taking advantage of the ToArray method ReadOnlySpan<byte> already has.

[...]

[BrennanConroy]

We just updated the SDK in ASP.NET Core and are hitting two classes of issues. I think both are variations that would be resolved by the first item mentioned above.

string foo = "...";
Method(foo); // now ambiguous, resulting in compile error
void Method(ReadOnlySpan<char>);
void Method(ReadOnlySpan<byte>);

With a Protect method that accepts a byte[] and an extension method that accepts a string with the invocation site of:
protector.Protect("Hello\ud800");
We now get the error:

error CS9026: The input string cannot be converted into the equivalent UTF8 byte representation. Unable to translate Unicode character \uD800 at index 5 to specified code page.

Using a similar API but with a valid utf8 convertible string we now call the byte[] overload which is breaking as we assume the string is base64 encoded and decode it first, so now we get a byte[] that's actually a base64 string and assume it's already base64 decoded.