Add TypeConverter fallback to DefaultValueAttribute #19354

MarcoRossignoli · 2018-08-08T13:48:50Z

contributes to https://github.com/dotnet/corefx/issues/30648

jkotas · 2018-08-08T14:59:26Z

src/System.Private.CoreLib/shared/System/ComponentModel/DefaultValueAttribute.cs

+        static DefaultValueAttribute()
+        {
+            // We discover/cache types for conversion fallback on first load
+            s_typeDescriptorTypeCached = Type.GetType("System.ComponentModel.TypeDescriptor, System, Version=0.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089", throwOnError: false);


This should not be in static constructor. This should be done as lazy as possible only when it is needed.

could i use Lazy<T>/LazyInitializer.EnsureInitialized or usually there are other pattern on codebase? I mean i could simply check null and pay possible benign multiple instances on race to keep code simpler as possible...could be ok?

Here is the pattern to follow: https://github.com/dotnet/corefx/blob/b9d82ae7fd6912b6c0c758e47d0824689adaeafc/src/System.Runtime.Extensions/src/System/AppDomain.cs#L409

jkotas · 2018-08-08T15:19:49Z

src/System.Private.CoreLib/shared/System/ComponentModel/DefaultValueAttribute.cs

@@ -44,14 +56,32 @@ public DefaultValueAttribute(Type type, string value)
                {
                    _value = TimeSpan.Parse(value);
                }
-                else
+                else if (type.Module == typeof(string).Module)


This condition looks suspect. There are intrinsic type converters for many random types in CoreLib: https://github.com/dotnet/corefx/blob/e0ba7aa8026280ee3571179cc06431baf1dfaaac/src/System.ComponentModel.TypeConverter/src/System/ComponentModel/ReflectTypeDescriptionProvider.cs#L103

Should we just always use the TypeConverters for everything when they are available?

i think that the idea comes from https://referencesource.microsoft.com/#System/compmod/system/componentmodel/DefaultValueAttribute.cs,b956ede3a6b29f1f,references same "intrinsics" are present also on .net full? @ericstj

Should we just always use the TypeConverters for everything when they are available?

If custom converter is not defined GetConverter return a "default type converter". Do you mean check if return something different than "default type converter" and in that case fallback to _value = Convert.ChangeType(value, type, CultureInfo.InvariantCulture); ?

I think the upstack type converters should take care of all conversions. I do not think we need any fallbacks once the upstack type converter is called.

If type converter is present, use it. If not, use the old code. There are still cases where folks won't have TypeConverter in their app (eg: trimmed selfcontained app) and we shouldn't break those.

If type converter is present, use it

As i said above if converter is not present we get default type converter(not null). Do you mean that if GetConverter return something different than default right?

I mean that if the TypeConverter assembly itself is missing (your reflection lookups fail) use the old logic.

Ah ok trimmed assemblies understood thank's!

ericstj · 2018-08-08T17:14:32Z

src/System.Private.CoreLib/shared/System/ComponentModel/DefaultValueAttribute.cs

            }
            catch
            {
            }
+
+            object ConvertFromInvariantString(Type typeToConvert, string stringValue)


nit: missing private

It's a local func...if i'm not mistaken it's not possibile add access modifier

I see, totally missed the placement of the closing brace. Not sure I've seen any usage of local functions yet in our codebase and so far our style guidelines are mum on it. @jkotas do you have any opinion on style guidelines for local functions? Normally I'd ask @stephentoub @danmosemsft @weshaggard but they're all out.

Just some initial thoughts on style:
place local functions at the end of the parent member and an explicit return before the local function to make it visually clear where the parent member ends.
include a comment above the local function.

I used a local func because we are replacing simple TypeConverter call(with a lot of code) and it's used only in this method(ctor), to me it's more clear...but let me know if i need to tranform to normal private method!

Not sure I've seen any usage of local functions yet in our codebase

I know people have been adding them here and there for a while. Here is an example:
https://github.com/dotnet/corefx/pull/24389/files#diff-8df21afb33a350b9ca945d04a5296b9bR119. The style is about what described.

ericstj · 2018-08-08T17:24:45Z

src/System.Private.CoreLib/shared/System/ComponentModel/DefaultValueAttribute.cs

+                if (localTypeDescriptor == null || localConvertFromInvariantStringMethod == null)
+                    return null;
+
+                MethodInfo typeDescriptorTypeGetConverter = localTypeDescriptor.GetMethod("GetConverter", new Type[] { typeToConvert });


You don't need to call this every time, cache it as with other reflection. TypeDescriptor.GetConverter signature does not change based on typeToConvert.

You'are right! Thank's!

jkotas · 2018-08-09T13:47:37Z

src/System.Private.CoreLib/shared/System/ComponentModel/DefaultValueAttribute.cs

@@ -23,6 +21,10 @@ public class DefaultValueAttribute : Attribute
        /// </devdoc>
        private object _value;

