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

Roslyn analyzers for bug-prone usage of DateTime/DateTimeOffset #79105

Open
timmydo opened this issue Dec 1, 2022 · 5 comments
Open

Roslyn analyzers for bug-prone usage of DateTime/DateTimeOffset #79105

timmydo opened this issue Dec 1, 2022 · 5 comments
Labels
area-System.DateTime code-analyzer Marks an issue that suggests a Roslyn analyzer
Milestone

Comments

@timmydo
Copy link

timmydo commented Dec 1, 2022

I recently ran into two separate bugs related to time handling in our code and thought roslyn analyzers might be able to assist.

The two instances were:

DateTimeOffset.Parse("2020-01-02").ToString(...)
and
if (DateTimeOffset.TryParse(expiryString, out var expiry) && now < expiry) // expiry is a utc YYYY-MM-DD date

in both of these usages, I should have used something like DateTimeOffset.TryParse(expiryString, null, System.Globalization.DateTimeStyles.AssumeUniversal, out var expiry)

I see this as similar to requiring explicit usage of culture objects in string comparisons. Is there an analyzer that could recommend the version of the function with the explicit default behavior request like we have for string comparison functions?

Semi-related #43956

@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 Dec 1, 2022
@Clockwork-Muse
Copy link
Contributor

I see this as similar to requiring explicit usage of culture objects in string comparisons.

Yes, especially for one surprising reason - at least one culture uses a YYYY-DD-MM format.

in both of these usages, I should have used something like

You should be using

DateTimeOffset.TryParse(expiryString, DateTimeFormatInfo.InvariantInfo, out var expiry)

... which is explicitly using a culture.

@jkoritzinsky jkoritzinsky added the code-analyzer Marks an issue that suggests a Roslyn analyzer label Dec 1, 2022
@timmydo
Copy link
Author

timmydo commented Dec 1, 2022

Well, I'm glad my attempt at a fix is wrong if it makes the case for an analyzer stronger :)

@tarekgh tarekgh added this to the Future milestone Feb 15, 2023
@tarekgh tarekgh removed the untriaged New issue has not been triaged by the area owner label Feb 15, 2023
@tarekgh
Copy link
Member

tarekgh commented Feb 15, 2023

Actually, if you previously know the format of the date string you should use [Try]ParseExact instead of [Try]Parse.

[Try]Parse is useful only if having date strings which you don't know the actual format and try to parse to get something from it. Recommending Invariant culture or anything else here can break other user cases. We need to think more how we can write this analyzer to address the bad usages and allow the legitimate usage in same time.

@timmydo
Copy link
Author

timmydo commented Feb 16, 2023

I don't think you should recommend invariant culture--you just need to recommend using an API where the culture is explicitly specified, similar to how string comparison API analyzers recommend you specify the culture explicitly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.DateTime code-analyzer Marks an issue that suggests a Roslyn analyzer
Projects
None yet
Development

No branches or pull requests

5 participants