Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DefaultValueAttribute doesn't use TypeConverter #26604

Closed
ericstj opened this issue Jun 25, 2018 · 7 comments · Fixed by dotnet/corefx#31649
Closed

DefaultValueAttribute doesn't use TypeConverter #26604

ericstj opened this issue Jun 25, 2018 · 7 comments · Fixed by dotnet/corefx#31649
Assignees
Milestone

Comments

@ericstj
Copy link
Member

ericstj commented Jun 25, 2018

In desktop DefaultValueAttribute would use TypeDesicriptor/TypeConverter to try and convert a string to a type: https://referencesource.microsoft.com/#System/compmod/system/componentmodel/DefaultValueAttribute.cs,b956ede3a6b29f1f,references

In Core we are not doing this: https://github.com/dotnet/coreclr/blob/85374ceaed177f71472cc4c23c69daf7402e5048/src/System.Private.CoreLib/shared/System/ComponentModel/DefaultValueAttribute.cs#L38-L51

We should add back the call to TypeConverter as an ultimate fallback, where we use lightup to load it. See https://devdiv.visualstudio.com/DevDiv/_git/DotNet-CoreCLR-Trusted/commit/90f7bf708e4a23f4f2096a847a820d75bf94aa2d?refName=refs%2Fheads%2Fericstj%2Fadd.back.exeperiment.

I suspect this will be needed for WinForms work. /cc @Tanya-Solyanik I ran into in some application code I was porting where the app was explicitly using DefaultValueAttribute itself.

@MarcoRossignoli
Copy link
Member

We should add back the call to TypeConverter as an ultimate fallback, where we use lightup to load it. See https://devdiv.visualstudio.com/DevDiv/_git/DotNet-CoreCLR-Trusted/commit/90f7bf708e4a23f4f2096a847a820d75bf94aa2d?refName=refs%2Fheads%2Fericstj%2Fadd.back.exeperiment.

could you "gist" this(link is internal)?I could help if i understand well. Do you mean add condition type.Module == typeof(string).Module to current fallback(else) and as new fallback use this.value = TypeDescriptor.GetConverter(type).ConvertFromInvariantString(value);?

@ericstj
Copy link
Member Author

ericstj commented Aug 7, 2018

Yes, that's what I was referring to, though we can't call typeconverter directly.

Here's what I did in that internal commit: ericstj/coreclr@17ac7be

@Anipik
Copy link
Contributor

Anipik commented Aug 7, 2018

@MarcoRossignoli are you planning to work on this ?

@MarcoRossignoli
Copy link
Member

@Anipik if It isn't too urgent i could try a PR!Feel free to work on it if you have to!

@Anipik
Copy link
Contributor

Anipik commented Aug 7, 2018

No, it`s not urgent. Feel free to give it a try.

@MarcoRossignoli
Copy link
Member

Yes, that's what I was referring to, though we can't call typeconverter directly.

Only to be sure to understand...because TypeDescriptor/TypeConverter are on corefx codebase?If so do we need to move "classes" to coreclr, in past i asked and answer was "we move to coreclr if we need".

/cc @jkotas

@jkotas
Copy link
Member

jkotas commented Aug 8, 2018

If so do we need to move "classes" to coreclr, in past i asked and answer was "we move to coreclr if we need"

There are exceptions to this rule when the the closure of types that would need to be moved is large like in this case. We make the call using reflection like in the Eric's prototype.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants