-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Add SwitchExpressionException #34954
Add SwitchExpressionException #34954
Conversation
79ae1b3
to
a28dfb8
Compare
Fixes: #33284
a28dfb8
to
3ec5b78
Compare
...m.Runtime.Extensions/tests/System/Runtime/CompilerServices/SwitchExpressionExceptionTests.cs
Outdated
Show resolved
Hide resolved
src/System.Runtime.Extensions/src/System/Runtime/CompilerServices/SwitchExpressionException.cs
Outdated
Show resolved
Hide resolved
src/System.Runtime.Extensions/src/System/Runtime/CompilerServices/SwitchExpressionException.cs
Outdated
Show resolved
Hide resolved
src/System.Runtime.Extensions/src/System/Runtime/CompilerServices/SwitchExpressionException.cs
Outdated
Show resolved
Hide resolved
src/System.Runtime.Extensions/src/System/Runtime/CompilerServices/SwitchExpressionException.cs
Outdated
Show resolved
Hide resolved
public override void GetObjectData(SerializationInfo info, StreamingContext context) | ||
{ | ||
base.GetObjectData(info, context); | ||
info.AddValue(nameof(UnmatchedValue), UnmatchedValue, typeof(object)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the type of UnmatchedValue isn't serializable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of exception serialization in .NET Core is basically telemetry (viz Azure Backup that requested we bring it back). So what I suggest is attempting to serialize, and if there's a SerializationException, create some kind of potentially useful string (using ToString and possibly ellipses?) and stuff that in there instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW if we think that's the right approach, then we should open an issue to make the same fix to ArgumentOutOfRangeException
which serializes a private field of arbitrary type. In the .NET Core world, it's more likely that's not in fact binary serializable and since most exceptions aren't serialized most of the time, it wouldn't necessraily be noticed for a while.
Similar for RuntimeWrappedException
and a few others. In fact serailization of XmlSchemaValidationException
seems always broken - it serializes an object that looks like it can be (maybe always is) an XmlNode, which we don't support serializing. BinaryFormatterTestData tests a constructor overload that doesn't set this object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what I suggest is attempting to serialize
We probably don't need to actually attempt to serialize, but could instead check obj.GetType().IsSerializable.
Regardless, your suggestion for what to do if it's false sounds reasonable: ToString() it and treat that as the UnmatchedValue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the pattern to use? i.e. where should I be checking for UnmatchedValue.GetType().IsSerializable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the other end of the spectrum, the matched item can be a really big serializable object graph. You can end up with megabytes of serializable payload by accident.
If we think about the exception binary serialization should be optimized for telemetry, we may want to always convert the object to string to have a better chance for a reasonably sized payload.
The binary serialization of exception stack trace is handled in similar way. The stacktrace is converted to string for binary serialization because of the elements that it is constructed from are not binary-serialization friendly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the worst case ToString at least gives me the type name. Im thinking of how many times had to diagnose something with only the exception details in a log or bug report. That has made me quite biased towards including any potentially useful information. @stephentoub what are your thoughts? We seem to be having the same discussion in the linked issue 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@arunjvs any thoughts, I believe you were or first customer for exception serialization..? See #35041
Always ToString might work although it could be worse if ToString is not usefully implemented on the type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am all for having the right level of detail in ToString
.
I just do not think we should spending any cycles on trying to make binary serialization better in creative ways. Binary serialization is a dead end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maryamariyan please go ahead and submit without the IsSerializable check, and we can have this discussion in #35041
...m.Runtime.Extensions/tests/System/Runtime/CompilerServices/SwitchExpressionExceptionTests.cs
Outdated
Show resolved
Hide resolved
...m.Runtime.Extensions/tests/System/Runtime/CompilerServices/SwitchExpressionExceptionTests.cs
Show resolved
Hide resolved
4736db7
to
4950167
Compare
src/System.Runtime.Extensions/src/System/Runtime/CompilerServices/SwitchExpressionException.cs
Outdated
Show resolved
Hide resolved
src/System.Runtime.Extensions/src/System/Runtime/CompilerServices/SwitchExpressionException.cs
Outdated
Show resolved
Hide resolved
src/System.Runtime.Extensions/src/System/Runtime/CompilerServices/SwitchExpressionException.cs
Outdated
Show resolved
Hide resolved
src/System.Runtime.Extensions/src/System/Runtime/CompilerServices/SwitchExpressionException.cs
Outdated
Show resolved
Hide resolved
src/System.Runtime.Extensions/src/System/Runtime/CompilerServices/SwitchExpressionException.cs
Outdated
Show resolved
Hide resolved
src/System.Runtime.Extensions/src/System/Runtime/CompilerServices/SwitchExpressionException.cs
Outdated
Show resolved
Hide resolved
src/System.Runtime.Extensions/src/System/Runtime/CompilerServices/SwitchExpressionException.cs
Outdated
Show resolved
Hide resolved
src/System.Runtime.Extensions/src/System/Runtime/CompilerServices/SwitchExpressionException.cs
Outdated
Show resolved
Hide resolved
8b82654
to
04ce80b
Compare
04ce80b
to
91f6c16
Compare
9429bfa
to
941db3a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with the contructors as specified by Immo.
ed81ceb
to
e93700a
Compare
@dotnet-bot test OSX x64 Debug Build @dotnet-bot test UWP CoreCLR x64 Debug Build |
@dotnet/dnceng UWP CoreCLR x64 Debug Build failure seems unrelated to change in PR: Logs show
For
|
That error is discussed here: #35142 |
CI was previously green, overall this was completely tested |
* Add SwitchExpressionException Fixes: dotnet/corefx#33284 * Apply PR feedbacks * Type just added to .net core 3.0 * quick cleanup * Add test in BinaryFormatterTestData * Updated the APIs for SwitchExpressionException * Remove IsSerializable condition Commit migrated from dotnet/corefx@3685052
Fixes: #33284
cc: @danmosemsft, @ViktorHofer