Feature proposal: remove implicit construction of DateTimeOffset from DateTime

[slavanap]

Specifically these 2 lines of code
https://github.com/dotnet/corert/blob/master/src/System.Private.CoreLib/shared/System/DateTimeOffset.cs#L825-L826

public static implicit operator DateTimeOffset(DateTime dateTime) =>
            new DateTimeOffset(dateTime);

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.

[slavanap]

Here's very basic example of the case described

using System;
using System.Linq;
					
public class Program
{
    public static readonly DateTime Epoch =
        new DateTime(1970, 1, 1, 0, 0, 0, DateTimeKind.Utc);
    
    public static readonly TimeZoneInfo EST =
        TimeZoneInfo.GetSystemTimeZones()
            .Where(x => x.Id == "Eastern Standard Time")
            .SingleOrDefault()
        ?? TimeZoneInfo.FindSystemTimeZoneById("America/New_York"); 


    public static DateTimeOffset UnixEpochToDateTimeOffsetInEST(long msVal)
    {
        return TimeZoneInfo.ConvertTime(Epoch.AddSeconds(msVal), EST);
    }

    public static void Main()
    {
        var now = DateTimeOffset.Now;
        Console.WriteLine("Local time = {0}", now);
        Console.WriteLine("UTC time = {0}\n", now.UtcDateTime);


        var epochNow = Convert.ToInt64((now.UtcDateTime - Epoch).TotalSeconds);
        Console.WriteLine("EST time = {0}", UnixEpochToDateTimeOffsetInEST(epochNow));

        // here you get correct time in EST, but local offset.
    }
}

[slavanap]

I think, marking this implicit operator as obsolote may show a lot of bugs around it.

[davidfowl]

Roslyn Analyzer?

[Joe4evr]

Or how about Better Obsoletion?

[bartonjs]

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)

[slavanap]

it should be relatively rare that something accepts both DateTime and DateTimeOffset via overloads.

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.

[slavanap]

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)

[bartonjs]

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 [Obsolete], but that would make reasonable code like

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 array[range] expression is implicitly converted to a ReadOnlySpan) would provide the value you are seeking without as high of costs.

[slavanap]

but that would make reasonable code like

IMHO, I don't think such code expresses clearly what it's doing. I personally would prefer

// if you want to assume someDateTime is in local time zone:
if (someDateTime < someDateTimeOffset.LocalDateTime) ...
// or 
if (new DateTimeOffset(someDateTime) < someDateTimeOffset) ...

// if you want to compare just stored DateTime value:
if (someDateTime < someDateTimeOffset.DateTime) ...

P.S. Just noticed that HEREDOC is not very precise for DateTimeOffset.

[slavanap]

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,
Vyacheslav

[KalleOlaviNiemitalo]

Roslyn Analyzer?

One can already use Microsoft.CodeAnalysis.BannedApiAnalyzers for this.

BannedSymbols.txt:

M:System.DateTimeOffset.op_Implicit(System.DateTime)~System.DateTimeOffset;Might use an unexpected UTC offset.

Unwanted implicit conversion:

DateTimeOffset ofs = DateTime.Now;

A resulting warning:

Sample.cs(38,20,38,70): warning RS0030: The symbol 'DateTimeOffset.implicit operator DateTimeOffset(DateTime)' is banned in this project: Might use an unexpected UTC offset.

[slavanap]

@KalleOlaviNiemitalo thanks for potential solution!
One correction: not "it may use not UTC offset", but "it determines timezone offset based on DateTime.Kind value".
Also, for DateTimeKind.Unspecified it assumes DateTimeKind.Local.

[YohanSciubukgian]

Thanks @KalleOlaviNiemitalo, the alternative solution is working great.
The only downside is that the BannedSymbols.txt + the nugget installation have to be applied on each csproj.
Is there a way to apply the same behavior for a specific solution (.sln) ? or at Visual Studio level ?

[KalleOlaviNiemitalo]

@YohanSciubukgian, yes, it can be done with a Directory.Build.targets file that applies to projects in all subdirectories. See https://github.com/dotnet/roslyn-analyzers/blob/b9bd53c045a46a383f4a7affb2f4a75043de4167/src/Directory.Build.targets#L39-L42 for an example, although you'd have to change the Version attribute.

[miloush]

I think the implicit operator is reasonable. There is no loss of precision. The behaviour is the same as with DateTime constructor without the kind specified, i.e. it's treated as local time unless told otherwise. There is nothing obsolete on the conversion. If you think it is being misused, analyser seems like an appropriate counter measure.

[slavanap]

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

[miloush]

loss of precision, if the offset stored within DateTimeOffset value does not match with your computer local time zone or UTC

@slavanap but the DateTimeOffset is constructed so that the offset always matches either local time or UTC, and that's the direction of the implicit conversion.

Time zone may be updated

