Create APIs to deal with processing ASCII text (as bytes)

@KrzysztofCwalina

EDIT: most up to date proposal can be found here: #28230 (comment)

Most recent API review comment: #28230 (comment)

If you are interested in the original proposal, click "details" below:

Details

Networking protocols and parsers in particular use ASCII very frequently - even more so than UTF-8. It would be useful to introduce high-performance APIs that work very specifically on ASCII text without trying to interpret it as UTF-8.

using System;
using System.Buffers;

/*
 * This work item proposes a new static class for performing operations
 * on byte buffers assumed to contain ASCII text. For the purposes of
 * these APIs, an ASCII byte / char is any value in the range [ 00 .. 7F ], inclusive;
 * and ASCII text is a sequence of such values. Most APIs in this type will
 * treat non-ASCII values as opaque data unless the API description specifies
 * a different behavior.
 *
 * For APIs which take both a source and a destination buffer, the behavior of
 * the method is undefined if the source and destination buffers overlap,
 * unless the API description specifies otherwise. The behavior of all APIs
 * is undefined if another thread mutates the buffers while these APIs are
 * operating on them.
 *
 * All case conversion APIs are culture-unaware.
 */

namespace System.Buffers.Text
{
    public static class Ascii
    {
        // Compares two ASCII buffers for equality, optionally treating [A-Z] and [a-z] as equal.
        // All non-ASCII bytes / chars are compared for pure binary equivalence.

        public static bool Equals(ReadOnlySpan<byte> left, ReadOnlySpan<byte> right);
        public static bool Equals(ReadOnlySpan<char> left, ReadOnlySpan<char> right);
        public static bool EqualsIgnoreCase(ReadOnlySpan<byte> left, ReadOnlySpan<byte> right);
        public static bool EqualsIgnoreCase(ReadOnlySpan<char> left, ReadOnlySpan<char> right);

        // Compares an ASCII byte buffer and an ASCII char buffer for equality, optionally treating
        // [A-Z] and [a-z] as equal. Returns false if the ASCII byte buffer contains any non-ASCII
        // data or if the char buffer contains any element in the range [ 0080 .. FFFF ], as we
        // wouldn't know what encoding to use to perform the transcode-then-compare operation.

        public static bool Equals(ReadOnlySpan<byte> left, ReadOnlySpan<char> right);
        public static bool Equals(ReadOnlySpan<byte> left, string right);
        public static bool EqualsIgnoreCase(ReadOnlySpan<byte> left, ReadOnlySpan<char> right);
        public static bool EqualsIgnoreCase(ReadOnlySpan<byte> left, string right);

        // Searches for the first occurrence of the target substring within the search space,
        // optionally treating [A-Z] and [a-z] as equal. All non-ASCII bytes are compared for pure
        // binary equivalence. Returns the index of where the first match is found, else returns -1.
        // ADDENDUM: Also assume there are *Last equivalents of the following.

        public static int IndexOf(ReadOnlySpan<byte> text, ReadOnlySpan<byte> value);
        public static int IndexOfIgnoreCase(ReadOnlySpan<byte> text, ReadOnlySpan<byte> value);

        // Searches for the first occurrence of the target substring within the search space,
        // optionally treating [A-Z] and [a-z] as equal. Returns the index of where the first match
        // is found, else returns -1. If the target string contains any non-ASCII chars ([ 0080 .. FFFF ]),
        // the search is assume to have failed, and the method returns -1.
        // ADDENDUM: Also assume there are *Last equivalents of the following.

        public static int IndexOf(ReadOnlySpan<byte> text, ReadOnlySpan<char> value);
        public static int IndexOf(ReadOnlySpan<byte> text, string value);
        public static int IndexOfIgnoreCase(ReadOnlySpan<byte> text, ReadOnlySpan<char> value);
        public static int IndexOfIgnoreCase(ReadOnlySpan<byte> text, string value);

        // Given a buffer, returns the index of the first element in the buffer which
        // is a non-ASCII byte, or -1 if the buffer is empty or all-ASCII. The bool-
        // returning method is a convenience shortcut to perform the same check.

