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

Convert.ChangeType does not work with DateOnly #73074

Closed
MgSam opened this issue Jul 29, 2022 · 17 comments
Closed

Convert.ChangeType does not work with DateOnly #73074

MgSam opened this issue Jul 29, 2022 · 17 comments
Labels
area-System.DateTime documentation Documentation bug or enhancement, does not impact product or test code
Milestone

Comments

@MgSam
Copy link

MgSam commented Jul 29, 2022

Description

System.Convert.ChangeType does not work converting System.String to System.DateOnly. I would expect it to work given that it does work for System.String to System.DateTime.

Reproduction Steps

Convert.ChangeType("1/1/2000", typeof(DateTime)); //Works
Convert.ChangeType("1/1/2000", typeof(DateOnly)); //InvalidCastException

Expected behavior

System.Convert.ChangeType should work for String -> DateOnly

Actual behavior

System.Convert.ChangeType does not work for String -> DateOnly

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jul 29, 2022
@ghost
Copy link

ghost commented Jul 29, 2022

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

Issue Details

Description

System.Convert.ChangeType does not work converting System.String to System.DateOnly. I would expect it to work given that it does work for System.String to System.DateTime.

Reproduction Steps

Convert.ChangeType("1/1/2000", typeof(DateTime)); //Works
Convert.ChangeType("1/1/2000", typeof(DateOnly)); //InvalidCastException

Expected behavior

System.Convert.ChangeType should work for String -> DateOnly

Actual behavior

System.Convert.ChangeType does not work for String -> DateOnly

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

Author: MgSam
Assignees: -
Labels:

area-System.Runtime, untriaged

Milestone: -

@MichalPetryka
Copy link
Contributor

Convert.ChangeType is a legacy API that generally shouldn't be used and this is caused by DateOnly not being in the TypeCode enum.

@MgSam
Copy link
Author

MgSam commented Jul 30, 2022

Convert.ChangeType is a legacy API that generally shouldn't be used and this is caused by DateOnly not being in the TypeCode enum.

Why is it "legacy"? And what is the replacement? Why not put DateOnly into TypeCode?

@tannergooding
Copy link
Member

Convert is slow, not type safe, is lacking quite a bit of "key" functionality, and is not easily extensible to new types as much of it functions on or around TypeCode.

For converting between number types, .NET 7 exposes the INumberBase<T>.CreateChecked, CreateSaturating, and CreateTruncating APIs.

For converting between a string and a type, you have the IParsable (.NET 7) and IFormattable (.NET 1.0) APIs.

When you directly know the types you're converting between, using the concrete type is almost always a better option (DateOnly.Parse in this case). When you're working with unknown types, the new interfaces in .NET 7 provide a fast, safe, and extensible way to expose the functionality to developers.

@MgSam
Copy link
Author

MgSam commented Aug 1, 2022

Convert is slow, not type safe, is lacking quite a bit of "key" functionality, and is not easily extensible to new types as much of it functions on or around TypeCode.

I agree it's not type safe. That's kind of the whole point of the API :) The types are only known at runtime. Yes, it's slower but that's a price you sometimes have to pay when you're working with purely dynamic data.

I disagree that it's not extensible. I looked at the implementation and it looks at whether the type is IConvertible. DateOnly is not IConvertible for whatever reason. You don't actually even need the DateOnly in the TypeCode enum for it to work here.

For converting between number types, .NET 7 exposes the INumberBase<T>.CreateChecked, CreateSaturating, and CreateTruncating APIs.

For converting between a string and a type, you have the IParsable (.NET 7) and IFormattable (.NET 1.0) APIs.

When you directly know the types you're converting between, using the concrete type is almost always a better option (DateOnly.Parse in this case). When you're working with unknown types, the new interfaces in .NET 7 provide a fast, safe, and extensible way to expose the functionality to developers.

I'm still not clear how any of that is equivalent to converting from one Type to another Type, where both types are only known at runtime. Seems like you're assuming we always want to convert from strings, which is definitely not the case.

