Repository navigation
staticcheck: Report incorrect equality checks of time.Time values. #47
Description
Package time documents that:
Do not use == with Time values.
(Source: https://godoc.org/time#Time.Equal.)
This probably extends to != for inequality checks too.
Activity
dmitshur commentedon Mar 1, 2017
Although https://godoc.org/time#Time says:
Which kinda implies that there are some legal uses of using
==operator ontime.Timevalues? I'm not sure.This might also change, get updated or clarified for Go 1.9 as a result of golang/go#12914.
dmitshur commentedon Mar 1, 2017
This issue might be invalid, or at least headed in that direction in Go 1.9.
From Go master godoc of
timepackage:The phrase "In general, prefer t.Equal(u) to t == u" tells me that it's not illegal to use
==operator ontime.Timevalues, and as a result, this might not be a valid suggestionstaticcheckcan make.dominikh commentedon Mar 1, 2017
Will have to evaluate, putting it on the todo list.
mvdan commentedon Mar 1, 2017
I've used
==in the past with times I have calledUTC()on, so I'm inclined to believe this would be too strict.myitcv commentedon Mar 2, 2017
Just to add a bit of colour to the 👍 I added to @mvdan's comments. We use
time.Timevalues as part of map keys which requires us (per the docs) to have guaranteed the locations are identical. As a result, we can use==everywhere (and do), implicitly via map key hashing but also explicitly. So having such a rule would be a false positive in our case where, per the docs, there is not an issue of correctness.mvdan commentedon Mar 2, 2017
To be exact, I meant the same - equality in general, not just
==. Map keys can't be told to use.Equal().dominikh commentedon Mar 2, 2017
Based on the feedback provided I'm going to close this issue. While we could, via flow analysis, track time.Times that have been modified so that comparing them is save, this would reduce the number of false positives, not eliminate them. staticcheck rules, however, must err on the side of having false negatives, not false positives.
dmitshur commentedon Mar 2, 2017
I think we should turn it around and say it's a bug in Go documentation of time package.
That sentence, as is, cannot be correct based on discussion above.
mvdan commentedon Mar 2, 2017
@shurcooL I take that more as a "Do not use equality on Time values unless you know what you're doing", i.e. "Do not use equality on Time values unless you're sure they have the same Location".
That might be too lax for the Go spec though, and they might want to restrict this usage in case they want to break programs that use equality like that in the future.
dmitshur commentedon Mar 2, 2017
Elsewhere in the time package documentation it uses a much better and correct phrase:
I don't think there's much room for interpretation of "do not do X" except that doing X is not allowed.
mvdan commentedon Mar 2, 2017
Then Time values can't be used at all in comparable types, for example when used as map keys at some level. That's unfortunate. I've been using Go as if calling UTC() was fine. Even if it isn't technically, as of 1.8 it is in practice.
dmitshur commentedon Mar 2, 2017
What makes you say that?
Read the entire paragraph I quoted in #47 (comment), it pretty much says explicitly that you can use
time.Timevalues as map keys, provided you perform the neccessary actions to make==operator work as expected (and there are extra considerations in Go 1.9 compared to 1.8).Anyway, I'm tracking dealing with this issue upstream at shurcooL-legacy/backlog#31.
mvdan commentedon Mar 3, 2017
Ah, yes, sorry. Then I would say that the Go docs are wrong - they not say
never use ==. The longer paragraph sayingIn general, prefer t.Equal(u) to t == uis more accurate and should be enough.dmitshur commentedon Mar 11, 2017
Following up here. I believe there's a documentation issue in the
timepackage, and I've reported it at golang/go#19510.dominikh commentedon Oct 15, 2021
Reopening under the
aggressivelabel, for a future set of checks specific to code review.