+        // We cache reflection types for TypeConverter conversion
+        static Func<MethodInfo> s_getConverterMethodCached;


Nit: I would omit the Cached suffix.

MichalStrehovsky · 2018-08-09T13:48:35Z

src/System.Private.CoreLib/shared/System/ComponentModel/DefaultValueAttribute.cs

+                    if (s_getConverterMethodCached == null)
+                    {
+                        Type typeDescriptorType = Type.GetType("System.ComponentModel.TypeDescriptor, System, Version=0.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089", throwOnError: false);
+                        Volatile.Write(ref s_getConverterMethodCached, () => typeDescriptorType?.GetMethod("GetConverter", new Type[] { typeof(Type) }));


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.

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

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).

It should be all just delegates

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

Right, like in the Principal example.

... 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

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

@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;

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).

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

I'll try this way!

MarcoRossignoli · 2018-08-11T10:06:30Z

src/System.Private.CoreLib/shared/System/ComponentModel/DefaultValueAttribute.cs

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


@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?

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.

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)); }

jkotas · 2018-08-11T14:08:06Z

src/System.Private.CoreLib/shared/System/ComponentModel/DefaultValueAttribute.cs

-        // We cache reflection types for TypeConverter conversion
-        static Func<MethodInfo> s_getConverterMethodCached;
-        static Func<MethodInfo> s_convertFromInvariantStringMethodCached;
+        // Delegate 'TypeDescriptor.InternalConvertFromInvariantString' reflection object cache


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

jkotas · 2018-08-11T16:04:22Z

src/System.Private.CoreLib/shared/System/ComponentModel/DefaultValueAttribute.cs

+                    // 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);


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

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?

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.

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

jkotas · 2018-08-13T13:39:15Z

The existing DefaultValueAttributeTests CoreFX test is failing:

   System.ComponentModel.Tests.DefaultValueAttributeTests.Ctor [FAIL]
Assert.Equal() Failure
Expected: Monday
Actual:   (null)
Stack Trace:
/home/aand/repositories/corefx/src/System.Runtime/tests/System/ComponentModel/DefaultValueAttributeTests.cs(32,0): at System.ComponentModel.Tests.DefaultValueAttributeTests.Ctor()

It sounds like that fallback path has been handling more cases than the TypeConverter path. Ugh. Maybe we need to move the TypeConverter path later to handle the existing cases.

MarcoRossignoli · 2018-08-13T14:52:09Z

@jkotas i think it's correct...we removed throwOnBindFailure the current CoreFx build doesn't have new custom helper...so bind throw exception "This method overload is equivalent to calling the CreateDelegate(Type, Type, String, Boolean, Boolean) method overload, specifying true for throwOnBindFailure."
Throw exception goes to empty catch and so null value. With throwOnBindFailure = false we got new object for s_convertFromInvariantString with correct return false and fallback.
The key is you assert we should expect that the helper method exists too, but now doesn't exist. Does my suspect makes sense?
In my local cross repo build/test works well.

EDIT: I think it's not so easy...but could be "super" run CI with a CoreFx picked up from PR @dotnet-bot test Windows_NT x64 Release CoreFX Tests PR12345.

jkotas · 2018-08-13T15:20:37Z

I see. Could you please split your CoreFX to two: One that adds the helper method and second with the test? We will merge the helper method first, then the CoreCLR change, and the tests the last. It will allow us to get the change through without having failing tests.

MarcoRossignoli · 2018-08-14T08:41:02Z

@dotnet-bot test this please

MarcoRossignoli · 2018-08-14T09:18:52Z

@jkotas i re-run build...CoreFx tests leg pick up "last commit" repo or there are a different workflow(maybe tell to ci to use new commits)?

jkotas · 2018-08-14T12:25:39Z

#19377 needs to go through to pick up updated CoreFX.

MarcoRossignoli · 2018-08-14T12:54:18Z

needs to go through to pick up updated CoreFX.

Ok so for now stand-by.

jkotas · 2018-08-14T15:08:36Z

@dotnet-bot test this please

jkotas · 2018-08-14T15:10:11Z

src/System.Private.CoreLib/shared/System/ComponentModel/DefaultValueAttribute.cs

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


Nit: The other places where we do this omit the Version=0.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 part.

jkotas · 2018-08-14T15:10:31Z

src/System.Private.CoreLib/shared/System/ComponentModel/DefaultValueAttribute.cs

Nit: Unnecessary new line?

jkotas · 2018-08-14T17:00:17Z

@dotnet-bot test OSX10.12 x64 Checked Innerloop Build and Test please

…9354) Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