        public static int GetIndexOfFirstNonAsciiByte(ReadOnlySpan<byte> buffer);
        public static int GetIndexOfFirstNonAsciiChar(ReadOnlySpan<char> buffer);
        public static bool IsAllAscii(ReadOnlySpan<byte> buffer);
        public static bool IsAllAscii(ReadOnlySpan<char> buffer);

        // Returns true iff the provided byte is an ASCII byte; i.e., in the range [ 00 .. 7F ];
        // or if the provided char is in the range [ 0000 .. 007F ].

        public static bool IsAsciiByte(byte value);
        public static bool IsAsciiChar(char value);

        // Copies source to destination, converting [A-Z] -> [a-z] or vice versa during
        // the copy. All values outside [A-Za-z] - including non-ASCII values - are unchanged
        // during the copy.
        //
        // If source.Length <= destination.Length, succeeds and returns source.Length (# bytes copied).
        // If source.Length > destination.Length, returns -1.

        public static int ToLowerInvariant(ReadOnlySpan<byte> source, Span<byte> destination);
        public static int ToLowerInvariant(ReadOnlySpan<char> source, Span<char> destination);
        public static int ToUpperInvariant(ReadOnlySpan<byte> source, Span<byte> destination);
        public static int ToUpperInvariant(ReadOnlySpan<char> source, Span<char> destination);

        // Performs case conversion ([A-Z] -> [a-z] or vice versa) in-place. All values
        // outside [A-Za-z] - including non-ASCII values - are unchanged.

        public static void ToLowerInvariantInPlace(Span<byte> buffer);
        public static void ToLowerInvariantInPlace(Span<char> buffer);
        public static void ToUpperInvariantInPlace(Span<byte> buffer);
        public static void ToUpperInvariantInPlace(Span<char> buffer);

        // Performs case conversion on a single value, converting [A-Z] -> [a-z] or vice versa.
        // All values outside [A-Za-z] - including non-ASCII values - are unchanged.

        public static byte ToLowerInvariant(byte value);
        public static byte ToLowerInvariant(char value);
        public static byte ToUpperInvariant(byte value);
        public static byte ToUpperInvariant(char value);

        // Returns a hash code for the provided buffer suitable for use in a dictionary or
        // other keyed collection. For the OrdinalIgnoreCase method, the values [A-Z] and [a-z]
        // are treated as equivalent during hash code computation. All non-ASCII values
        // are treated as opaque data. The hash code is randomized but is not guaranteed to
        // implement any particular algorithm, nor is it guaranteed to be a member of the same
        // PRF family as other GetHashCode routines in the framework.

        public static int GetHashCode(ReadOnlySpan<byte> buffer);
        public static int GetHashCode(ReadOnlySpan<char> buffer);
        public static int GetHashCodeOrdinalIgnoreCase(ReadOnlySpan<byte> buffer);
        public static int GetHashCodeOrdinalIgnoreCase(ReadOnlySpan<char> buffer);

        // Widens an ASCII buffer to UTF-16 or narrows a UTF-16 buffer to ASCII.
        // Returns OperationStatus.InvalidData if the source buffer contains a non-ASCII byte
        // or a char in the range [ 0080 .. FFFF ].
        // OPEN QUESTION: Should we have an equivalent with Latin-1 semantics? Probably doesn't
        // belong on the ASCII class if we do that.

        public static OperationStatus WidenToUtf16(ReadOnlySpan<byte> source, Span<char> destination, out int bytesRead, out int charsWritten);
        public static OperationStatus NarrowFromUTf16(ReadOnlySpan<char> source, Span<byte> destination, out int charsRead, out int bytesWritten);

        // Widens the provided buffer to UTF-16, then returns a new string instance from it.
        // Throws ArgumentException if the buffer contains non-ASCII data. If the desired behavior
        // is character substitution instead of throwing an exception, consider instead using
        // Encoding.ASCII.GetString.        

        public static string ToString(ReadOnlySpan<byte> buffer);

        // Trims only ASCII whitespace values from the buffer, returning the trimmed buffer.

        public static ReadOnlySpan<byte> Trim(ReadOnlySpan<byte> buffer);
        public static ReadOnlySpan<char> Trim(ReadOnlySpan<char> buffer);
        public static ReadOnlySpan<byte> TrimStart(ReadOnlySpan<byte> buffer);
        public static ReadOnlySpan<char> TrimStart(ReadOnlySpan<char> buffer);
        public static ReadOnlySpan<byte> TrimEnd(ReadOnlySpan<byte> buffer);
        public static ReadOnlySpan<char> TrimEnd(ReadOnlySpan<char> buffer);

