[Proposal]: Provide a way to annotate methods that will always throw.

[tannergooding]

Scenario

It is fairly common practice to wrap the throwing of exceptions in a static class to ensure that a method can still be inlined in the scenario where it does not throw.

For example:

public static class ExceptionUtilities
{
    public static void ThrowArgumentNullException(string paramName)
    {
        throw NewArgumentNullException(paramName);
    }
}

public class MyClass
{
    public MyClass(string arg)
    {
        if (arg is null)
        {
            ExceptionUtilities.ThrowArgumentNullException(nameof(arg));
        }
    }
}

However, there is currently no way to indicate to the compiler that ExceptionUtilities.ThrowArgumentNullException will always throw. This breaks scenarios where you want to ensure that a variable is definitely assigned.

For example:

if (!(obj is Delegate d))
{
    ExceptionUtilities.ThrowArgumentExceptionForInvalidType(nameof(obj), obj.GetType());
}

Console.WriteLine(d);

The above code will fail to compile because the compiler thinks d could be unassigned. This is because it does not know that ExceptionUtilities.ThrowArgumentExceptionForInvalidType will always throw.

Proposal

The compiler should provide a way to indicate that a given method will always throw. Perhaps something like:

public static throw ThrowArgumentNullException(string paramName);

The compiler would then validate that the method unconditionally throws an exception or it would emit an error.

The compiler would additionally take this metadata into account when determining definite assignment for a variable.

[tannergooding]

This has some overlap with code contracts.

[tannergooding]

The behavior would likely need to be undefined in the scenario where a user compiles against an assembly that guarantees the throw but runs against an assembly where it does not (such as binding redirects).

[HaloFour]

This overlaps with #538 (and previously dotnet/roslyn#1226), which covers more scenarios than throwing.

[tannergooding]

@HaloFour, I haven't finished going through the proposal yet, but I think throw and never have a small section where they do not overlap.

It seems like never is used to annotate a method which will never return, which is not (strictly speaking) the case for throw.

For example:

try
{
    if (!(obj is Delegate d))
    {
        // Attributed with 'always throws'
        ExceptionUtilities.ThrowArgumentExceptionForInvalidType(nameof(obj), obj.GetType());
    }

    Console.WriteLine($"1: {d}"); // d is definitely assigned
}
catch (Exception e)
{
    Console.WriteLine($"2: {d}"); // d is potentially unassigned
}

Console.WriteLine($"3: {d}"); // d is potentially unassigned

In the above case, where attribute with always throws. The first C.WL is ok, but the second and third are not, because the exception thrown by the method may have been caught and handled.

Even if you were to attribute a type with never returns, you still couldn't definitely say 2/3 are assigned, because the call stack could be continued.

[tannergooding]

(also ignoring the fact that d isn't in scope for 2/3)

[HaloFour]

@tannergooding

Using a never type to represent an expression that will throw is one of the primary use cases. If a method is marked as returning "never" then that method will never return successfully. The only possible options are for the method to throw, block indefinitely or result in the termination of the current thread. The call stack can't continue; that's the entire point.

Using never types, in your example (assuming that you're assigning some other variable declared in a higher scope), d would always be assigned.

[tannergooding]

How is 2 definitely assigned? If the exception caught is the one thrown by the method, then obj is Delegate d was false and d is definitely not assigned. If it is any other exception caught then it is potentially assigned.

Here is actual valid code:

if (!(obj is Delegate d))
{
    ExceptionUtilities.ThrowArgumentExceptionForInvalidType(nameof(obj), obj.GetType());
}

Console.WriteLine($"1: {d}"); // d is definitely assigned

vs:

if (!(obj is Delegate d))
{
    try
    {
        ExceptionUtilities.ThrowArgumentExceptionForInvalidType(nameof(obj), obj.GetType());
    }
    catch (Exception e)
    {
    }
}

Console.WriteLine($"1: {d}"); // d is potentially assigned

never returns, at least to me, doesn't make sense for this scenario because the exception is either unhandled (thus crashing the application) or it is handled and control flow continues at some point higher in the callstack.

Cleaning the stack and any other optimizations that could be performed with noreturn become invalid for managed code once exception handling comes into play.

[HaloFour]

Oops, you're right. d wouldn't be definitely assigned in cases 2 and 3, in either of our scenarios.

never absolutely makes sense in this case as it would apply to ExceptionUtilities.ThrowArgumentExceptionForInvalidType. The compiler would then know that when calling that method that the execution would never return to the caller unless an exception was being thrown. The remainder of that if block can never be invoked.

public static class ExceptionUtilities
{
    public static never ThrowArgumentExceptionForInvalidType(string paramName, Type paramType)
    {
        throw NewArgumentInvalidTypeException(paramName, paramType);
        // compiler ensures that this method can never successfully return
    }
}

public class C {
    public void M1(object obj) {
        if (!(obj is Delegate d)) {
            ExceptionUtilities.ThrowArgumentExceptionForInvalidType(nameof(obj), obj.GetType());
            // compiler ensures execution can never get here
        }
        Console.WriteLine(d); // d is definitely assigned
    }

    public void M2object obj) {
        if (!(obj is Delegate d)) {
            try {
                ExceptionUtilities.ThrowArgumentExceptionForInvalidType(nameof(obj), obj.GetType());
                // compiler ensures execution can never get here 
            }
            catch { }
            // however, execution can reach here
        }
        Console.WriteLine(d); // d is not definitely assigned
    }
}

[tannergooding]

I'm probably going to keep this open, since it appears the primary motivation between #538 seems to be to expand the set of things which can be treated as expressions, while this is specifically trying to cover a "gap" in definite assignment analysis. Both would end up covering what I am looking for, but the other proposal is just too broadly scoped for my liking 😄

[HaloFour]

@tannergooding

Sure, it's a different way to tackle the same problem. I just wanted to note the similarity. While the motivation was partly to implement throw exceptions (which didn't need it), determining definite assignment is one of the first things mentioned on dotnet/roslyn#1226:

