staticcheck: Report incorrect equality checks of time.Time values.

[dmitshur]

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.

[dmitshur]

Although https://godoc.org/time#Time says:

Note that the Go == operator compares not just the time instant but also the Location. Therefore, Time values should not be used as map or database keys without first guaranteeing that the identical Location has been set for all values, which can be achieved through use of the UTC or Local method.

Which kinda implies that there are some legal uses of using == operator on time.Time values? I'm not sure.

This might also change, get updated or clarified for Go 1.9 as a result of https://github.com/golang/go/issues/12914.

[dmitshur]

This issue might be invalid, or at least headed in that direction in Go 1.9.

From Go master godoc of time package:

Note that the Go == operator includes the monotonic clock reading in its comparison. If time values returned from time.Now and time values constructed by other means (for example, by time.Parse or time.Unix) are meant to compare equal when used as map keys, the times returned by time.Now must have the monotonic clock reading stripped, by setting t = t.AddDate(0, 0, 0). In general, prefer t.Equal(u) to t == u, since t.Equal uses the most accurate comparison available and correctly handles the case when only one of its arguments has a monotonic clock reading.

The phrase "In general, prefer t.Equal(u) to t == u" tells me that it's not illegal to use == operator on time.Time values, and as a result, this might not be a valid suggestion staticcheck can make.

[dominikh]

Will have to evaluate, putting it on the todo list.

[mvdan]

I've used == in the past with times I have called UTC() on, so I'm inclined to believe this would be too strict.

[myitcv]

Just to add a bit of colour to the 👍 I added to @mvdan's comments. We use time.Time values 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]

To be exact, I meant the same - equality in general, not just ==. Map keys can't be told to use .Equal().

[dominikh]

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]

I think we should turn it around and say it's a bug in Go documentation of time package.

Do not use == with Time values.

That sentence, as is, cannot be correct based on discussion above.

[mvdan]

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

Elsewhere in the time package documentation it uses a much better and correct phrase:

In general, prefer t.Equal(u) to t == u, since [...].

I don't think there's much room for interpretation of "do not do X" except that doing X is not allowed.

[mvdan]

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]

Then Time values can't be used at all in comparable types, for example when used as map keys at some level.

What makes you say that?

Read the entire paragraph I quoted in https://github.com/dominikh/go-tools/issues/47#issuecomment-283490396, it pretty much says explicitly that you can use time.Time values 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 https://github.com/shurcooL/backlog/issues/31.

[mvdan]

Ah, yes, sorry. Then I would say that the Go docs are wrong - they not say never use ==. The longer paragraph saying In general, prefer t.Equal(u) to t == u is more accurate and should be enough.

[dmitshur]

Following up here. I believe there's a documentation issue in the time package, and I've reported it at https://github.com/golang/go/issues/19510.

[dominikh]

Reopening under the aggressive label, for a future set of checks specific to code review.