In short, I think this API exists for a reason, and unless you guys offer a real replacement for converting from Type to Type, I think it should continue to work for the new "primitive" types being added (like DateOnly). If you guys want to to not use IConvertible or TypeCode, I think that's an implementation detail- but the need for this particular API is as real today as when it was created 20 years ago.

@tannergooding
Copy link
Member

tannergooding commented Aug 1, 2022

In short, I think this API exists for a reason, and unless you guys offer a real replacement for converting from Type to Type, I think it should continue to work for the new "primitive" types being added (like DateOnly)

It is not the responsibility of the BCL to provide "everything" and it is impossible for us to do so. It is just as much the responsibility of consumers to fill the gap where they have needs by introducing their own wrappers or additional functionality where required.

We do provide APIs where they are warranted and where they have a concrete need. This is not one of those cases and is likely the type of API we would not expose or would expose significantly differently if the framework were redesigned from scratch (without backcompat concerns).

IConvertible and the Convert class both have real limitations and high back-compat concerns. There are a plethora of "core" types that don't support (including types that were in .NET 1.0) and the interface/enum hasn't been versioned for any of them. DateOnly and TimeOnly are no exception here and so just like you would have to do with Half, Int128, UInt128, Guid, TimeSpan, nullables, or any of the other "core" types you will need to fill the gap here yourself.

DateOnly is not IConvertible for whatever reason

It is not IConvertible because IConvertible is not extensible. The interface is designed around TypeCode which is very limited and basically only supports the "core" types that VB and C# had as "primitives" in .NET 1.0. Modifying the TypeCode enum has a very high risk of breaking existing "core" code for third party libraries and additionally requires that the supported types exist in S.P.Corelib itself.

In particular the existing DateTime is only convertible to itself, string, and object. Every other IConvertible and Convert.* API throws for DateTime. If you need/have an object, then casting to object directly is better. It will be a nop if you already have a reference type and a box/unbox otherwise. If you need/have a string, then the format/parse APIs are better.

@MgSam
Copy link
Author

MgSam commented Aug 1, 2022

I agree, the TypeCode enum and IConvertible have limitations. So make an implementation that doesn't use them. The signature is Convert.ChangeType(object value, Type conversionType). There is nothing about that signature that implies nor requires any particular implementation. It could just as easily do something entirely different using is.

In particular the existing DateTime is only convertible to itself, string, and object. Every other IConvertible and Convert.* API throws for DateTime. If you need/have an object, then casting to object directly is better. It will be a nop if you already have a reference type and a box/unbox otherwise. If you need/have a string, then the format/parse APIs are better.

I think you're misunderstanding a lot of the usage of these APIs. They usually go hand-in-hand with ADO.NET and its successors. Dynamically reading/writing data from a database what could be any primitive type and need to get converted to other primitive types on its journey back and forth (where such conversions make sense). No one is expecting it to miraculously convert DateOnly into a double somehow.

I don't think asking Microsoft to make BCL methods that already exist add support for new "primitive" types that Microsoft is now shipping to be particularly onerous or unfair. Obviously you cannot ship baked-in support for 3rd party types you don't know about.

More broadly, I think your team should be more cautious about "deprecating" APIs that are still shipping and still in heavy use without having any clear replacement. (DataTable comes to mind as another obvious example. The team refuses to improve it- yet there is no real alternative- the DataFrame project has been more-or-less abandoned.)

@tannergooding
Copy link
Member

tannergooding commented Aug 1, 2022

My point remains that there are two scenarios that IConvertible supports:

  1. Conversion to/from string
  2. Conversion between primitive number types

For 1, the parsing and formatting APIs are better. For 2, the new number create APIs are better. Both of these approaches also work well with trimming, AOT, and scenarios where reflection is limited or unavailable.

If you wanted to support a generic conversion API using a truly arbitrary object and Type, you have the option of writing a small reflection based wrapper that invokes the relevant IParsable<T>.Parse or INumberBase<T>.Create* APIs. This will end up being, especially with caching of the resolve MethodInfo, faster and will support more conversion scenarios.

@jeffhandley jeffhandley removed the untriaged New issue has not been triaged by the area owner label Aug 2, 2022
@jeffhandley jeffhandley added this to the Future milestone Aug 2, 2022
@TomaszWegrzyn
Copy link