        // Similar to Trim, but returns the Range of the untrimmed data. Useful for
        // scenarios where the resulting ROS<byte> needs to be mapped back to some
        // container data structure.
        // Example: Assume ROM<byte> mem = GetSomeAsciiData();
        // Then ROM<byte> trimmed = mem[Ascii.GetTrimmedRange(mem.Span)];

        public static Range GetTrimmedRange(ReadOnlySpan<byte> buffer);
        public static Range GetTrimmedRange(ReadOnlySpan<char> buffer);
    }
}

Why is it useful to return source length?

@danmosemsft It's not useful. It's just to match the API shape of MemoryExtensions.ToUpperInvariant, which also unnecessarily returns the source length.

I suggest not to perpetuate the design flaw?

In-place APIs - do we need these?

As mentioned that's easy to achieve, but it would be convenient to have these (as common use case?) in corefx.

@danmosemsft - Sure, no complaints from my end to make it void-returning. We can simply document the behavior.

would it be better to have these helper methods on the ASCIIEncoding class instead of creating a new utility class?

@tarekgh

@tarekgh I had this same concern with the UTF-8 APIs. Why not put them on the existing UTF8Encoding class rather than a separate static class?

The argument that pushed me away from putting them on the existing Encoding classes is that the proposed static APIs are very opinionated in how they behave. If you subclass AsciiEncoding, UTF8Encoding, etc., these static APIs would still show up, and their behavior might differ from that of the overridden instance methods on the type. This is not a pit of success.

I didn't see anyone sub-classing Ascii encoding at all. this mean is unlikely to happen. even it show up in the list when sub-classing, I don't think this will be issue as these static methods anyway. my whole point having these in the Ascii encoding for easier discoverability as one class encapsulating the Ascii operations.

You don't have to block on that though :-) and we should proceed with the design review.

What is the behavior of this API if source and destination partially overlap?

@vcsjones

@vcsjones I assume undefined. See also https://github.com/dotnet/corefx/issues/34799, where I contemplated this same question with the existing span-based text case conversion APIs.

I suggest not to perpetuate the design flaw?

It was done intentionally.

Sure, no complaints from my end to make it void-returning. We can simply document the behavior.

I would keep it consistent and return an int.

Here is the context for why we returned an int: https://github.com/dotnet/corefx/issues/31340#issuecomment-407866739

The API initially returned void but was changed, the reasoning for which can be seen in this GitHub thread.

Essentially, if the presupposition that the ToUpper/ToLower APIs do not change the length of the data was removed, we would have to change the public signature of the API. Having the API return the amount of data written avoids API change in the future.

All four functions always return source.length in case of success. Do you think it can be something different on other platforms?

Hence, this is an implementation detail and not guaranteed, necessarily.

Yes, exceptions are expensive, but this is error prone design, because callers will forget they must check the return value.

That is why the API was designed with a return value. The contract suggests you should do something with the return value (in an attempt to avoid scenario 1). A void returning method gives even less indication that the caller needs to do anything and as you mentioned, exceptions are expensive (especially if we are catching the exception to resize within a loop). The span-resizing pattern is a common practice with a lot of our span-based APIs and hence we haven't used exceptions for length mismatch anywhere.

Hopefully that adds some context to the design decision.

@GrabYourPitchforks

@GrabYourPitchforks regarding undefined behavior: It seems better to detect this and throw or to make it defined (for example, by using a slower but safe algorithm).

Essentially, if the presupposition that the ToUpper/ToLower APIs do not change the length of the data was removed, we would have to change the public signature of the API. Having the API return the amount of data written avoids API change in the future.

I don't think this is ever going to happen realistically. String manipulation APIs like ToUpper / ToLower in the Framework have always used simple case folding semantics, and simple case folding semantics are guaranteed per the Unicode Standard never to change the length of a UTF-16 string. The only public APIs which use complex case folding semantics are String comparison APIs (e.g., Equals(..., StringComparison.CurrentCulture)) and CompareInfo.GetSortKey.