Sure, that's why we have both DateTime and DateTimeOffset, no one is forcing you to use one or another. If your intention is a relative time, use DateTime, if absolute, use DateTimeOffset, but you convert at one point in time.

[slavanap]

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

[miloush]

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, IsDaylightSavingTime treats unspecified as local, ToLocalTime treats it as UTC.

The DateTimeKind is not preserved during serialization. The comparison operators do not use it either. You can compare Local and Utc date times but it's only the tick value that matters, they are not converted to each other. Actually in this sense the implicit conversion improves the comparison with DateTimeOffset value as it makes the comparison well defined.

The DateTimeOffset is a newer addition to the framework compared to DateTime, partially to resolve some of the issues above. If I remember correctly, the idea was to lower the barriers of DateTimeOffset adoption, so that libraries/API can switch to DateTimeOffset without much disruption in the existing code. An absolute point in time (represented by DateTimeOffset) is deemed to be the most common use of date/time in code, at least as per the documentation. If you need to do date arithmetic, like in your early example, you shouldn't be using DateTime in the first place. So much for the theory.

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.

[slavanap]

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

[miloush]

As I said the perceived loss is only if you view the "unspecified" as "neither".

It is rare, but disruptive behavior from the behavior expected

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

it allows to review places in code

I think that is a wrong goal. Let me know if I am misunderstanding you, but you are saying:

1. there is a problem of people not paying attention to date behavior and introducing bugs
2. the code with potentially undesired behavior should be reviewed
3. making it obsolete will force people to review

But that is a one-time solution to a long-term problem. Once you change the syntax to new DateTimeOffset(dateTime), we will be where we are now: people will write new code just calling the constructor without much thinking and introducing the same bugs.

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.

[slavanap]

@miloush
This is not "undesired" behavior, but "unexpected" behavior, because of the implicit nature of the operator. I don't know a human who can infer types 100% accurately while reading source code.
That's why,

But that is a one-time solution to a long-term problem. Once you change the syntax to new DateTimeOffset(dateTime), we will be where we are now

even new expression with constructor call will help detecting such cases and handle them as informed developer would like to. Alternatively, one may cast DateTimeOffset to DateTime via DateTimeOffset.LocalDateTime or DateTimeOffset.UtcDateTime properties, which is a good explicit alternative.

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.

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.

[jeffhandley]

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

[slavanap]

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 one would need current behavior, they can call DateTimeOffset constructor explicitly,
• For others, operator obsoletion would warn them of undesirable behavior running application in an environment with a different local time zone setting.

If we're in agreement on that I can work on PR.

[miloush]

If we're in agreement

@jeffhandley just pointed out there isn't an agreement

[tarekgh]

Yes, we are not in agreement of that. The comment #32954 (comment) explained the concerns with that.

[slavanap]

@tarekgh what are the concerns of marking the operator [Obsolete] ? How addition of the attribute can be a breaking change?

[jeffhandley]

@tarekgh what are the concerns of marking the operator [Obsolete] ? How addition of the attribute can be a breaking change?

Adding an [Obsolete] attribute will introduce build warnings for anyone already using this operator. Many developers/teams enable treating warnings as errors as well, which then produces a build failure from the usage of the now-obsolete API. We have a high bar for introducing obsoletions since we strive to provide a smooth version-to-version upgrade experience.

There's some more background on obsoletions here:
https://github.com/dotnet/designs/blob/main/accepted/2020/better-obsoletion/better-obsoletion.md

[IanKemp]

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

[slavanap]

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

• There are other developers who run into the issue unintentionally,
• Developers who want to perform the cast can still do it after deprecation by replacing dt with new DateTimeOffset(dt) or dt.DTO() where DTO is their extension method. The ticket is about to remove implicit construction.
• conversion of DateTimeOffset to DateTime has an ambiguity to treat any Offset different from local time zone and UTC as Kind.Unspecified which later is treated similarly to Kind.Local in other operations. If you use DateTimeOffset with only local or UTC timezones the bug is not visible to you.

[Shivian444]

@tarekgh what are the concerns of marking the operator [Obsolete] ? How addition of the attribute can be a breaking change?

Adding an [Obsolete] attribute will introduce build warnings for anyone already using this operator. Many developers/teams enable treating warnings as errors as well, which then produces a build failure from the usage of the now-obsolete API. We have a high bar for introducing obsoletions since we strive to provide a smooth version-to-version upgrade experience.

There's some more background on obsoletions here: https://github.com/dotnet/designs/blob/main/accepted/2020/better-obsoletion/better-obsoletion.md

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.

[tannergooding]

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?

[tarekgh]

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.

[danmoseley]

Do we have an issue tracking a potential analyzer? I hit this recently myself and it's insidious

[tarekgh]

I don't think there is an analyzer issue for that. We can open one or repurpose this issue to the analyzer.

[tarekgh]

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.

[danmoseley]

I agree