Convert.ChangeType is a legacy API that generally shouldn't be used and this is caused by DateOnly not being in the TypeCode enum.

This is kind of a sidenote, but I would prefer to find this information in docs instead random comment on github
Wish my team had known this earlier, before we started using it.

@jkotas jkotas added the documentation Documentation bug or enhancement, does not impact product or test code label Aug 10, 2022
@tannergooding
Copy link
Member

tannergooding commented Aug 10, 2022

Noting there is a difference between something being deprecated/obsolete and something which isn't going to see new features added in the future.

Convert is and always has been only usable with a small subset of types overall, even when looking at the "built-in" types for .NET. Due to the way it was originally designed and how long its been around, there are issues adding support for new types without breaking devs. It will continue existing and will likely never be deprecated/obsoleted. There is no fundamental "need" to move off the API for code that already works.

If you're targeting something before .NET 7 (including .NET Framework/.NET Standard) then using it for the types it already and has always supported is and continues to be fine (and likely one of the simpler choices). Once you're on .NET 7, you have a new set of APIs that can provide similar functionality but in an way that makes it overall faster, less error-prone, and which can be extended to support new types over time. This allows users who are interested in such functionality to extend themselves to support such scenarios.

@dakersnar
Copy link
Contributor

Explanation for closing was given above.

@MgSam
Copy link
Author

MgSam commented Nov 10, 2022

Sorry I dropped off this thread. @tannergooding I think you're still missing the point and not addressing the core concerns.

ADO.NET simply does not work with DateOnly. It deeply relies on IConvertible. Are you guys going to replace or substantially rewrite ADO.NET? We both know the answer to that is almost certainly no as its barely changed in the past 20 years. The fact is you are today shipping core types that your own database tech stack cannot handle. DateOnly may have been added in .NET 6 but I think most people agree it should have been there in .NET Framework 1.0 as it is directly analogous to the date type in databases.

You allude to "issues" with adding new types into IConvertible. What are the issues? Wasn't that the entire point of adding default interface methods into C#- to allow evolving these ancient interfaces?

@tannergooding
Copy link
Member

It has always been the case that IConvertible does not support numerous types throughout the ecosystem, "core" or otherwise.

Any requests that another tech stack support new types or update to support more modern APIs should be filed against that repo or in the respect feedback location for that product.

While DIMs were added to allow versioning, that is not a free pass to version or extend things however we can imagine. We still have to do so with the consideration of broader impact including conflicts with other interfaces, other DIMs, diamond problems, intended usage of a type, etc.

IConvertible is not an interface we are looking at versioning moving forward due to the reasons given earlier.

@MgSam
Copy link
Author

MgSam commented Nov 10, 2022

It has always been the case that IConvertible does not support numerous types throughout the ecosystem, "core" or otherwise.

Your reasoning is circular- "It is this way because it has always been this way". You could make literally the same argument about any feature you guys have ever implemented. "There is no DateOnly type because it has always been the case that we don't add every conceivable type into the BCL".

DateOnly has been added to the BCL. It is massively useful. It is a type as fundamental as int or string. (Any type that makes it over the extremely high bar to being a native type in SQL you can bet is pretty core) Yet it isn't supported in core parts of the BCL which you yourself have said are not deprecated.

Any requests that another tech stack support new types or update to support more modern APIs should be filed against that repo or in the respect feedback location for that product.

That's a bit of a cop out isn't it? ADO.NET isn't "another" tech stack. It is and always has been part of the BCL. System.Data has shipped with .NET since 2002.

While DIMs were added to allow versioning, that is not a free pass to version or extend things however we can imagine. We still have to do so with the consideration of broader impact including conflicts with other interfaces, other DIMs, diamond problems, intended usage of a type, etc.

So finally we get to the real reason- you don't believe it is worth the effort.

I would be perfectly happy if an alternative method to Convert.ChangeType was supplied and IConvertible usage was removed across the BCL. But right now we're in the worst of both worlds- there is no plan or proposal that I'm aware of to replace this API and related friends in ADO.NET with something that supports DateOnly, and there is no plan to update the existing legacy APIs. So instead an important section of the BCL will continue to ship, yet remain frozen in time with increasingly outdated type support like the worst echoes of VBA or Microsoft Access.

