Investigate changing "throw ex.InnerException" call sites to preserve stack info

[GrabYourPitchforks]

It's extraordinarily common for code to unwrap an outer exception, get at its Exception.InnerException property, and rethrow it. (See all coreclr / corefx references to the InnerException property.)

TargetInvocationException is a particularly interesting use case, because all reflection APIs wrap any inner exception in this outer type, necessitating that the calling code manually unwrap.

By making InnerExceptionDispatchInfo a first-class citizen on the Exception class, anybody who needs to unwrap + rethrow can do so while still preserving full fidelity of the inner stack trace.

I don't have a concrete API proposal right now because I need to think further about how this interacts with the existing reflection APIs and the existing Exception ctors. It's just a high-level idea for now.

[stephentoub]

I'm not quite understanding. Are you proposing something to make:

var edi = ExceptionDispatchInfo.Capture(e.InnerException ?? e);

simpler? Or to make:

var edi = ExceptionDispatchInfo.Capture(e.GetBaseException());

simpler? Can you share an example from coreclr/corefx that you're trying to improve?

TargetInvocationException is a particularly interesting use case, because all reflection APIs wrap any inner exception in this outer type, necessitating that the calling code manually unwrap.

This is no longer required if you pass BindingFlags.DoNotWrapExceptions.

[stephentoub]

@GrabYourPitchforks, can this be closed?

[GrabYourPitchforks]

I don't think this should be closed just yet, as there's still room for improvement. Here's one common pattern which attempts to preserve the stack:

https://source.dot.net/#Microsoft.AspNetCore.Hosting/Startup/ConventionBasedStartup.cs,31

And here's another which loses the stack:

https://source.dot.net/#Microsoft.AspNetCore.Mvc.Core/ModelBinding/Validation/DefaultComplexObjectValidationStrategy.cs,115

The unwrapping code doesn't always have the ability to pass a BindingFlags to reflection, as often it's not the component responsible for the reflection call.

[GrabYourPitchforks]

If within the catch block we can wrap the inner exception with an EDI and rethrow it, and if this preserves the stack trace appropriately, then this could potentially be an analyzer instead of a public API.

[davidfowl]

I think BindingFlags.DoNotWrapExceptions solves the problem but it requires everyone to opt-into it. We applied this for the hosting code https://github.com/dotnet/aspnetcore/blob/f3f9a1cdbcd06b298035b523732b9f45b1408461/src/Hosting/Hosting/src/Internal/MethodInfoExtensions.cs#L17. Even though that logic still accounts for catching TAE it's for nested code (user code that might be using reflection and not using DoNotWrapExceptions.

I'll file an issue to look at the places we're not using this in ASP.NET Core.

[stephentoub]

If within the catch block we can wrap the inner exception with an EDI and rethrow it, and if this preserves the stack trace appropriately

We have ExceptionDispatchInfo.Throw(e);.

[GrabYourPitchforks]

Maybe we should instead use this issue to track fixing up existing rethrow call sites and to introduce a repo-wide static analyzer to enforce good hygiene within first-party code?

[stephentoub]

Sure. Keeping in mind there are call sites that purposefully throw to get rid of the call stack and keep everything below it an implementation detail (though we could reconsider such cases). Please rename the issue appropriately.