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

Open
erikdubbelboer opened this issue Oct 5, 2021 · 16 comments
Open

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

erikdubbelboer opened this issue Oct 5, 2021 · 16 comments

Comments

@erikdubbelboer
Copy link
Contributor

@erikdubbelboer erikdubbelboer commented Oct 5, 2021

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
Contributor

@seankhliao seankhliao commented Oct 5, 2021

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

Loading

@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 Oct 6, 2021
@rsc
Copy link
Contributor

@rsc rsc commented Oct 6, 2021

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

Loading

@rsc
Copy link
Contributor

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

Loading

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

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

Loading

@twmb
Copy link
Contributor

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

Loading

@rsc
Copy link
Contributor

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

Loading

@timothy-king
Copy link
Contributor

@timothy-king timothy-king commented Oct 20, 2021

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

Loading

@erikdubbelboer
Copy link
Contributor Author

@erikdubbelboer erikdubbelboer commented Oct 21, 2021

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.

Loading

@rsc
Copy link
Contributor

@rsc rsc commented Oct 27, 2021

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

Loading

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

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

Loading

@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 removed this from the Proposal milestone Nov 3, 2021
@rsc rsc added this to the Backlog milestone Nov 3, 2021
@erikdubbelboer
Copy link
Contributor Author

@erikdubbelboer erikdubbelboer commented Nov 3, 2021

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

Loading

@ernado
Copy link
Contributor

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

Loading

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Nov 5, 2021

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

Loading

@ernado
Copy link
Contributor

@ernado ernado commented Nov 5, 2021

Agree, thank you.

Loading

@erikdubbelboer
Copy link
Contributor Author

@erikdubbelboer erikdubbelboer commented Nov 6, 2021

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

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants