Roslyn: DateTime.UtcNow Analyzer

[mikadumont]

Analyzer that suggests DateTime.UtcNow whenever DateTime.Now is used.

[sharwell]

I would be _more_ in favor of an analyzer that suggests DateTimeOffset whenever DateTime is used.

[Therzok]

This probably should be filed under dotnet/roslyn-analyzers as there are no BCL analyzers in the roslyn repo?

[Neme12]

@sharwell There are totally valid use cases when you might not need the offset at all. Even really simple things like storing timestamps in a database. You want to know when something happened (the point in time, probably sorted as unix time or a date & time in UTC) but do not care at all what time zone offset the server was in at the time the event happened. In fact the system time zone of the server should always be irrelevant.

[Neme12]

That is like saying there is no valid use case for an Instant and each usage should be flagged to be replaced with a date time and an offset. @jskeet please help me out here :D

[Neme12]

I think I can see why some people argue for always using DateTimeOffset because with DateTime, you can never be sure whether it's in UTC or not, whereas when you use DateTimeOffset, you always know the UTC time (and also the local time in case you're interested in it). But this is really just that way because of the poor design of System.DateTime.

Adding an offset to your data when you absolutely do not need it seems like a bandaid - it allows you to use a data type that tells you the instant in time and the local time. Even though you don't need the local time. Only because there is no data type that reliably tells you the instant on its own.

[sharwell]

@Neme12 DateTimeOffset is harder for inexperienced developers to misuse, and does a better job of expressing context information in API signatures (where DateTime values at minimum require additional documentation). It's like Instant, except without adding a new dependency.

[Neme12]

@sharwell Even if you come to the conclusion that DateTimeOffset is a better data type to use to represent instants in time, there's still a need for local date-only or date-time values.

What I want to store somebody's birthday? That's a date without a time, and it's not tied to any particular time zone or any instant(s) in time, and in .NET the best option is to use DateTime. It's not a 24-hour period starting from a particular point in time (and definitely not a point in time). It's a box in a calendar. If I'm in a different time zone it's not that my birthday suddenly starts in the middle of the day. It doesn't matter where in the world I am. I will always observe it in my local time.

I've seen developers screw this up so many times, where they store something that's supposed to be a local date as a DateTimeOffset and the next day they get a bug report that sometimes the date ends ups being a different date, because they stored an offset where they shouldn't have (they store a time of 00:00 and voila - somebody calls their API with a different offset and the date ends up being wrong... or they compare points in time where they really should have compared dates - somebody has a different offset and suddenly equality is broken).

I've seen so many bugs caused by the fact that people stored (or used DateTimeOffset but used it to retrieve) instants in time where they really needed something else. "Just always use DateTimeOffset" is not a good rule to follow.

[jmarolf]

I suggest this conversation be moved to roslyn-analyzers or be added to the list on this issue

CC: @mavasani

[Neme12]

I also don't necessarily agree that using DateTime.Now is always a bad idea. It depends on the kind of app you're building. Yes, in a server application, it's almost always the wrong thing to do and you should look at every DateTime.Now reference with a lot of scepticism. In a desktop app? Maybe - and maybe not - it really depends on what you're doing. It would be totally fine for the Windows taskbar to use DateTime.Now to show you the time. It would be totally fine for your smartwatch to use DateTime.Now.

and does a better job of expressing context information in API signatures

Maybe. It still doesn't tell you whether the API actually needs the instant in time or the local time. It very rarely needs both. In reality, DateTimeOffset has a really narrow use case but people advocate for it because it has 2 pieces of information and you can get both, depending on which you want, even though in reality you almost always only want 1 of these and know exactly which one.

In any case, having analyzers for this seems like a bad solution to a very real problem. .NET should include proper NodaTime-like data types in the BCL and maybe even deprecates the current ones, just like Java did with java.time. No analyzer can really work here because there is no silver bullet. .NET only has 2 date/time data types and adding an analyzer to enforce only using 1 does not make the situation better, but rather worse IMHO.

[jskeet]

I agree that DateTime.Now is far more likely to be correct in local applications than in anything server-side. I'd be in favour of this analyzer being available, and being enabled in ASP.NET Core project templates.