An enormous amount of code is written with the assumption that simple case folding semantics are used for all String manipulation APIs, and it would introduce a massive breaking change throughout our ecosystem if we were to suddenly upend this. If we were going to introduce an API to perform full folding (which could change the length of the UTF-16 data), it really should be on a brand new API rather than one that purports to have the same semantics as existing APIs. Something like TextInfo.ToUpperFullFolding(...) would fit the bill if we can think of a good name.

See also https://github.com/dotnet/coreclr/issues/20955#issuecomment-438389558.

Edit: I'd still prefer returning an int just for consistency at this point. And any ToUpper(ROS<char8>) overloads would absolutely need it since the length of UTF-8 data can change, even with simple folding semantics. My intent with this comment was that the original reasoning for making the ToUpper(ROS<char>) overloads return an int appears to have been flawed.

@GrabYourPitchforks

@GrabYourPitchforks are we still targeting this for 3.0?

@GrabYourPitchforks

@GrabYourPitchforks @stephentoub while studying ASP.NET code today I've realized that their corresponding helper for ToUtf16 method treats NULL character (0) as an invalid character:

https://github.com/dotnet/aspnetcore/blob/792e021af928d435276ffdb2149082ea3d8ce9c5/src/Servers/Kestrel/Core/src/Internal/Infrastructure/HttpUtilities.cs#L96-L97

The check if performed here: https://github.com/dotnet/aspnetcore/blob/c4b57c452447f70ab874097436226542c70d74c0/src/Shared/ServerInfrastructure/StringUtilities.cs#L799

So what we have right now in the proposal won't meet their requirements. What is your opinion on extending ToUtf16 and FromUtf16 with a boolean parameter that describes how to treat NULL characters?

-    public static OperationStatus ToUtf16(ReadOnlySpan<byte> source, Span<char> destination, out int elementsProcessed);
+    public static OperationStatus ToUtf16(ReadOnlySpan<byte> source, Span<char> destination, bool allowNullCharacters = true, out int elementsProcessed);
-    public static OperationStatus FromUtf16(ReadOnlySpan<char> source, Span<byte> destination, out int elementsProcessed);
+    public static OperationStatus FromUtf16(ReadOnlySpan<char> source, Span<byte> destination, bool allowNullCharacters = true, out int elementsProcessed);

I just want to make sure that they can delete their own copy of ASCIIUtility when we introduce the new Ascii APIs.

@adamsitnik

@adamsitnik in case you missed it: dotnet/aspnetcore#44040 (comment)

I strongly oppose special-casing NULL (or any other control character, including 0x7F) in the general purpose text processing APIs. These are all perfectly valid ASCII characters. It's fine for protocols to impose their own restrictions above and beyond mere ASCII: some protocols disallow NUL, some protocols disallow CRLF, some protocols disallow VT, some protocols disallow 0x7F, and so on. But general-purpose building block APIs should normally remain agnostic over all of these, leaving the vagaries of protocol details up to whatever higher level component is build atop this. We don't special-case NULL or any other control character anywhere else within the System.Text namespace, for instance.

For the particular API being suggested here, we don't special-case control chars within System.Text.ASCII.GetString(byte[]). Similarly, we should not special-case it here. The behaviors of the two APIs should be perfectly identical when given valid ASCII as input.

For the case of an OperationStatus API where Consumed is guaranteed equal to Written we decided to go with omitting Consumed (and documenting the behavior), leaving the naming up to the destination type (for bytesWritten/charsWritten/elementsWritten).
We discussed, and reiterated, that "Try" is not appropriate if a false return would out a non-default value, so went with OperationStatus everywhere.
We discussed, and rejected, adding replacement semantics to the UTF-16->ASCII transformations.

namespace System.Text;

public static class Ascii
{   
    public static bool IsValid(ReadOnlySpan<byte> value);
    public static bool IsValid(ReadOnlySpan<char> value);

    public static bool IsValid(byte value);
    public static bool IsValid(char value);

    public static OperationStatus ToUpper(ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten);
    public static OperationStatus ToUpper(ReadOnlySpan<char> source, Span<char> destination, out int charsWritten);
    public static OperationStatus ToUpper(ReadOnlySpan<byte> source, Span<char> destination, out int charsWritten);
    public static OperationStatus ToUpper(ReadOnlySpan<char> source, Span<byte> destination, out int bytesWritten);

