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
Merged

Add TypeConverter fallback to DefaultValueAttribute #19354

Changes from 8 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
50 changes: 45 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,9 @@
// 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.Reflection;
using System.Threading;

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

// We cache reflection types for TypeConverter conversion
static Func<MethodInfo> s_getConverterMethodCached;
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 would omit the Cached suffix.

static Func<MethodInfo> s_convertFromInvariantStringMethodCached;

/// <devdoc>
/// <para>Initializes a new instance of the <see cref='System.ComponentModel.DefaultValueAttribute'/> class, converting the
/// specified value to the
Expand All @@ -36,7 +38,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,6 +54,40 @@ public DefaultValueAttribute(Type type, string value)
{
_value = Convert.ChangeType(value, type, CultureInfo.InvariantCulture);
}

return;

// Try to load and use TypeConverter/TypeDescriptor types
bool TryConvertFromInvariantString(Type typeToConvert, string stringValue, out object conversionResult)
{
conversionResult = null;

// lazy init reflection objects
if (s_getConverterMethodCached == 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

Volatile.Write(ref s_getConverterMethodCached, () => typeDescriptorType?.GetMethod("GetConverter", new Type[] { typeof(Type) }));
Copy link
Member

Choose a reason for hiding this comment

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

Can we just cache the MethodInfos? Having to re-run typeDescriptorType?.GetMethod("GetConverter", new Type[] { typeof(Type) } every time the cache is accessed is not a great cache.

I would just have a static object s_getConverterMethodCached; field that is either null (which means we didn't populate it), or set to a MethodInfo (which means we populated the cache and succeeded), or something else (new object()?) (which means we tried to populate it, but couldn't find the method or type - and we should not try again). It's more lightweight than a lambda capture and a delegate.

Most importantly, it makes it easier for static analysis tools to analyze the reflection usage. IL Linkers will typically remove methods that don't appear used, and reflection usage is notoriously hard to statically analyze. Using patterns that are as straightforward as possible makes it less likely these things will get removed. The existing code is passing the type reflected on through a lambda capture and that's one of the harder things to analyze.

Copy link
Member Author

Choose a reason for hiding this comment

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

i followed this pattern, but there makes more sense Principal could change. Ok thank's for advice!

Copy link
Member

Choose a reason for hiding this comment

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

I do not think you should be caching MethodInfos here at all. It should be all just delegates (much faster in steady state than MethodInfo).

Copy link
Member Author

Choose a reason for hiding this comment

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

It should be all just delegates

could you elaborate? Do you mean use Delegate.Create?

Copy link
Member

Choose a reason for hiding this comment

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

Right, like in the Principal example.

Copy link
Member Author

@MarcoRossignoli MarcoRossignoli Aug 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

...
var converter = getConverter(typeToConvert);
Func<string, object> del = (Func<string, object>)Delegate.CreateDelegate(typeof(Func<string, object>), converter, "ConvertFromInvariantString");
...

@jkotas to call instance method ConvertFromInvariantString with delegate i need to pass concrete converter. What do you think?It's better than invoke(i don't have numbers)?Or we need to cache(key/value) ConvertFromInvariantString delegate? I think it's not correct to cache...are instances...i did a test and seems cached instance today...but could change in future

Copy link
Member

Choose a reason for hiding this comment

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

You should be able to create open delegate that takes this as argument.

Copy link
Member Author

@MarcoRossignoli MarcoRossignoli Aug 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@jkotas @ericstj i worked on open delegate(first time...very fun, if i could not eat to live i'd work 24/24 on this codebase for free, i'm learning a lot, exciting...thanks).
I cannot create Func<object,string,object> delegate because open delegate creation fail(this could not be object), so i used a on-the-fly generated generics. But now i cannot do a typed invocation but only dynamic one(cannot cast to Func<object,string,object>).
What do you think?

...
Volatile.Write(ref s_convertFromInvariantStringMethod, typeConverterType == null ? new object() : Delegate.CreateDelegate(typeof(Func<,,>).MakeGenericType(typeConverterType, typeof(string), typeof(object)), null, typeConverterType.GetMethod("ConvertFromInvariantString", new Type[] { typeof(string) })));
...
if (!(s_getConverterMethod is Func<Type, object> getConverter) || !(s_convertFromInvariantStringMethod is Delegate convertFromInvariantString))
      return false;

conversionResult = convertFromInvariantString.DynamicInvoke(getConverter(typeToConvert), stringValue);

return true;

Copy link
Member

@jkotas jkotas Aug 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I see. I think the root cause of them problem is that you cannot reference TypeConverter type in CoreLib.

I guess the options are to either give up on performance and just use MethodInfo; or add a helper method to System.ComponentModel.TypeConverter that does the whole conversion for you. It is https://github.com/dotnet/corefx/blob/b9d82ae7fd6912b6c0c758e47d0824689adaeafc/src/System.Runtime.Extensions/src/System/AppDomain.cs#L412 has done too - the GetDefaultInstance method is not part of the public surface, it is specific method for use by the AppDomain reflection (https://github.com/dotnet/corefx/blob/master/src/System.Security.Claims/src/System/Security/Claims/GenericPrincipal.cs#L88).

Copy link
Member Author

Choose a reason for hiding this comment

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

or add a helper method to System.ComponentModel.TypeConverter that does the whole conversion for you

I'll try this way!

}

if (s_convertFromInvariantStringMethodCached == null)
{
Type typeConverterType = Type.GetType("System.ComponentModel.TypeConverter, System, Version=0.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089", throwOnError: false);
Volatile.Write(ref s_convertFromInvariantStringMethodCached, () => typeConverterType?.GetMethod("ConvertFromInvariantString", new Type[] { typeof(string) }));
}

// use local var to avoid double delegate invocation
MethodInfo localGetConverterMethod = s_getConverterMethodCached();
MethodInfo localConvertFromInvariantStringMethod = s_convertFromInvariantStringMethodCached();

// if we didn't found required types on initialization fallback on old code
if (localGetConverterMethod == null || localConvertFromInvariantStringMethod == null)
return false;

// typeConverter cannot be null GetConverter return default converter
conversionResult = localConvertFromInvariantStringMethod.Invoke(localGetConverterMethod.Invoke(null, new[] { typeToConvert }), new[] { stringValue });

return true;
}
}
catch
{
Expand Down