Repository navigation
ShouldSerializeValue for PropertyDescriptor fails for values of underlying enum types #52116
Description
Description
The ShouldSerializeValue method of the PropertyDescriptor fails, when - in C# - a DefaultValue for a Property of an enum type is not defined with the enum type, but with it's underlying type. For example:
internal class Foo : Component
{
[DefaultValue(0)]
public TestEnum Bar { get; set; }
}
internal enum TestEnum
{
Value1 = 0,
Value2 = 1
}
This is even more dramatic in Visual Basic, where the compiler replaces the Enum type by its underlaying type by default, which makes properties of type enum, which have been defined in Visual Basic, never succeed in a ShouldSerializeValue call.
<DefaultValue(TestEnum.Value1)>
Public Property Bar As TestEnum
becomes automatically (IL translated to C#):
[DefaultValue(0)]
public TestEnum Bar
{
[CompilerGenerated]
get
{
return _Bar;
}
[CompilerGenerated]
set
{
_Bar = value;
}
}
Background:
When the DefaultValue property of a ReflectPropertyDescriptor is called, e.g. by calling ShouldSerializeProperty of PropertyDescriptor, and then the DefaultValue is of Type Enum but the value is given of the type of the underlying type, a NullReferenceException is thrown and the call fails.
Making this work and most likely serviced from .NET Core 3.1 on is, amongst other scenarios, critical for the Windows Forms Designer not to crash in those scenarios. (Which are not niche.)
Configuration
- All .NET Core/.NET Versions.
- Windows.
- x86, x64, ARM
- No.
Regression?
No.
Other information
The error might be here:
where toObject is called with _defaultValue instead of the local variable defaultValue.
(Thanks @DustinCampbell for helping tracking this down.
Also @merriemcgaw, @RussKie, @JeremyKuhne, @Pilchie for reference.)
Activity
merriemcgaw commentedon Apr 30, 2021
Please note that this is blocking a common scenario for WinForms devs, and now that usage of the designer is increasing it's going to generate a lot of noise as people port their apps. We should consider servicing 3.1 and 5 as they are the most common targets for porting Framework apps to.
merriemcgaw commentedon Jul 14, 2021
@maryamariyan can you let us know if there is any status update on this one? It is blocking a high priority scenario for the WinForms designer and we need it fixed in .NET 6, probably even serviced so customers building Custom Controls are able to use their default values as they did before.
cc: @Pilchie
safern commentedon Jul 15, 2021
FYI: @ericstj
ericstj commentedon Jul 15, 2021
@merriemcgaw @KlausLoeffelmann this was not filed as a regression, did the same behavior exist on .NETFramwork? Why is this so important to fix now if it’s not a regression?
DustinCampbell commentedon Jul 15, 2021
Yes, this is a regression from .NET Framework that occurred during some refactoring of
ReflectPropertyDescriptorin .NET Core. The NetFx code looks like this:Essentially, the NetFx version uses a field called
defaultValuethroughout. In .NET Core, this was refactored to lift the field to a local variable. However, one of the field references was missed, resulting in a null-ref:The fix is simple. Change
Enum.ToObject(PropertyType, _defaultValue)toEnum.ToObject(PropertyType, defaultValue).We didn't discover it until more recently because VB code is more likely to produce IL that triggers the exception, due to the fact that VB's implicit conversions and overload resolution is different from C#'s. There are more details about that here.
ericstj commentedon Jul 16, 2021
I see, it was regressed here between 1.1 and 2.0: dotnet/corefx@af3f34f#diff-4e980b410175c51d6330532288f63963ecd72513029c6954e5e862e7476e39c4R297. Seems like a simple refactoring bug (which we should have had tests to catch) cc @stephentoub
Thanks for the great report, it's clear now. I'll put up a fix. For things like this in the future you're welcome to submit a PR. You've done a ton of leg-work to get this far already and it might have even been less work to submit a PR than file the issue. There's probably a lot more expertise in WinForms on this codebase too. Thanks again for the report and a fix will go in shortly. Let us know if you need this ported to P7 or servicing.
KlausLoeffelmann commentedon Aug 4, 2021
@merriemcgaw - considering we are getting customer reports about this, should we consider servicing?
dotnet/winforms#2748
merriemcgaw commentedon Aug 17, 2021
If we're getting customer reports we should consider it. It doesn't seem to be a terribly edge case situation, but I'd leave the final call with @ericstj.
ericstj commentedon Aug 18, 2021
We don't proactively service and "getting customer reports" isn't sufficient for meeting the bug bar. Could you apply the bug bar? https://dev.azure.com/devdiv/DevDiv/_wiki/wikis/DevDiv.wiki/545/NET-Servicing?anchor=bug-bar