4 ms·
This does not appear to be undefined at all. The issue is using reflect.DeepEqual to compare structs with unexported fields. The fields are an implementation d
by xyzzy_plugh 5y ago
This does not appear to be undefined at all.
The issue is using reflect.DeepEqual to compare structs with unexported fields. The fields are an implementation detail, and comparing time.Time values is generally incorrect.
Here are the actual docs from https://pkg.go.dev/time#Time.Equal https://pkg.go.dev/time#Time.Equal
> Equal reports whether t and u represent the same time instant. Two times can be equal even if they are in different locations. For example, 6:00 +0200 and 4:00 UTC are Equal. See the documentation on the Time type for the pitfalls of using == with Time values; most code should use Equal instead.
and
> Time instants can be compared using the Before, After, and Equal methods. The Sub method subtracts two instants, producing a Duration. The Add method adds a Time and a Duration, producing a Time.
and
> 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.
If I were reviewing this code, the bug squarely lands on the shoulders of the author who chose to use reflect.DeepEqual (or similar function available in many test helper libraries) here. Frankly, I find code that uses such blind comparisons to be lazy and prone to bugs exactly like this one.
For readers unfamiliar with Go, this pattern is not unique to the time library. In general you avoid these sort of comparisons, at least without reading the docs. It's one thing if it's your own type or a fully-exported type or something obvious but this is just so clearly wrong.
Furthermore I find this entire blog in pretty bad taste. Read the docs, own up to your own mistake, don't make blind assumptions about data types, don't lean on reflection to be lazy. Trying to throw shade at the Go authors crosses the line for me here, and name-calling Rob Pike is ultra immature.
Trying to be cute, I guess? But ultimately I now think poorly of this business and its engineers. Bad form, folks.
- cinntaile 5y ago> Trying to throw shade at the Go authors crosses the line for me here, and name-calling Rob Pike is ultra immature. The whole blogpost is written in a tongue in cheek fashion, I don't think it's meant as name-calling.
- RVuRnvbM2e 5y agoAbsolutely spot on. And just to pile on a bit more, this part of the blog post gives me the heebie jeebies: There are some actual foot-guns hidden in the runtime: * Reads from a nil map are fine, writes panic * Reads from a closed channel block, sends on a closed channel panic * nil sometimes does not == nil * Any panic in any goroutine kills the entire process * Can't safely store references to loop variables Calling these footguns is an exaggeration. All are natural outcomes of the design of the language. * Zero values are a key concept in Go, and avoid uninitialised memory. Reading from a map returns the zero value of its keys. Writes panic because there is no underlying object. * Channels are another key concept in the synchronisation of goroutines. Block/panic behaves this way because these are programming logic errors. * Equality of interfaces (another key concept) includes the dynamic type. * I'm confused about what the blog author expects from `panic()`? * This is the same behaviour as any language with pointers, such as C. Pretty fundamental stuff. If the author finds this list surprising I wonder about the quality of their software. And this is just cringingly arrogant: > stumped an entire room of seasoned industry veterans for a solid hour
- amw-zero 5y agoThe main problem I got out of the article was that the stateful timezone of the entire process does not get set until `time.Format` gets called. I think that's a legitimate criticism - which part made you think that `reflect.DeepEqual` is at fault?
- xyzzy_plugh 5y agoDeepEqual is simply inappropriate here. It's fundamentally problematic to do deep comparisons on types you don't have visibility into. You're not entitled to manage the fields, so you lack any guarantee that the implementation matches your expectations. Maybe comparisons require holding a lock, maybe there's a pointer to a shared object (like in this instance), maybe there's some reference to CGo stuff. It's just not the right way to go about it. If the language lacked reflection, how would you solve this problem?
- amw-zero 5y agoCan you try addressing what I said? You’re really hung up on the deep equality check. As I said, I don’t think that’s the root of the issue. The root issue is time.Format changing the global time zone unexpectedly. Do you have an opinion on that?