The compiler would treat a value of the type specially for definite assignment purposes - whenever a value of this type appears on the stack, everything is definitely assigned and the code is unreachable.

I've taken to using a different convention for my exception helpers. Rather than having the helper throw it instead returns an instance of Exception which the caller is then expected to throw. Then the compiler already handles definite assignment:

if (!(obj is Delegate d)) {
    throw ExceptionUtilities.ArgumentExceptionForInvalidType(nameof(obj), obj.GetType());
}
Console.WriteLine(d); // d is definitely assigned here

[tannergooding]

The 'issue' with that convention is that throwing the exception directly in the method prevents the JIT from inlining that method (I think CoreCLR may have relaxed this for some scenarios, such as when it detects the exception will truly never throw).

The purpose of wrapping the entire exception throw is to ensure that the method can still be inlined if it is sufficiently small.

For example, I might have the following method:

[DllImport("D3D12", BestFitMapping = false, CallingConvention = CallingConvention.Winapi, CharSet = CharSet.Unicode, EntryPoint = "D3D12CreateDevice", ExactSpelling = true, PreserveSig = true, SetLastError = false, ThrowOnUnmappableChar = false)]
[SuppressUnmanagedCodeSecurity]
public static extern HRESULT D3D12CreateDevice(
    [In, Optional] IUnknown* pAdapter,
    [In] D3D_FEATURE_LEVEL MinimumFeatureLevel,
    [In] ref Guid riid,
    [Out, Optional] void** ppDevice
);

For the fourth parameter, ppDevice, I am using unsafe code because the parameter is optional and can be set to null if you just want to validate whether the pAdapter supports MinimumFeatureLevel (thus going down a significantly cheaper code-path). I cannot use out because you cannot pass null to an out parameter.

I might then have a wrapper method which exposes the two code-paths in a "safe" manner:

public bool Supports(FeatureLevel featureLevel)
{
    var result = D3D12CreateDevice(_handle, (D3D_FEATURE_LEVEL)(uint)(featureLevel), ref IID_ID3D12Device, null);
    return result.Succeeded;
}

public GraphicsDevice CreateDevice(FeatureLevel featureLevel)
{
    void* pDevice = null;
    var result = D3D12CreateDevice(_handle, (D3D_FEATURE_LEVEL)(uint)(featureLevel), ref IID_ID3D12Device, &pDevice);

    result.ThrowExceptionIfFailed();
    return new GraphicsDevice((IntPtr)(pDevice));
}

The second method, CreateDevice, is small enough that it can be inlined, but only if the exception handling for the failure scenario is wrapped in a separate call.

[HaloFour]

@tannergooding

<rant>
Worse than the quirks of the compiler itself is having to learn and memorize the quirks of the JIT implementation that will have measurable performance impact on your code. Those should largely be implementation details. They're effectively a moving target, so being "clever" to exploit a performance boost today might yield a penalty tomorrow.
</rant>