* Add TypeConverter fallback to DefaultValueAttribute (dotnet/coreclr#19354) Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com> * Workaround for mcs * Use mcs specific define

add TypeConverter fallback

aca9eb3

MarcoRossignoli mentioned this pull request Aug 8, 2018

Add TypeConverter fallback to DefaultValueAttribute tests dotnet/corefx#31649

Merged

jkotas reviewed Aug 8, 2018

View reviewed changes

MarcoRossignoli added 2 commits August 8, 2018 18:04

add lazy init for reflection objects

921b2f5

Use null propagation

e2bce7b

ericstj reviewed Aug 8, 2018

View reviewed changes

MarcoRossignoli added 5 commits August 9, 2018 12:38

address PR feedback

c996bfe

nit: cleanup code

304df3e

fix GetConverter reflection lookup

3909a3c

nit: fix comment

5075ea8

nit: rename local var

5d63db2

jkotas reviewed Aug 9, 2018

View reviewed changes

MichalStrehovsky reviewed Aug 9, 2018

View reviewed changes

use new helper on System.ComponentModel.TypeDescriptor

aee210a

MarcoRossignoli commented Aug 11, 2018

View reviewed changes

jkotas reviewed Aug 11, 2018

View reviewed changes

MarcoRossignoli added 2 commits August 11, 2018 17:16

address PR feedback

8cae155

address PR feedback

3390620

jkotas reviewed Aug 11, 2018

View reviewed changes

address PR feedback

ea79dac

MarcoRossignoli mentioned this pull request Aug 13, 2018

Add CoreLib helper used by DefaultValueAttribute constructor dotnet/corefx#31739

Merged

jkotas reviewed Aug 14, 2018

View reviewed changes

address PR feedback

65cb4aa

jkotas merged commit 431d041 into dotnet:master Aug 15, 2018

dotnet-maestro-bot pushed a commit to dotnet-maestro-bot/corefx that referenced this pull request Aug 15, 2018

Add TypeConverter fallback to DefaultValueAttribute (dotnet/coreclr#1…

e264ea3

…9354) Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

dotnet-maestro-bot pushed a commit to dotnet-maestro-bot/corert that referenced this pull request Aug 15, 2018

Add TypeConverter fallback to DefaultValueAttribute (dotnet/coreclr#1…

f70c9b4

…9354) Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

jkotas pushed a commit to dotnet/corert that referenced this pull request Aug 15, 2018

Add TypeConverter fallback to DefaultValueAttribute (dotnet/coreclr#1…

1271b47

…9354) Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

jkotas pushed a commit to dotnet/corefx that referenced this pull request Aug 15, 2018

Add TypeConverter fallback to DefaultValueAttribute (dotnet/coreclr#1…

2f86ad4

…9354) Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

MarcoRossignoli deleted the defaultvalueattributetypeconverter branch August 15, 2018 07:16

joperezr mentioned this pull request Nov 30, 2018

Some driver cleanup dotnet/iot#56

Merged

MaximLipnin pushed a commit to MaximLipnin/corefx that referenced this pull request Jan 14, 2019

Add TypeConverter fallback to DefaultValueAttribute (dotnet/coreclr#1…

91a6e63

…9354) Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

MaximLipnin pushed a commit to MaximLipnin/corefx that referenced this pull request Jan 14, 2019

Add TypeConverter fallback to DefaultValueAttribute (dotnet/coreclr#1…

5c323de

…9354) Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

lewurm mentioned this pull request Feb 1, 2019

[2018-08] Bump corert mono/mono#12721

Merged

Add TypeConverter fallback to DefaultValueAttribute #19354

Add TypeConverter fallback to DefaultValueAttribute #19354

Conversation

MarcoRossignoli commented Aug 8, 2018

Choose a reason for hiding this comment

MarcoRossignoli Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

MarcoRossignoli Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoRossignoli Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoRossignoli Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcoRossignoli Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

jkotas Aug 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Aug 11, 2018 • edited Loading

Choose a reason for hiding this comment

MarcoRossignoli Aug 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Aug 13, 2018

MarcoRossignoli commented Aug 13, 2018 • edited Loading

jkotas commented Aug 13, 2018

MarcoRossignoli commented Aug 14, 2018

MarcoRossignoli commented Aug 14, 2018

jkotas commented Aug 14, 2018

MarcoRossignoli commented Aug 14, 2018

jkotas commented Aug 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas commented Aug 14, 2018

MarcoRossignoli Aug 8, 2018 •

edited

Loading

jkotas Aug 8, 2018 •

edited

Loading

MarcoRossignoli Aug 8, 2018 •

edited

Loading

MarcoRossignoli Aug 8, 2018 •

edited

Loading

MarcoRossignoli Aug 9, 2018 •

edited

Loading

MarcoRossignoli Aug 10, 2018 •

edited

Loading

jkotas Aug 10, 2018 •

edited

Loading

jkotas Aug 11, 2018 •

edited

Loading

MarcoRossignoli Aug 11, 2018 •

edited

Loading

MarcoRossignoli commented Aug 13, 2018 •

edited

Loading