proposal: cmd/vet: reject uses of reflect.DeepEqual on imported types with unexported fields

[ianlancetaylor]

Proposal Details

The reflect.DeepEqual function is simple, and people often use it in tests in preference to more complex packages like github.com/google/go-cmp/cmp. But this can be misleading, and leads to issues like #43986 and #71702.

I propose that we add a vet check warning about uses of reflect.DeepEqual with values of types that are 1) imported; 2) contain references to unexported fields. Those are cases where reflect.DeepEqual is testing the internal state of a type defined in a different package, which means that the call is subject to breaking if the other package changes the type definition.

I believe that this meets vet's requirements for correctness and precision. However, I don't know how frequent the problem is.

I note that there is already a check in vet that warns about uses of reflect.DeepEqual specifically for the type reflect.Value (though I don't know how to actually run that check; it's not part of standard vet). This more general check could perhaps subsume that one.

CC @golang/tools-team

[dsnet]

I'm generally in favor of this, but I think there are still many false positives:

• I often use reflect.DeepEqual on errors from my own package and today deepequalerrors often complains at me.
• I might have a group of packages in a module and I'm the author of all of them, using reflect.DeepEqual is reasonable across those package boundaries. Might we instead want to say that reflect.DeepEqual is permitted on any types in the same module? While a module is worked on by potentially multiple teams and a module-level boundary might be too broad, at least breakages within the module can usually be fixed in the same commit, reducing the harm of a poorly made reflect.DeepEqual check.
• Should we permit reflect.DeepEqual on any type that is comparable? For example, it seems entirely reasonable to use reflect.DeepEqual on netip.Addr since the type is deliberately designed to be comparable.

[zigo101]

This seems too strong. Maybe it is better to just mention the behavior a bit in DeepEqual's doc.

[adonovan]

@randall77 wrote a reflectvaluecompare checker but says he didn't launch it because it had too many false positives; he also has an unfinished CL https://go.dev/cl/308749 to improve it.

[adonovan]

FYI, I plan to write a modernizer to strength-reduce calls to reflect.Value.Compare to use slices.Equal instead when the operands are comparable.

Update: it's https://go.dev/cl/649457

[gopherbot]

Change https://go.dev/cl/649615 mentions this issue: go/analysis/passes/reflectvaluecompare: add main.go

[rogpeppe]

• Should we permit reflect.DeepEqual on any type that is comparable? For example, it seems entirely reasonable to use reflect.DeepEqual on netip.Addr since the type is deliberately designed to be comparable.

I'd say no. All pointer types and interface types are comparable, but I don't think that's any guide to whether we should be using DeepEqual with them. If you mean by-value comparable types, I think I'd still say no: a by-value comparable type might still hold unstable contents, and in fact #43986 is about exactly such a type.