[CyrusNajmabadi]

I agree with @HaloFour . I would prefer this sort of thing not be put into the language to try to work around a specific issue with the jit that could be addressed.

[tannergooding]

@CyrusNajmabadi, I think that this is something that cannot be fully addressed by the runtime.

Not only is some analysis outside the scope of what the JIT can do (due to time constraints), but even if it is able to optimize away all the calls that are unnecessary, there will still be checks that are required or that can't be optimized away and those cannot be inlined because it would break certain functionality (such as capturing the entire call stack).

I think we are getting to the point where, if users want to write performant code in C#, the language will need to start providing functionality and optimizations itself. It will need to due this due to time constraints that the JIT has, due to AOT not always being available (or possibly wanted).

The runtime is also going to need to start providing features that allow users to provide hints that help offload pressure or even to allow users a guarantee that an intrinsic will be used if it is available on the underlying platform (even today, implementing a certain pattern isn't guaranteed to have the JIT output the appropriate intrinsic, such as if the method is complex/large).

I'm not saying that this particular proposal is the right way to fix this particular problem, just that I think this is one of the problems that will require language or compiler support and that their will be plenty more like this coming in the future.

[mikedn]

The 'issue' with that convention is that throwing the exception directly in the method prevents the JIT from inlining that method

Not really, having a throw in a method does not explicitly prevent inlining. In fact until about a year ago the JIT happily inlined throw helpers thus defeating the whole point of using such helpers.

Now, having throws in a method simply increases code size and that may lead to inlining failures. Using a throw helper helps a bit in this regard but only up to a point. The real reason why some use throw helpers is not inlining but the fact that native code generated for a throw is pretty large. For example preventing throw helper inlining saved ~100k in corelib (which tends to use throw helpers).

After the JIT was changed to no longer inline such helpers an effort was started to use throw helpers in more places in the framework. Soon enough it was decided that changing all throws is just not worth it and this should only be done in commonly used generic code. And the rest of the cases would better be handled by some sort of IL optimizer tool that outlines throw blocks. Makes sense, when you have to create such helpers to keep code size down it means that the tools are failing you. Improving the tools to make it easier to workaround their own failure isn't very useful.

[Joe4evr]

And the rest of the cases would better be handled by some sort of IL optimizer tool that outlines throw blocks.

And that would fall under dotnet/roslyn#15929.

[tannergooding]

having a throw in a method does not explicitly prevent inlining

Interesting, I've always seen, even in simple cases, that it prevents my method from being inlined (and thought I had read similar in documentation somewhere, I must be remembering incorrectly 😄).

[alrz]

This has some overlap with code contracts.

think it could morph into ensures clause.

// `ensures` ensures the following expression is always true when we leave the method
// `false` means we don't
static void ThrowArgumentNullException(string p) ensures false { ... } 

[juliusfriedman]

I like the idea, feels similar to checked exceptions (throws) in java but overall I feel it should be worded like so:

The compiler should provide a way to indicate that a given method will always throw and indicate what it will throw. Perhaps something like:

public static throw ThrowArgumentNullException(string paramName);
The compiler would then validate that the method unconditionally throws the indicated exception type or it would emit an error. (Use Exception to provide general throwing where the type may be unknown or Generic)

The compiler would additionally take this metadata into account when determining definite assignment for a variable and could be enhanced to provide for analysis where the given exception type is not handled in the calling code.

This also begs for the question what if I have a method that throws various different types of exceptions given an object? e.g. a CheckObjectDisposed or even a Validation API of some type which uses exceptions to propagate the information?

It could be argued that such as method should be refactored to provide a single responsibility and throw one type of exception or use a generic Exception....

[Joe4evr]

The compiler should provide a way to indicate that a given method will always throw and indicate what it will throw.

Not all methods that don't return properly are because they throw an exception; look no further than Environment.FailFast().

[Foxtrek64]

I do like using never/bottom types for this (#538) as extensively discussed in that thread.
Alternatively, #105 (and by extension dotnet/roslyn#119) would be an interesting alternative.

public static void ThrowArgumentNullException(string paramName, string message)
    ensures return throws ArgumentNullException
{
    throw new ArgumentNullException(paramName, message);
}

Multiple options here and I'm interested to see where this goes.

Edit: just noticed alrz suggested code contracts earlier.

[333fred]

Closing as a duplicate of #8604.