@tannergooding
Copy link
Member

Your reasoning is circular- "It is this way because it has always been this way".

It's not circular. It's stating a fact on the type. IConvertible has never been a type to support all the "core" types. It doesn't support fairly crucial types that have existed since v1.0 like TimeSpan, Guid, IntPtr, UIntPtr, or DateTimeOffset. At least one of which does have direct ADO.NET support.

You could make literally the same argument about any feature you guys have ever implemented. "There is no DateOnly type because it has always been the case that we don't add every conceivable type into the BCL".

Yes, and API requests frequently get closed as being not in scope for the BCL. It is not the job of the BCL to provide everything. Each feature and change we add has to be carefully considered. This requires taking many factors into consideration including overall number of requests, complexity, potential impact (both positive and negative), etc.

DateOnly has been added to the BCL. It is massively useful. It is a type as fundamental as int or string. (Any type that makes it over the extremely high bar to being a native type in SQL you can bet is pretty core) Yet it isn't supported in core parts of the BCL which you yourself have said are not deprecated.

As are other types like Half, Int128, UInt128, and TimeOnly. The same could also be said for types like BigInteger. The first three I listed are also more fundamental than int. They are ABI primitives and require fundamental support by the BCL/runtime to be correctly implemented. An end user could never provide such a type. A 3rd party user could have provided DateOnly or TimeOnly, however.

The support for each of these is integrated throughout the BCL, as needed and as appropriate.

That's a bit of a cop out isn't it? ADO.NET isn't "another" tech stack. It is and always has been part of the BCL. System.Data has shipped with .NET since 2002.

It being in dotnet/runtime nor it being provided "in box" by default makes it part of the BCL. System.Data is owned and maintained by a team outside the main libraries team and any new features for it need to be raised to them and in their area.

So finally we get to the real reason- you don't believe it is worth the effort.

Not everything is worth the effort while others are worth having but can't be easily exposed due to concrete limitations or high negative impact.

An example of the latter is we might like to have IEnumerable<T> implement IEnumerable.GetEnumerator as a DIM. The practical thing is that we likely can't because it will introduce real world diamond problems for developers on a core type that practically every application in the world users at some layer.

In the case of IConvertible, its a type that:

  • has been around since .NET 1.0 and that means the risk of versioning it can be extremely high.
  • relies on TypeCode, which is not trivially versionable.
  • the general format of the interface is setup in a way that cannot play well with AOT scenarios.
  • versioning it using the one extensible API (ToType) has a high risk of breaking existing users overriding the method (as many methods explicitly check a subset and fallback to throw new InvalidCastException).
  • exposing new members, like ToDateOnly via DIMs would likewise require them to throw new InvalidCastException by default, greatly hindering usability
  • there is no trivial way to check if a type conversion is supported
  • etc

For number to number conversions, we now have a better more general purpose mechanism.

For type to string and string to type conversions, we have IFormattable and IParsable which are overall better and more performant

For other conversions, IConvertible effectively only allowed conversions between the T and object -- this is how DateTime is implemented. Literally everything except for ToDateTime and ToType where the target type was string or object. In such a scenario, you want to use IFormattable/IParsable or simple box/unbox.

@Wraith2
Copy link
Contributor

Wraith2 commented Nov 10, 2022

It is a type as fundamental as int or string. (Any type that makes it over the extremely high bar to being a native type in SQL you can bet is pretty core)

Sql Server has had date and time types for a long time but they're not handled by the SqlClient driver. Support was added a week ago and will be in the next release dotnet/SqlClient#1813

The DateOnly and TimeOnly types haven't been present for most of the last 20 years. That doesn't quality them for the status of an essential type. Similarly to Half and in128 they're useful but hardly required. Using ChangeType is a code smell and I suggest you investigate other ways to achieve what you're using it for.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.DateTime documentation Documentation bug or enhancement, does not impact product or test code
Projects
None yet
Development

No branches or pull requests

8 participants