proposal: cmd/vet: time.Time should not compared

[colin-sitehost]

background

Time is hard and time.Time is really great in most all cases, but between the two issues (#45958 #45960) I came across recently, it seems like anything that does a runtime equality check is probably a bug. Please let me know if there are cases where comparing time is good, safe, and necessary.

summary

As such, it seems reasonable to add a vet check to disallow using time.Time like so:

if time.Time{} == time.Time{} {}
map[time.Time]struct{}{{}: {}}

alternatives

I support enforcing this via the complier, but I get the impression that is a non starter:

package time

type Time struct {
	_    [0]func()
	wall uint64
	ext  int64
	loc  *Location
}

We could also add a new symbol that is intentionally comparable, say time.Comparable, but it would be somewhat confusing when to use which one, and that would probably involve a go vet rule too.

[seankhliao]

related #22978

[guodongli-google]

Comparing times is an acceptable usage as described in the document, although extra care is needed:

"Note that the Go == operator compares not just the time instant but also the Location and the monotonic clock reading. See the documentation for the Time type for a discussion of equality testing for Time values."

Issues (#45958 #45960) are more related to the marshaling and unmarshaling of the time object. Since Specific fields will be ignored during marshaling/unmarshaling, the "==" operator will already return false.

"the serialized forms generated by t.GobEncode, t.MarshalBinary, t.MarshalJSON, and t.MarshalText omit the monotonic clock reading, and t.Format provides no format for it. Similarly, the constructors time.Date, time.Parse, time.ParseInLocation, and time.Unix, as well as the unmarshalers t.GobDecode, t.UnmarshalBinary. t.UnmarshalJSON, and t.UnmarshalText always create times with no monotonic clock reading".

So the problem is to compare two times after marshaling/unmarshaling rather than comparing two times generally, and the following patterns are not necessarily bugs?

if time.Time{...} == time.Time{...} {}
map[time.Time]...

In addition, if we want to capture these general patterns, a StaticCheck checker seems more appropriate.

[colin-sitehost]

the phrase "extra care is needed" scares me; I agree with your assertion, there exist cases where informed users can safely use ==, but that is not what I see. this whole domain is really complex, and it is easy to miss this nuance, especially as a new gopher, plus most people (in my experience) mean time.Time.Equal when they write ==.

if we feel this check is too false positive prone, I think moving it to StaticCheck is reasonable, but I would always prefer first party integrations, so I started here.

[dominikh]

if we feel this check is too false positive prone, I think moving it to StaticCheck is reasonable

Note that Staticcheck isn't the right home for checks with false positives, either.

The next major release of Staticcheck will, when running inside gopls, emit a suggestion to use time.Time.Equal instead of ==, but it will be a suggestion, not a warning or error, and it will not be emitted when running Staticcheck standalone. It is merely a suggestion to look at the code carefully.

[timothy-king]

Here is an excerpt from the documentation on time.Time on comparisons:

Note that the Go == operator compares not just the time instant but also the Location and the monotonic clock reading. 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, and that the monotonic clock reading has been stripped by setting t = t.Round(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.

This sounds like there exist valid usages (an real world example would be nice), but these are tricky to get right and [at least] moderately error prone.

if we feel this check is too false positive prone,

This comment from #22978 is I think basically still the issue for vet:

But somebody could do some analysis on GitHub to see what percentage of time.Time-keyed maps are valid.

Evidence to answer the false positive question would be great. If this is low enough, vet could consider it. vet probably is not the right home at a 10% false positive rate. We have evidence of bugs that could be caught but no great sense of how many people would be impacted if we turned this on.

/cc @richardartoul did a check for this ever make it into gometalinter ?

[colin-sitehost]

This sounds like there exist valid usages (an real world example would be nice), but these are tricky to get right and [at least] moderately error prone.

I see two use cases:

• unnecessary: I have a bunch of unique times and they map to a bunch of other data points, and I just need to put them in a container format
  • note that this is equivalent to []struct{time.Time; interface{}}, but it is easier to write map[time.Time]interface{}
  • a safer alternative is map[*time.Time]interface{}, much like map[*func()]interface{}
• invalid: I want to use a map to deduplicate or merge a list of dates
  • in this case []struct{time.Time; interface{}} is what the effectively get
  • most users want time.Time.Equal (whether they know it or not)

There is an argument about hyrum's law, but since this is just a vet, I am convinced it is not too dangerous. this reads like how we disallow map[func()]interface{}, it is about comparability and the notion of a key set, not just a mapping.

Evidence to answer the false positive question would be great. If this is high enough, vet could consider it. vet probably is not the right home at a 10% false positive rate. We have evidence of bugs that could be caught but no great sense of how many people would be impacted if we turned this on.

example does not have any references to time.Time is there a better corpus to check against?

/cc @richardartoul did a check for this ever make it into gometalinter ?

is not the gometalinter basically dead? and golangci-lint does not define its own lints.

[ianlancetaylor]

Basically it's fine to compare time.Time values with == if you create all the values using functions like time.Date. It's problematic to compare them with == if you create values using functions like time.Now. If we could somehow restrict the vet check to the latter case, I think it would be fine. But I don't see an obvious way to do that.

I have seen code that constructs time.Time values using time.Date and using the time.Time values as map keys. That code is fine. I don't think we want a vet check to fire on that code.

[guodongli-google]

Given the possible confusion people may have, now I am inclined to have a checker for this. Typically, the == case should be replaced by Equal(), as described in Equal. The map case is trickier, as it requires non-trivial code changes.

The remaining problem is whether the incoming StaticCheck suggestion is sufficient, or we need to go deeper, e.g. see whether the Time object comes from time.Now, time.Date, and so on.

In addition, what I am thinking now is whether this can be generalized to other data types than just Time. Such a checker will analyze whether a "Equal" function is defined on a data type, if so then it will discourage (1) using ==, (2) using the data type as a map key, and (3) any other place equality check uses the default struct comparison. If so this could be a good vet checker.

[rsc]

The docs make clear that it is perfectly fine to use time.Time with == and in map keys provided you do it correctly.
I don't see how a checker can tell whether you are doing it correctly, and if it can't, then it will have too many false positives (or no reports).

[rsc]

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

[timothy-king]

I don't see how a checker can tell whether you are doing it correctly, and if it can't, then it will have too many false positives (or no reports).

After @ianlancetaylor 's comment about time.Date, I think this would require showing a flow from a known "bad for equality" source like time.Now() into an == without modification to the Time like UTC(). This is a fairly considerable step up in difficulty compared to warning on every == on type time.Time and will warn on a lot less stuff.