Runtime: Should the Span-based Try* methods be renamed?

[kevinchalet]

As part of the (awesome) Span work, many Try* methods have been introduced in a bunch of places (e.g https://github.com/dotnet/corefx/blob/master/src/System.Security.Cryptography.Algorithms/src/System/Security/Cryptography/RSA.cs).

Historically, the Try* pattern has almost always been used for methods that don't throw under normal circumstances and instead return a boolean indicating whether the operation was successful or not. Yet, many of these new methods don't respect this pattern and only use the boolean to indicate whether the destination Span was large enough (e.g RSA.TryDecrypt() will throw an exception if the source data is invalid).

Using the Try* prefix for methods that actually throw is extremely confusing. Couldn't you remove it and use OperationStatus instead of the boolean?

[danmosemsft]

@KrzysztofCwalina

[joshfree]

/cc @ahsonkhan @GrabYourPitchforks

[GrabYourPitchforks]

From an API design perspective, returning OperationStatus from APIs we expect developers to use commonly makes me nervous. I've seen if (Foo(...) == ERROR_INVALID_DATA) { /* ... */ } on StackOverflow more times than I care to count, and I'd rather not perpetuate that incorrect convention throughout the C# development landscape.

But I _do_ agree with the original concern. We need to go through these APIs and ensure that we're following _some_ sane convention. The example of TryDecrypt throwing when presented with invalid data showcases that we're creating an inconsistent development story.

[KrzysztofCwalina]

Well, here is what the Try pattern guidelines say (see the last paragraph):

7.5.2 Try-Parse Pattern
For extremely performance-sensitive APIs, an even faster pattern than the Tester-Doer Pattern described in the previous section should be used. The pattern calls for adjusting the member name to make a well-defined test case a part of the member semantics. For example, DateTime defines a Parse method that throws an exception if parsing of a string fails. It also defines a corresponding TryParse method that attempts to parse, but re-turns false if parsing is unsuccessful and returns the result of a successful parsing using an out parameter.
c# public struct DateTime { public static DateTime Parse(string dateTime){ ... } public static bool TryParse(string dateTime, out DateTime result){ ... } }
When using this pattern, it is important to define the try functionality in strict terms. If the member fails for any reason other than the well-defined try, the member must still throw a corresponding exception.

[Clockwork-Muse]

In particular, for DateTime.TryParse returns false if the string parameter is null or empty, which could be thought equivalent to "this Span isn't long enough".

[morganbr]

@KrzysztofCwalina, can you expand on that a bit? If I understand correctly, DateTime.TryParse will return false for:

• A null string
• An empty string
• A string whose contents can't be successfully parsed

It could still throw due to underlying dependencies (e.g. OutOfMemoryException, IOException).

I would think that decrypting data would be comparable to parsing a string into a DateTime. That might mean that TryDecrypt should return false for:

• Null/empty inputs
• Failed decryption
  But throw for:
• Errors accessing the key
• Out of memory

cc @bartonjs

[bartonjs]

What Krzysztof told me in a hallway once is "every false return should have the same caller response". In the case of DateTime.TryParse it's "Your input is no good, give me a different input" for all three of those cases.

For TryDecrypt you'd really want to know the difference between "you didn't give me enough space to write down the answer" and "this data and this key don't go together", because the recovery action for it as currently written is "get bigger, try again". The data+key mismatch will always be true, getting bigger just makes you eventually OOM.

_{(Quoting not attributing direct words to Krzysztof, but the concept as I understood it)}

[KrzysztofCwalina]

I am not sure if there are more questions/discussion related to this. If people feel some specific Try APIs should be changed, please reopen.

[kevinchalet]

@KrzysztofCwalina I don't think the concern was addressed. Could you please re-open? (I can't do it myself).

[bartonjs]

@PinpointTownes What do you feel wasn't addressed?

@GrabYourPitchforks Expressed the concern with OperationStatus-type responses here. @KrzysztofCwalina quoted the rules-as-written for the Try pattern (which, I'll admit, are different than the "it won't throw" that most people seem to have internalized).

TryDecrypt is consistent with Convert.TryFromBase64String and many other Spanified methods in that it returns false for "destination too short" and throws for "invalid".

[kevinchalet]

@GrabYourPitchforks Expressed the concern with OperationStatus-type responses here.

He also said the resulting story was inconsistent, which was confirmed by @morganbr:

I would think that decrypting data would be comparable to parsing a string into a DateTime.

TryDecrypt is consistent with Convert.TryFromBase64String and many other Spanified methods in that it returns false for "destination too short" and throws for "invalid".

It's perhaps consistent with Convert.TryFromBase64String but it's inconsistent with Base64.DecodeFromUtf8, that - unlike the Try equivalent - doesn't throw for invalid input.

IMHO it doesn't make much sense for Convert.TryFromBase64String to throw an exception if the input string is malformed, just like it wouldn't make sense for DateTime.TryParse to throw an exception for the same reason.

[kevinchalet]

@GrabYourPitchforks Expressed the concern with OperationStatus-type responses here.

If OperationStatus is not a good option for Span-based methods, then it shouldn't be used at all for consistency.

I'm sure there are also other patterns you could explore. E.g you could add a new type offering an implicit conversion to bool:

public readonly struct SpanResult
{
    private SpanResult(OperationStatus state) => Status = state;

    public OperationStatus Status { get; }

    public static SpanResult Done
        => new SpanResult(OperationStatus.Done);

    public static SpanResult DestinationTooSmall
        => new SpanResult(OperationStatus.DestinationTooSmall);

    public static SpanResult NeedMoreData
        => new SpanResult(OperationStatus.NeedMoreData);

    public static SpanResult InvalidData
        => new SpanResult(OperationStatus.InvalidData);

    public static implicit operator bool(SpanResult result)
        => result.Status == OperationStatus.Done;
}

static SpanResult Parse(string input, Span<byte> bytes, out int bytesWritten)
{
    if ([invalid input])
    {
        bytesWritten = 0;
        return SpanResult.InvalidData;
    }

    if ([buffer too small])
    {
        bytesWritten = 0;
        return SpanResult.DestinationTooSmall;
    }

    bytesWritten = [bytes written];
    return SpanResult.Done;
}

static void Main(string[] args)
{
    if (Parse(input, bytes, out int bytesWritten))
    {
        // Everything is okay.
    }

    if (!Parse(input, bytes, out int bytesWritten))
    {
        // Something went wrong (invalid data, buffer too small, etc.).
    }

    var result = Parse(input, bytes, out int bytesWritten);
    if (result.Status == OperationStatus.DestinationTooSmall)
    {
        // Try again with a larger buffer.
    }
    else
    {
        // Otherwise, retrying is pointless.
    }
}

[terrajobst]

The way I (and some others on our team) have been looking at TryXxx is that it's less about exceptions and more about whether the caller is expected to handle the situation that a valid return value cannot be produced. It's basically an alternative for the (somewhat) well established patterns where null is returned (for reference types).

The idea of using TryXxx is to make that very visible. In my opinion this is different from exceptions in that exceptions are usually used in error conditions and are often handled by code higher up the call chain (ignoring contract violations like ArgumentExceptions which argubaly shouldn't be exceptions but process terminating conditions). In contrast to excetions, the idea of TryXxx is making it part of the operational contract of the API that the immediate caller is expected to handle it.

We considered using an enum like OperationStatus but the downside is that it kind of implies the caller has to handle all possible enum cases where in the TryXxxx it's binary only, which avoids having to read up which return values of OperationStatus this API will actually produce.