Do not do math with local DateTime

[terrajobst]

Find places where a DateTime known to be in local time (e.g. resulting from DateTime.Now) is used in math.

Category: Reliability

[jeffhandley]

Estimates:

• Analyzer: Large
• Fixer: Not Applicable

[stephentoub]

This might be a dup of dotnet/roslyn-analyzers#2937.

[carlossanlop]

@stephentoub I'm not sure if they are the same. That issue is talking about leap year math specifically.

dotnet/roslyn-analyzers#2937 already got approved back in Dec 2019, before we decided analyzers should go through API proposals in the runtime repo. It even has a PR to implement it.

[buyaa-n]

1. Track DateTime instances created using DateTime.Now or DataTime dt = DateTime.SpecifyKind(datetime, DateTimeKind.Local);.
   • Probably might want to add instances initiated with DateTime.Today because it also created with DateTimeKind .Local
2. Flag usages in Math or comparison operation (for all operators except addition -, ==, !=, <, <=, >, >=) and methods that do not consider the value of the DateTime .Kind property of the two DateTime values for Math or comparison operation (DateTime.Compare, DateTime.CompareTo, DateTime.Subtract, DateTime .Equals)

[carlossanlop]

Suggested severity: warning

Examples:

static void Main()
{
    // Local
    DateTime now = DateTime.Now;
    DateTime nowLocal = DateTime.SpecifyKind(now, DateTimeKind.Local);
    DateTime nowUtc = DateTime.SpecifyKind(now, DateTimeKind.Utc);
    DateTime today = DateTime.Today;

    // UTC
    DateTime utc = DateTime.UtcNow;

    // Unspecified
    DateTime nowWithUnspecifiedKind = DateTime.SpecifyKind(now, DateTimeKind.Unspecified);



    //// FLAGGED ////

    // Flag the variable 'now'
    TimeSpan substracted = now - utc;
    TimeSpan substracted2 = now.Subtract(utc);

    // Flag the variable 'today'
    bool equal = today == utc;
    bool notEqual = today != utc;
    bool lessThan = today <= utc;
    bool greatedThan = today > utc;

    // Flag the variables 'nowLocal'
    bool equal2 = nowLocal.Equals(utc);
    int compareTo = utc.CompareTo(nowLocal);

    // We are comparing unknown with DateTime.Now
    bool equalFromMethod = GetDateTime() == now;
    bool equalFromProperty = MyDateTime == now;

    // Known local compared with unspecified
    int compareWithUnspecified2 = DateTime.Compare(nowWithUnspecifiedKind, now);



    //// NOT FLAGGED ////

    // Skip this case: it not possible to add two datetimes
    // TimeSpan addition = now + utc;

    // Math with timespans does not care about timezone
    DateTime addition = now + TimeSpan.FromDays(5);
    DateTime additionUtc = utc + TimeSpan.FromDays(5);

    // Both are local
    bool equalAllowed = today == now;

    // Both are utc
    int compareToAllowed = utc.CompareTo(nowUtc);

    // Don't know if the returned values of methods and properties are DateTime.Now or not
    bool equalFromMethod2 = GetDateTime() == utc;
    bool equalFromProperty2 = MyDateTime == utc;




    //// UNCLEAR ////

    // Should unspecified DateTimeKind be flagged when compared to a known utc or a known now?
    int compareWithUnspecified = DateTime.Compare(nowWithUnspecifiedKind, utc);
}

static DateTime GetDateTime()
{
    DateTime x = ...;
    return x;
}

static DateTime MyDateTime
{
    get
    {
        DateTime x = ...;
        return x;
    }
}

[mattjohnsonpint]

Hi. Just seeing this now, but I'm very interested and would like to help in any way I can.

Feedback on the proposal:

• I suggest flagging any usage of local time if the project is an ASP.Net project (assuming such awarness is possible.) Local time in a web project is the time zone setting of the server, which is often UTC but should be considered irrelevant. The analyzer would warn on any usage of DateTime.Now, DateTime.Today, DateTimeOffset.Now, DateTimeKind.Local, or TimeZoneInfo.Local - but only when the project sdk is Microsoft.NET.Sdk.Web.

• I'm unsure why equalFromMethod and equalFromProperty are flagged but equalFromMethod2 and equalFromProperty2 are not. It seems the only difference is one is comparing against UTC and one is comparing against local time. Both have unknown (not unspecified) kind on the left-side, so I would think that comparison should be allowed. Same for compareWithUnspecified2. In particular, it is a common use case to pull a datetime from elsewhere and compare against local time. For example, the following should be allowed:

  // call some retrieval or calculation function that returns the current birthday (with time truncated)
  DateTime birthday = GetCurrentYearBirthday();
  
  // equality comparison to local time is valid for desktop apps
  if (birthday == DateTime.Today)
      Console.WriteLine("Happy Birthday!");

• The above addresses comparison with unknown or unspecified against local time. However, the "unclear" item is asking about UTC comparison. I believe that case should be flagged. Comparison to UTC is inherently a point-in-time operation. If one side is UTC but the other is known to be Local or Unspecified kinds, then it should flag. (but not if one side is unknown)

• The statement "Math with timespans does not care about timezone" is incorrect. This is a common source of bugs related to daylight saving time transition. Adding to a DateTime goes strictly off wall time, so if one adds an absolute unit of time to a non-UTC DateTime, then they could potentially be getting invalid results if the operation crosses a transition in the relevant time zone. I suggest flagging on Add, AddHours, AddMinutes, AddSeconds, and AddTicks when the kind is known to be Local or Unspecified. Don't flag on Utc or unknown kind. Don't flag AddYears, AddMonths, or AddDays at all, because those are date-math operations that are expected to operate on wall-time rather than absolute time.

Also, I worked with @kwilkins on the original leap-year efforts. He wrote the analyzer that we sent the PR for, but has since moved on to other efforts. The feedback from that PR is that we should re-write it using a dataflow analysis approach. I am just starting to pick this back up and am researching that approach. I think it applies to much of what is written here as well.

May I suggest that the leap-year analyzer proposal (dotnet/roslyn-analyzers#2937) and the one proposed here be merged into a common set of date/time analyzers? I believe the previous PR (dotnet/roslyn-analyzers#3152) can be abandoned.

I can think of many other useful scenarios that could be added to this as well - various time zone and DST issues such as the ones I blogged about many years ago, and others. I'd like to participate in the design and implementation of this. Is the best way to go through the proposal process for each one? Or should I perhaps start on a standalone implementation to prototype?

Thanks.

[bartonjs]

Video

It's unclear what is being proposed here. The examples seem to focus on the interaction with DateTimeKind.Local and DateTimeKind.Utc, not "do[ing] math with local DateTime", as the title suggests.

It's also not clear if this is realistically achievable, since any DateTime that comes in as a method parameter has a DateTimeKind that is not known to the called method (unless something coerced it).

Perhaps an updated title, or more clear examples would help drive a discussion.