-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Feature proposal: remove implicit construction of DateTimeOffset from DateTime #32954
Comments
Here's very basic example of the case described
|
I think, marking this implicit operator as obsolote may show a lot of bugs around it. |
Roslyn Analyzer? |
Or how about Better Obsoletion? |
This seems better as an analyzer. The breaking change would be very disruptive, and it should be relatively rare that something accepts both DateTime and DateTimeOffset via overloads. I'd even limit the analyzer to identifying the confusing cases, where a DateTime is provided to a method that takes a DateTimeOffset in an overload and the output of that method is a DateTime that is implicitly converted. Or maybe it's specifically a problem with TimeZoneInfo.ConvertTime(DateTime, TZI) |
A DateTimeOffset typed variable can be constructed from either DateTime or DateTimeOffset. This case is pretty often and dangerous when one works with DateTimeOffset typed variables and time conversions. Please let me know if the basic example of the issue above that I've listed is not clear. |
Don't get me wrong, I'm not for removing the constructor, I'm for removing the implicit construction. The one who wants to construct DateTimeOffset from DateTime will still be able to do it, but such obsoletion will force them to list this conversion explicitly, e.g. new DateTimeOffset(DateTime.Now) |
In addition to the obvious source-breaking change, removing the implicit conversion is a binary breaking change which would make .NET (Core) no longer compatible with .NET Standard, so it's unlikely to happen in the foreseeable future. The conversion could potentially be marked as if (someDateTime < someDateTimeOffset)
{
...
} start to get obsoletion warnings. (In point of fact, DateTime/DateTimeOffset is called out in Framework Design Guidelines as doing operators well via the implicit conversion.) It feels like an analyzer for identifying where such an implicit conversion is problematic (like the one we added for detecting when an |
IMHO, I don't think such code expresses clearly what it's doing. I personally would prefer
P.S. Just noticed that HEREDOC is not very precise for DateTimeOffset. |
Hi @joperezr, Before this issue drowns deep into your future milestone, could you please ask someone of the framework developers to comment on it? I have a strong feeling there are lots of bugs in production code associated with this API misuse. Thanks, |
One can already use Microsoft.CodeAnalysis.BannedApiAnalyzers for this. BannedSymbols.txt:
Unwanted implicit conversion: DateTimeOffset ofs = DateTime.Now; A resulting warning:
|
@KalleOlaviNiemitalo thanks for potential solution! |
Thanks @KalleOlaviNiemitalo, the alternative solution is working great. |
@YohanSciubukgian, yes, it can be done with a |
I think the implicit operator is reasonable. There is no loss of precision. The behaviour is the same as with |
@miloush there's a loss of precision, if the offset stored within DateTime value does not match with your computer local time zone or UTC. Not to mention that your local time zone may be updated by you any time while the program runs. |
@slavanap but the
Sure, that's why we have both |
@miloush there's a problem with this if you work with DateTime with Kind = Unspecified and can easy compare it with DateTimeOffset value, which leads to Undefined Behavior for the conversion because of luck of information what offset Unspecified Kind of DateTime should be used for. And it's very easy to receive DateTime with the kind of Unspecified, if you work with at least 3 timezones, because DateTimeOffset.DateTime has that specific time. The core issue is that DateTime with Unspecified Kind is silently converted to DateTimeOffset with local time zone offset. Sorry, if my message above hasn't been clear about that. |
I think the issue here is that you are interpreting "unspecified" as "neither local nor UTC", while the API treats it more like "could be local or UTC, whichever you want". For example, The The I understand your frustration, and I support helping developers to make the right decisions, I just don't think obsoletion is the right tool for this. |
@miloush I think that precedence of DateTime over DateTimeOffset is not correct and is the real issue while you're making assumptions like this. These types are independent types. They can be used for functions overloads and with this implicit conversion of one over another, it creates possiblity of the API incorrect usage like in the example above. Incorrect usage that is hardly debuggable, because if time zone used matches local or UTC (which are covered by DateTimeKind enum), the potential bug does not show itself until either local time zone starts to differ from your stored value or new time zone introduced in a program operation somehow else. I'm ultimately against any implicit conversion if it may lead to loss of data. In this case this loss is achievable with DateTimeKind.Unspecified. It is rare, but disruptive behavior from the behavior expected by a developer, thus I suggest to make this transition between types more explicit, that will force developers to pay attention to these border cases when explicitly using them. Also, I don't think that adding such conversion explicitly (with straight-forward "new DateTimeOffset (X)" expression after implicit operator obsoletion) is too complicated task, but it allows to review places in code with potential issues or ignore them if one would prefer so, but forcing everyone to potentially ignore such use cases by default is not constructive. P.S. And I doubt a little in analyzers these days because CodeLens, for instance, still doesn't see class property usage with += expression in the latest available VS version. |
As I said the perceived loss is only if you view the "unspecified" as "neither".
It works as expected to me - clearly there are developers with different expectations here. Making the operator obsolete will therefore be also disruptive, to existing code that uses it correctly. (I don't have any numbers to judge which situation is more common.)
I think that is a wrong goal. Let me know if I am misunderstanding you, but you are saying:
But that is a one-time solution to a long-term problem. Once you change the syntax to I don't think I have any new arguments to the discussion. Obsolete either means "there is a better alternative" or "there is no alternative but you need to stop anyway". I don't consider different syntax to be a better alternative, it is still the same behavior without any new mitigations. |
@miloush
even
Obsolete also means that "suggested approach with API brings unintended issues", not to say that obsoletion is a required step before removal of any part of an API. |
I'm doing a pass over the obsoletion candidates before .NET 6.0 locks down. My interpretation on this suggestion is that we haven't reached a consensus on whether or not we should mark this implicit conversion as obsolete; instead we prefer heading in the analyzer direction (at least doing that first). /cc @tarekgh |
If I was in charge of such decisions, then I'd just marked that implicit conversion obsolete. In my opinion there's no argument of keeping it except for backward compatibility, because
If we're in agreement on that I can work on PR. |
@jeffhandley just pointed out there isn't an agreement |
Yes, we are not in agreement of that. The comment #32954 (comment) explained the concerns with that. |
@tarekgh what are the concerns of marking the operator [Obsolete] ? How addition of the attribute can be a breaking change? |
Adding an There's some more background on obsoletions here: |
@slavanap just because it's possible to blow your own foot off with this API doesn't make it a candidate for obsoletion. I'm sure there are many developers who are using the implicit operator exactly as it's intended, with full understanding of the consequences, who would be very irritated if it started telling them that they should not be using it. As such there is zero chance the .NET team is going to obsolete it simply because you feel strongly about it. A Roslyn analyzer is the correct method to address any possible issues with usage of this API. Developers who are using the operator and understand what it's doing can suppress the analyzer's warning, developers who are using it incorrectly can look at the warning and take corrective action, and the 99.99% of developers who ignore all warnings will continue to do so. |
@IanKemp I'm sorry, I have to ask you, have you read full discussion in the thread? If not, here's a short summary for you:
|
Can we have say a compiler directive to block it at solution level then? If our solution would explicitly never want this behavior I think it would make sense to have some sort of control on implicit behaviors like these. |
CC. @tarekgh, is this an issue we should keep open or close? If we should keep it open, is it near a point where it can be marked ready for review? |
I don't think we'll go with the proposal. As suggested, adding analyzer rule for it would be better as there is no agreement on the obsoletion. |
Do we have an issue tracking a potential analyzer? I hit this recently myself and it's insidious |
I don't think there is an analyzer issue for that. We can open one or repurpose this issue to the analyzer. |
I have opened the analyzer issue to track adding the analyzer for this implicit operator. if all agree, we can close this issue now and follow up with the analyzer issue. |
I agree |
Specifically these 2 lines of code
https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/shared/System/DateTimeOffset.cs#L825-L826
Misleading usecase is that TimeZoneInfo.ConvertTime has 2 overloads for DateTime and DateTimeOffset. When result of the first one is assigned to DateTimeOffset typed variable, DateTimeOffset record with local time zone offset is created. This is unclear for common code reader there's something unintended behaviour may take a place ((hey, I've supplied date, time and timezone to this function, and expect it to return date&time object for this timezone)), because types of either DateTime or DateTimeOffset that comes to ConvertTime argument may be masked by complex computational expression.
If the person wants to shoot their leg, let them do it, but explicitly.
The text was updated successfully, but these errors were encountered: