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

cmd/vet: check for time formats with 2006-02-01 #48801

Closed
erikdubbelboer opened this issue Oct 5, 2021 · 24 comments
Closed

cmd/vet: check for time formats with 2006-02-01 #48801

erikdubbelboer opened this issue Oct 5, 2021 · 24 comments

Comments

@erikdubbelboer
Copy link
Contributor

yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is much more likely that the user intended to use
yyyy-mm-dd instead and made a mistake. This happens quite often [2] because of the unusual way to handle time formatting and parsing in Go. Since the mistake is Go specific and happens so often a vet check will be useful.

See: golang/tools#342

  1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
  2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code
@gopherbot gopherbot added this to the Proposal milestone Oct 5, 2021
@seankhliao
Copy link
Member

I don't think "isn't really used" meets the precision bar for vet
Looking over the instances where they show up, there seem to be valid uses in there, especially wrt to tests

@seankhliao seankhliao changed the title proposal: add vet check for time formats with 2006-02-01 proposal: cmd/vet: check for time formats with 2006-02-01 Oct 5, 2021
@ianlancetaylor ianlancetaylor added this to Incoming in Proposals (old) Oct 6, 2021
@rsc
Copy link
Contributor

rsc commented Oct 6, 2021

We would need some evidence of frequency to put this in. Is there any?

@rsc
Copy link
Contributor

rsc commented Oct 6, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc rsc moved this from Incoming to Active in Proposals (old) Oct 6, 2021
@rsc
Copy link
Contributor

rsc commented Oct 13, 2021

That's pretty surprising but it does seem to meet the vet bars:

  • Correctness - a real or potential bug for sure
  • Frequency - seems to happen more than we expected!
  • Precision - can't imagine why anyone would use YYYY-DD-MM date format

@twmb
Copy link
Contributor

twmb commented Oct 14, 2021

The buffalo link is a test and specifically follows the 2006-02-01 line checking that 2017-01-10 is parsed as October 1st. The couchbase code looks to be code that generates database entries, and I wouldn't be surprised if it's deliberately choosing the US date format. Some other code in the first page of that search is explicitly using US format:

https://github.com/jserranojunior/learning-golang/blob/0503e6c60367225fc4b1218123b121bfce063441/Projects/ConvertDates/ConvertDate.go#L27

I've in the past run into YYYY-DD-MM and MM-DD-YYYY when dealing with government forms (which was highly confusing, personally).

