Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Add TypeConverter fallback to DefaultValueAttribute #19354

Merged
merged 13 commits into from
Aug 15, 2018
+31 −5
Merged

Add TypeConverter fallback to DefaultValueAttribute #19354

Changes from 9 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
aca9eb3
add TypeConverter fallback
MarcoRossignoli Aug 8, 2018
921b2f5
add lazy init for reflection objects
MarcoRossignoli Aug 8, 2018
e2bce7b
Use null propagation
MarcoRossignoli Aug 8, 2018
c996bfe
address PR feedback
MarcoRossignoli Aug 9, 2018
304df3e
nit: cleanup code
MarcoRossignoli Aug 9, 2018
3909a3c
fix GetConverter reflection lookup
MarcoRossignoli Aug 9, 2018
5075ea8
nit: fix comment
MarcoRossignoli Aug 9, 2018
5d63db2
nit: rename local var
MarcoRossignoli Aug 9, 2018
aee210a
use new helper on System.ComponentModel.TypeDescriptor
MarcoRossignoli Aug 11, 2018
8cae155
address PR feedback
MarcoRossignoli Aug 11, 2018
3390620
address PR feedback
MarcoRossignoli Aug 11, 2018
ea79dac
address PR feedback
MarcoRossignoli Aug 13, 2018
65cb4aa
address PR feedback
MarcoRossignoli Aug 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 39 additions & 5 deletions src/System.Private.CoreLib/shared/System/ComponentModel/DefaultValueAttribute.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,8 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.ComponentModel;
using System.Diagnostics;
using System.Globalization;
using System.Runtime.InteropServices;
using System.Threading;

namespace System.ComponentModel
{
Expand All @@ -23,6 +20,9 @@ public class DefaultValueAttribute : Attribute
/// </devdoc>
private object _value;

// Delegate 'TypeDescriptor.InternalConvertFromInvariantString' reflection object cache
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think we can call the method just ConvertFromInvariantString and drop the internal from all the names.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

static object s_convertFromInvariantString;

/// <devdoc>
/// <para>Initializes a new instance of the <see cref='System.ComponentModel.DefaultValueAttribute'/> class, converting the
/// specified value to the
Expand All @@ -36,7 +36,11 @@ public DefaultValueAttribute(Type type, string value)
// load an otherwise normal class.
try
{
if (type.IsSubclassOf(typeof(Enum)))
if (TryConvertFromInvariantString(type, value, out object convertedValue))
{
_value = convertedValue;
}
else if (type.IsSubclassOf(typeof(Enum)))
{
_value = Enum.Parse(type, value, true);
}
Expand All @@ -48,9 +52,39 @@ public DefaultValueAttribute(Type type, string value)
{
_value = Convert.ChangeType(value, type, CultureInfo.InvariantCulture);
}

return;

// Looking for object TypeDescriptor.InternalConvertFromInvariantString(Type, string) and call to conversion function ConvertFromInvariantString
bool TryConvertFromInvariantString(Type typeToConvert, string stringValue, out object conversionResult)
{
conversionResult = null;

// lazy init reflection objects
if (s_convertFromInvariantString == null)
{
Type typeDescriptorType = Type.GetType("System.ComponentModel.TypeDescriptor, System, Version=0.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089", throwOnError: false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should load the type from System.ComponentModel.TypeConverter directly, not from System facade.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should load the type from System.ComponentModel.TypeConverter directly, not from System facade.

Because we'll know if TypeConverter exists when we call conversion method and so check could pass but fail after?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

System.dll is .NET Framework compatibility shim. It should not be loaded or pulled unless really necessary

System.ComponentModel.TypeConverter is the native .NET Core implementation assembly for this, so it is where we should be loading this from.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh i misread...understood i used System...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


Delegate convertFromInvariantString = null;
if (typeDescriptorType != null)
{
convertFromInvariantString = Delegate.CreateDelegate(typeof(Func<Type, string, object>), typeDescriptorType, "InternalConvertFromInvariantString", ignoreCase: false, throwOnBindFailure: false);
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkotas @ericstj added this check to be sure that helper method exists on "corelib users". I'm not sure if this makes sense, is corefx the only "user" of coreclr today and in future?

Copy link
Member

@jkotas jkotas Aug 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (typeDescriptorType != null) check is fine. It is the whole point of the fallback path.

For Delegate.CreateDelegate, I do not think we need throwOnBindFailure: false. If System.ComponentModel.TypeDescriptor exists, we should expect that the helper method exists too.

Copy link
Member Author

@MarcoRossignoli MarcoRossignoli Aug 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should expect that the helper method exists too.

My fault i wasn't clear, but you answered! If i remove throwOnBindFailure: false i can collapse all on assignment...if InternalConvertFromInvariantString will miss throw exception, and no fallback...Value will be null.

// lazy init reflection objects
if (s_convertFromInvariantString == null)
{
   Type typeDescriptorType = Type.GetType("System.ComponentModel.TypeDescriptor, System, Version=0.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089", throwOnError: false);
   Volatile.Write(ref s_convertFromInvariantString, typeDescriptorType == null ? new object() : Delegate.CreateDelegate(typeof(Func<Type, string, object>), typeDescriptorType, "InternalConvertFromInvariantString", ignoreCase: false));
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


Volatile.Write(ref s_convertFromInvariantString, convertFromInvariantString ?? new object());
}

if (!(s_convertFromInvariantString is Func<Type, string, object> internalConvertFromInvariantString))
return false;

conversionResult = internalConvertFromInvariantString(typeToConvert, stringValue);

return true;
}
}
catch
{

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Unnecessary new line?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}
}

Expand Down