Add a way to opt out of TargetInvocationException wrapping on late-bound invokes.

[kingces95]

Late-bound invokes through Reflection wrap exceptions thrown by the target in a TargetInvocationException. In many cases, this behavior is not desired and counter-productive. For those who want the actual exception, it requires unwrapping the TargetInvocationException to rethrow the inner exception and to retrieve the full call stack. The fact that every exception is "caught" hampers the normal debugging experience. It'd be useful to have a way to opt out of this wrapping.

We can do this without adding lots of new overloads by adding a new member to the BindingFlags enum: BindingFlag.DoNotWrapExceptions. Setting this bit would disable the wrapping behavior.

Here is a fiddle of the code sample included below: https://dotnetfiddle.net/o9qUht

public class Program
{
	public static void Main()
	{
		try {
			var bf = BindingFlags.Static | 
                            BindingFlags.Public | 
                            BindingFlags.InvokeMethod;

                        bf |= BindingFlags.DoNotWrapExceptions;

			typeof(Program).InvokeMember("LateBoundTarget", bf, null, null, null);

		} catch(TargetInvocationException e) {
			Console.WriteLine("fail");

		} catch(InvalidOperationException e) {
			Console.WriteLine("success");
		}
	}
	
	public static void LateBoundTarget() {
		throw new InvalidOperationException();
	}
}

Apis that would be affected:

    public static class Activator
    {
        public static object CreateInstance(Type type, BindingFlags bindingAttr, Binder binder, object[] args, CultureInfo culture);
        public static object CreateInstance(Type type, BindingFlags bindingAttr, Binder binder, object[] args, CultureInfo culture, object[] activationAttributes);
    }

    public class Type
    {
        public object InvokeMember(string name, BindingFlags invokeAttr, Binder binder, object target, object[] args);
        public object InvokeMember(string name, BindingFlags invokeAttr, Binder binder, object target, object[] args, CultureInfo culture);
        public abstract object InvokeMember(string name, BindingFlags invokeAttr, Binder binder, object target, object[] args, ParameterModifier[] modifiers, CultureInfo culture, string[] namedParameters);
    }

    public class Assembly
    {
        public virtual object CreateInstance(string typeName, bool ignoreCase, BindingFlags bindingAttr, Binder binder, object[] args, CultureInfo culture, object[] activationAttributes);
    }

    public abstract class ConstructorInfo : MethodBase
    {
        public abstract object Invoke(BindingFlags invokeAttr, Binder binder, object[] parameters, CultureInfo culture);
    }

    public abstract class MethodBase : MemberInfo
    {
        public abstract object Invoke(object obj, BindingFlags invokeAttr, Binder binder, object[] parameters, CultureInfo culture);
    }

    public abstract class PropertyInfo : MemberInfo
    {
        public abstract object GetValue(object obj, BindingFlags invokeAttr, Binder binder, object[] index, CultureInfo culture);
        public abstract void SetValue(object obj, object value, BindingFlags invokeAttr, Binder binder, object[] index, CultureInfo culture);
    }

[EDIT] Added C# syntax highlight by @karelz

[ghost]

@MichalStrehovsky - Thoughts? This does seem like a pragmatic way to address a behavior that's been a long-standing irritance (at least for me.)

[MichalStrehovsky]

@MichalStrehovsky - Thoughts?

Heh, I never even saw the overload of Invoke/GetValue/SetValue that takes BindingFlags. I guess this should work. We just have to make sure we do whatever is the consistent thing to do when someone passes the new BindingFlag to e.g. Type.GetMethod (I assume we wouldn't return a different MethodInfo based on that).

Do we want to also take this opportunity to propose an overload of Activator.CreateInstance<T> that takes binding flags? We would obviously only respect the new binding flag and do whatever is the consistent thing for anything else (throw/ignore).

[ghost]

Activator.CreateInstance<T> - if we add a BindingFlags parameter, people will probably expect it to honor Public/NonPublic/Instance/Static as well as the new bit.

[JonHanna]

Surely CreateInstanceis ipso facto Instance? The behaviour with the other flags could be like that of the non-generic version that takes them, but its use is already more limited (one can normally do new T() if T is known at jit time to be suitable for use with it), so perhaps there's no gap to fill.

[MichalStrehovsky]

one can normally do new T()

new T() actually expands to Activator.CreateInstance<T> with all the baggage (exceptions thrown from the constructor will be wrapped in TargetInvocationException). That compat ship has unfortunately already sailed. The new overload would be for people who expect new T() to do the reasonable thing but can't express it currently. We can make Activator.CreateInstance<T> super fast, as opposed to the overload that takes a Type (we already do make it fast on AOT platforms).

Making it also work for private constructors is a bunch of work for AOT, so I would propose we make BindingFlags.NonPublic throw and see how the feedback looks like?

[JonHanna]

with all the baggage

True, it could be worth losing the syntactic sugar of new() for getting away from that.

Making it also work for private constructors is a bunch of work for AOT

Could it slow-path through (T)Activator.CreateInstance(typeof(T), … so even if it isn't as fast, it still produces the correct results?

[MichalStrehovsky]

Could it slow-path through (T)Activator.CreateInstance(typeof(T), … so even if it isn't as fast, it still produces the correct results?

It could, but it always makes me sad to have APIs with perf characteristics that can't be reasoned about (sure, we could document that, but who reads docs?).

If we make BindingFlags.NonPublic a thing, it's an extra day of work for CoreRT. Without that, it's as little work as adding a when clause to the catch here.

[JonHanna]

To the consumer, the main speed difference between slow and not working is how quickly they get sad.

[ghost]

There are also other ways to achieve non-exception-wrapping CreateInstance<T>() - a boolean parameter or a differently named method. That might be prefereable to over-advertising a capability we intend to throw on.

In any case, we should probably keep that separate from this api proposal. If the basic notion of a non-wrapping Invoke doesn't fly with the api review folks, figuring out how to extend it to the generic CreateInstance becomes moot.

[ghost]

Cleaned up the proposal a bit and promoted to api-ready-for-review.

We can tackle the CreateInstance<T>() thing separately if this gets approved.

[terrajobst]

Video

Looks good, the only suggestion is to rename PassExceptions to DoNotWrapExceptions. The rationale being that specifying ~PassExceptions could be read as catch-all.

var bf = BindingFlags.Public | BindingFlags.DoNotWrapExceptions;

[JonHanna]

That has nice didactic qualities too: Seeing DoNotWrapExceptions immediately teaches people that exceptions get wrapped!

[ghost]

Next steps: will add the enum value to BindingFlags in the code so both CoreCLR and CoreRT use the same (numerical) value. Then we open separate issues for implementation on CoreRT and CoreCLR. (Latter can be up for grabs.)

[karelz]

Marking as up-for-grabs.
Complexity wise: medium (see next steps in previous comment from @AtsushiKan)

[karelz]

FYI: The API review discussion was recorded - see https://youtu.be/VppFZYG-9cA?t=5527 (duration: 14 min)

[ghost]

Project N implementation is now merged.

CoreCLR implementation is still up for grabs. The exception wrapping seems to occur in various places in C++ code so this will require some C++ work.