I think this might be a good signal some of time, but would have false positives. I also feel that anybody that knows the 2006, 01, and 02 numbers also knows the documentation mentioning that 01 is the month and 02 is the day (or, if they're like me, the person looks up what 01 and 02 refer to every time they need to format a time in Go). I'd bet that more often than not, the choice of 02 and 01 is deliberate. If anything, this feels like a check more suited for a linter that an org / teams could opt into, rather than a default check that cannot be opted out of.

@rsc
Copy link
Contributor

rsc commented Oct 20, 2021

@twmb can you give specific examples of uses of YYYY-DD-MM?
https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format suggests it is not used anywhere.

The buffalo link and test looks like a bug to me, compounded by someone taking the output and hard-coding that as the expected answer.

Similarly, the jserranojunior link looks like a bug. The date is parsed and then reformatted using the same 2006-02-01 format, so you wouldn't notice for many dates that it was wrong. (Up until the 13th of the month it would pass through just fine.)

I don't understand the reference to YYYY-DD-MM as "US format". I have lived in the US my whole life and never seen a date written that way. Halloween is 10/31 or 10-31 not 31-10 or 31/10 and definitely not 2021-31-10.

@timothy-king
Copy link
Contributor

https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format points to a wikipedia entry: https://en.wikipedia.org/wiki/Date_format_by_country. It states for Kazakhstan:

Short format: (yyyy.dd.mm) in Kazakh

Like the SO post I also cannot read the citation for this. "yyyy.dd.mm" is close but uses a different delimiter ("." instead of "-").

@erikdubbelboer
Copy link
Contributor Author

I tried to make sense of the document but Google Translate doesn't really help. The specific section is:

Құжаттың күні араб санымен мынадай дәйектілікпен: жыл, айдың күні, ай. Айдың күні мен ай жұп араб санымен
нүкте арқылы бөлініп, жыл төрт араб санымен ресімделеді.

Which translates to:

The date of the document is in Arabic numerals in the following order: year, day of the month, month. The day of
the month and the month are separated by a pair of Arabic numerals, and the year is represented by four Arabic
numerals.

But since the author(s) on Wikipedia specifically use . as delimiter I don't think this format is relevant to this proposal.

@rsc
Copy link
Contributor

rsc commented Oct 27, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Oct 27, 2021
@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Nov 3, 2021
@rsc
Copy link
Contributor

rsc commented Nov 3, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: cmd/vet: check for time formats with 2006-02-01 cmd/vet: check for time formats with 2006-02-01 Nov 3, 2021
@rsc rsc modified the milestones: Proposal, Backlog Nov 3, 2021
@erikdubbelboer
Copy link
Contributor Author

That is great to hear. I already suggested an implementation here: https://go-review.googlesource.com/c/tools/+/354010 (and here: golang/tools#342)

@ernado
Copy link
Contributor

ernado commented Nov 5, 2021

There is a little chance of false positive in places that had to provide compatibility with such erroneous format.

For example, we can find such in LLVM code (which is still a bug), in some weird Apache tests and other places (?).

Adding this to go vet is unfortunate, IMO this should be implemented as linter that at least supports suppression.

@ianlancetaylor
Copy link
Contributor

@ernado As seen above there is Go code that erroneously uses this format, so the vet check is useful. Your examples are not Go code, so it's hard to know how significant those cases are. It will be possible to avoid the vet check by using a variable rather than a constant.

@ernado
Copy link
Contributor

ernado commented Nov 5, 2021

Agree, thank you.

@erikdubbelboer
Copy link
Contributor Author

The LLVM code says yyyy-dd-mm in the comment but has $year, $month, $day, in the code.

@icio
Copy link

icio commented Apr 22, 2022

If "2006-01-02" is worthy of a vet check, is it also worthy of a constant? Maybe time.RFC3339Date, time.ISO8601Date or just time.ISODate?

@erikdubbelboer
Copy link
Contributor Author

I'm not sure, none of the other formats have separate constants for the datetime and just the date.

erikdubbelboer added a commit to erikdubbelboer/tools that referenced this issue May 7, 2022
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.

Updates golang/go#48801

1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code
erikdubbelboer added a commit to erikdubbelboer/tools that referenced this issue May 27, 2022
Updates golang/go#48801

yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.

1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code
@gopherbot
Copy link

Change https://go.dev/cl/354010 mentions this issue: go/tools: add check for time formats with 2006-02-01

erikdubbelboer added a commit to erikdubbelboer/tools that referenced this issue Jun 5, 2022
Updates golang/go#48801

yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.

1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code
@icio
Copy link

icio commented Jun 6, 2022

I'm not sure, none of the other formats have separate constants for the datetime and just the date.

Indeed, but they also don't have a vet check.

@erikdubbelboer
Copy link
Contributor Author

If "2006-01-02" is worthy of a vet check, is it also worthy of a constant? Maybe time.RFC3339Date, time.ISO8601Date or just time.ISODate?

Seems like this is going to happen: #52746

erikdubbelboer added a commit to erikdubbelboer/tools that referenced this issue Jul 2, 2022
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.

1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code

Updates golang/go#48801
erikdubbelboer added a commit to erikdubbelboer/tools that referenced this issue Jul 19, 2022
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.

1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code

Updates golang/go#48801
erikdubbelboer added a commit to erikdubbelboer/tools that referenced this issue Jul 22, 2022
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.

1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code

Updates golang/go#48801
erikdubbelboer added a commit to erikdubbelboer/tools that referenced this issue Jul 29, 2022
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.

1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code

Updates golang/go#48801
gopherbot pushed a commit to golang/tools that referenced this issue Aug 3, 2022
yyyy-dd-mm is a time format that isn't really used anywhere [1]. It is
much more likely that the user intended to use yyyy-mm-dd instead and
made a mistake. This happens quite often [2] because of the unusual way
to handle time formatting and parsing in Go. Since the mistake is Go
specific and happens so often a vet check will be useful.

1. https://stackoverflow.com/questions/2254014/are-there-locales-or-common-programs-that-use-yyyy-dd-mm-as-the-date-format
2. https://github.com/search?l=&p=1&q=%222006-02-01%22+language%3AGo&type=Code

Updates golang/go#48801

Change-Id: I20960c93710766f20a7df90873bff960dea41b28
GitHub-Last-Rev: 496b991
GitHub-Pull-Request: #342
Reviewed-on: https://go-review.googlesource.com/c/tools/+/354010
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Tim King <taking@google.com>
Run-TryBot: Tim King <taking@google.com>
Reviewed-by: Robert Findley <rfindley@google.com>
@dmitshur dmitshur modified the milestones: Backlog, Go1.20 Aug 24, 2022
@AlexanderYastrebov
Copy link
Contributor

Should this be closed given https://go.dev/cl/354010 is merged?

@seankhliao
Copy link
Member

CL 354010 adds the timeformat analysis pass, but it's not part of cmd/vet yet.

@gopherbot
Copy link

Change https://go.dev/cl/450495 mentions this issue: cmd/vet: enable timeformat analyzer

@golang golang locked and limited conversation to collaborators Nov 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests