Allow DefaultValueAttribute with primitives when System.ComponentModel.DefaultValueAttribute.IsSupported disabled

[OwnageIsMagic]

Description

DefaultValueAttribute.Value throws on access (due to #100416) when System.ComponentModel.DefaultValueAttribute.IsSupported switch is false and it breaks XmlSerializer and other serializers/reflection. But it's very common to use not (Type, string) overload, but 1 arg primitive which do not use TypeDescriptor API family.

Reproduction Steps

using System.ComponentModel;
using System.Xml.Serialization;

// _ = typeof(C).GetProperty("i")!.GetCustomAttribute<DefaultValueAttribute>()!.Value;
XmlSerializer serializer = new(typeof(C));

public class C { [DefaultValue(5)] public int i {get; set;} }

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net9.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

  <ItemGroup>
    <RuntimeHostConfigurationOption
      Include="System.ComponentModel.DefaultValueAttribute.IsSupported"
      Value="false" Trim="true" />
  </ItemGroup>

</Project>

Expected behavior

No exception, use constructor arg as is.

Actual behavior

Unhandled exception. System.InvalidOperationException: There was an error reflecting type 'C'.
 ---> System.ArgumentException: Runtime instantiation of this attribute is not allowed.
   at System.ComponentModel.DefaultValueAttribute.get_Value()
   at System.Xml.Serialization.XmlAttributes..ctor(ICustomAttributeProvider provider)
   at System.Xml.Serialization.XmlReflectionImporter.GetAttributes(MemberInfo memberInfo)
   at System.Xml.Serialization.XmlReflectionImporter.InitializeStructMembers(StructMapping mapping, StructModel model, Boolean openModel, String typeName, RecursionLimiter limiter)
   at System.Xml.Serialization.XmlReflectionImporter.ImportStructLikeMapping(StructModel model, String ns, Boolean openModel, XmlAttributes a, RecursionLimiter limiter)
   at System.Xml.Serialization.XmlReflectionImporter.ImportTypeMapping(TypeModel model, String ns, ImportContext context, String dataType, XmlAttributes a, Boolean repeats, Boolean openModel, RecursionLimiter limiter)
   --- End of inner exception stack trace ---
   at System.Xml.Serialization.XmlReflectionImporter.ImportTypeMapping(TypeModel model, String ns, ImportContext context, String dataType, XmlAttributes a, Boolean repeats, Boolean openModel, RecursionLimiter limiter)
   at System.Xml.Serialization.XmlReflectionImporter.ImportElement(TypeModel model, XmlRootAttribute root, String defaultNamespace, RecursionLimiter limiter)
   at System.Xml.Serialization.XmlReflectionImporter.ImportTypeMapping(Type type, XmlRootAttribute root, String defaultNamespace)
   at System.Xml.Serialization.XmlSerializer..ctor(Type type, String defaultNamespace)
   at Program.<Main>$(String[] args) in C:\Source\ttt\Program.cs:line 4

Workaround

set _value to some sentinel if TypeDescriptor is required and throw only in this case.

[dotnet-policy-service]

Tagging subscribers to this area: @dotnet/area-system-componentmodel
See info in area-owners.md if you want to be subscribed.

[julealgon]

Wouldn't the flag name become incredibly misleading though? It clearly states that the entire attribute "should not be supported".

[OwnageIsMagic]

@julealgon it's not published in GA release, so can be renamed

[ericstj]

@LakshanF @steveharter please have a look

[LakshanF]

@OwnageIsMagic, thanks for the valuable feedback on the DefaultValueAttribute usage of primitive values that don't go through the TypeConverter route.

To help better understand your use case, could you give more details on your scenario please? The feature switch was added to help with trimming and non-trimming scenarios would have this feature set to true by default (and should not have any problems using DefaultValueAttribute).

[OwnageIsMagic]

@LakshanF I'm exploring aot for minimizing winforms app bundle and just disabled every runtime feature. Interface works ok, but XmlSerialzer is throwing this exception. Enabling it doesn't change exe size, so it seems like TypeDescriptor is trimmed anyway (didn't check), but disabling just make things bad without any benefit.

[steveharter]

Per offline discussion with @LakshanF, removing the check in the getter seems reasonable: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/ComponentModel/DefaultValueAttribute.cs#L248-L251.

However, we could just remove the check the (Type, string) ctor not the primitive ctors. This means adding a new flag to track that.

[OwnageIsMagic]

@steveharter no need for another flag, just rename this one to something like IsTypeConverterSupported.

private readonly static object? throwSentinel = IsSupported ? null : new();

public DefaultValueAttribute(Type type, string? value)
{
    if (type == null)
    {
        return;
    }

    try
    {
        if (type == typeof(TimeSpan) && value != null)
        {
            _value = TimeSpan.Parse(value);
        }
        else if (type.IsSubclassOf(typeof(Enum)) && value != null)
        {
            _value = Enum.Parse(type, value, true);
        }
        else if (type.IsAssignableFrom(typeof(IConvertible)))
        {
            try { _value = Convert.ChangeType(value, type, CultureInfo.InvariantCulture); }
            catch { }
        }
        // I reordered if blocks, can we allow this change?
        {
            Debug.Assert(IsSupported, "TODO");
            if (!IsSupported)
            {
                _value = throwSentinel;
                return;
            }

            _ = TryConvertFromInvariantString(type, value, out object? _value);

            static bool TryConvertFromInvariantString(...) { ... }
        }
    }
    catch
    {
    }
}

public virtual object? Value
{
    get
    {
        if (!IsSupported && _value == throwSentinel)
        {
            throw new ArgumentException(SR.RuntimeInstanceNotAllowed);
        }
        return _value;
    }
}