Create Custom Destructurers for More Exceptions
RehanSaeed opened this issue · 7 comments
Describe the feature
To avoid using reflection we can create custom destructurers for more exception types. ArgumentExceptionDestructurer is a good example for how to do this.
- System.AggregateException
- System.BadImageFormatException
- System.Diagnostics.Contracts.ContractException
- System.Globalization.CultureNotFoundException
- System.IO.FileLoadException
- System.IO.FileNotFoundException
- System.MissingFieldException
- System.MissingMemberException
- System.MissingMethodException
- System.NotFiniteNumberException
- System.ObjectDisposedException
- System.Reflection.ReflectionTypeLoadException
- System.Resources.MissingSatelliteAssemblyException
- System.Runtime.CompilerServices.RuntimeWrappedException
- System.Runtime.CompilerServices.SwitchExpressionException
- System.Runtime.InteropServices.ExternalException
- System.Security.SecurityException
- System.Text.DecoderFallbackException
- System.Text.EncoderFallbackException
- System.Threading.AbandonedMutexException
- System.Threading.Tasks.TaskCanceledException
- System.Threading.ThreadAbortException
- System.TypeInitializationException
- System.TypeLoadException
Just a question: is it worth it? The cost of reflection is negligible and paid just for the first time exception is destructured. The cost is also minuscule compared to the cost of logging anyway.
Supporting so many types of exceptions manually seems like a burden in terms of lines of code/tests/maintenance.
As was thinking even, maybe let's go the other way around, and remove all(or most) custom destructurers we have and leave just reflection-based one?
We only have a few custom destructurors at the moment. The list above just completes everything built into System.*. Some of the above are more important than others. I've added them all above for completeness.
The cost can add up if you have an app e.g. if you have an app throwing large numbers of TaskCanceledException exceptions for example which can happen in real life quite easily. There is an emphasis on performance in .NET lately, so I think it makes sense.
Hi @RehanSaeed, would you accept a PR that adds a destructurer for RegexMatchTimeoutException?
@Driedas Sure!
@RehanSaeed
heh, never mind, I've cloned the project, added the following test to prove that this isn't implemented yet:
var exception = new RegexMatchTimeoutException("input", "pattern", TimeSpan.FromSeconds(1));
Test_LoggedExceptionContainsProperty(exception, nameof(exception.Input), exception.Input);
Imagine my surprise when it turned green instead of red as expected :-). Looks like ReflectionBasedDestructurer already takes care of this using a bit of smart reflection. D'oh, TDD scores...
Yes, as you've found it works but requires reflection to output. I'd still accept a PR to add support without using reflection.
I believe we may be able to do this with a source generator. Worth looking into.