    public static OperationStatus ToLower(ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten);
    public static OperationStatus ToLower(ReadOnlySpan<char> source, Span<char> destination, out int charsWritten);
    public static OperationStatus ToLower(ReadOnlySpan<byte> source, Span<char> destination, out int charsWritten);
    public static OperationStatus ToLower(ReadOnlySpan<char> source, Span<byte> destination, out int bytesWritten);

    public static OperationStatus ToUpperInPlace(Span<byte> value, out int bytesWritten);
    public static OperationStatus ToUpperInPlace(Span<char> value, out int charsWritten);

    public static OperationStatus ToLowerInPlace(Span<byte> value, out int bytesWritten);
    public static OperationStatus ToLowerInPlace(Span<char> value, out int charsWritten);

    public static OperationStatus ToUtf16(ReadOnlySpan<byte> source, Span<char> destination, out int charsWritten);
    public static OperationStatus FromUtf16(ReadOnlySpan<char> source, Span<byte> destination, out int bytesWritten);

    public static Range Trim(ReadOnlySpan<byte> value);
    public static Range Trim(ReadOnlySpan<char> value);
    public static Range TrimStart(ReadOnlySpan<byte> value);
    public static Range TrimStart(ReadOnlySpan<char> value);
    public static Range TrimEnd(ReadOnlySpan<byte> value);
    public static Range TrimEnd(ReadOnlySpan<char> value);
}

@bartonjs

@bartonjs @GrabYourPitchforks @stephentoub I've realized that bytesWritten/charsWritten is not very accurate name for the out parameter for the in-place ToLower/ToUpper conversion when there is no need to convert anything. Imagine that we want to uppercase "ABC" string in-place. There is no need to copy any bytes/characters (charsWritten=0), but we need to process all of them.

What do you think about renaming written to processed for the in-place conversions?

- public static OperationStatus ToUpperInPlace(Span<byte> value, out int bytesWritten);
- public static OperationStatus ToUpperInPlace(Span<char> value, out int charsWritten);
+ public static OperationStatus ToUpperInPlace(Span<byte> value, out int bytesProcessed);
+ public static OperationStatus ToUpperInPlace(Span<char> value, out int charsProcessed);

- public static OperationStatus ToLowerInPlace(Span<byte> value, out int bytesWritten);
- public static OperationStatus ToLowerInPlace(Span<char> value, out int charsWritten);
+ public static OperationStatus ToLowerInPlace(Span<byte> value, out int bytesProcessed);
+ public static OperationStatus ToLowerInPlace(Span<char> value, out int charsProcessed);

What do you think about renaming written to processed for the in-place conversions?

The guideline is

CONSIDER naming an out parameter that reports the number of values
written by a buffer-writing method “bytesWritten,” “charsWritten,” or
“valuesWritten” (as appropriate to the data type).

If you see any existing OperationStatus method that has done something other than "Written" and has a similar justification, then maybe... but "Written" is what the pattern suggests, and it isn't so wrong as to feel like it needs something different.

Navigation Menu

Repository navigation

Create APIs to deal with processing ASCII text (as bytes) #28230

79 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Create APIs to deal with processing ASCII text (as bytes) #28230

Description

Activity

danmoseley commented on Dec 18, 2018

GrabYourPitchforks commented on Dec 19, 2018

danmoseley commented on Dec 19, 2018

gfoidl commented on Dec 19, 2018

GrabYourPitchforks commented on Dec 20, 2018

tarekgh commented on Jan 17, 2019

GrabYourPitchforks commented on Jan 19, 2019

tarekgh commented on Jan 19, 2019

vcsjones commented on Jan 24, 2019

GrabYourPitchforks commented on Jan 25, 2019

ahsonkhan commented on Jan 29, 2019

GSPP commented on Feb 3, 2019

GrabYourPitchforks commented on Feb 22, 2019

tarekgh commented on Apr 3, 2019

79 remaining items

adamsitnik commented on Dec 2, 2022

gfoidl commented on Dec 2, 2022

GrabYourPitchforks commented on Dec 6, 2022

bartonjs commented on Dec 6, 2022

adamsitnik commented on Dec 7, 2022

bartonjs commented on Dec 7, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions