Open dominiquelefevre opened 9 months ago
Hey, anybody out here? @dsnet could you please look at the patch?
The cmp
package is a testing-first package, so precise printing of types is a high priority.
I'm not sure I see how this helps since value.TypeString
is still transitively called from cmp.Diff
.
k8s.io/kube-openapi
does not use cmp.Diff()
itself. It happens to depend on cmp.Option
which brings the following dependency chain:
cmp.Option
's filter()
depends on cmp.state
,cmp.state
brings in cmp.Path
which brings in cmp.pathStep
,cmp.pathStep
's method String()
depends on TypeString()
.Can you point at concrete code? It's a bit esoteric to me that code would depend only on cmp.Option
but not but not the rest of cmp
functionality.
GRPC turns out to have an accidental use of go-cmp outside tests, too. It uses cmp.Equal()
. The latest current release, 1.58.1, has this: https://github.com/grpc/grpc-go/blob/863de7347326bdca22f6084e3ae28e8fa96d01ca/balancer/grpclb/grpclb_remote_balancer.go#L58
After this PR, GRPC no longer imports Method()
via go-cmp.
These both seem like cases that are better addressed by the place misusing cmp
.
In the case of k8s.io/kube-openapi
, I don't see why that package is taking a dependency on cmp
just to provide the SwaggerDiffOptions
var. There's nothing in that var that a caller can't construct themselves. If the package wants to make the Ref
type easily comparable by cmp
, it could provide a Ref.Equal
method.
In the gRPC case, the use of cmp.Equal
to compare two slices of proto.Message
is very heavyweight. I'd replace it with a more directed comparison. (Whenever gRPC is happy requiring Go 1.21, slices.EqualFunc
would be the simplest replacement.)
These are very reasonable arguments. The use in kube-openapi looks like a bug to me and I've filed them a PR. The use in gRPC, although inefficient, looks legit to me.
What bothers me more is that we are talking about a whack-a-mole solution. You've implemented some really useful features, and people will use them without thinking twice about go-cmp being mostly a package for tests. Kube-openapi, kube-apiserver and gRPC are just examples that were really trivial to find. There certainly are more. I think it is important to minimise the damage that go-cmp causes when used less than prudently. Especially given that disabled DCE seriously pessimises the code generation.
The cmp
documentation is quite explicit that it's designed for use in tests, and is not suitable for use in production contexts:
It is intended to only be used in tests, as performance is not a goal and it may panic if it cannot compare the values.
That line undersells "performance is not a goal", however. For example, that gRPC use of cmp.Equal
will compare each individual list item twice, in separate goroutines, to verify that the Comparer
is symmetric, deterministic, and does not modify its parameters.
This is quite useful for catching inadvertent errors in tests, but is really not suitable behavior for any production context.
If there were a build tag to isolate tests, I'd be more open to something like this, but as it stands we're going to prioritize good reporting of types in tests over reduced binary bloat.
Avoid calls to value.TypeString() that do not request fully qualified type names. The implementation of TypeString() depends on getting methods by index which disables the DCE in the compiler.
k8s.io/kube-openapi and k8s.io/apiserver happen to depend on go-cmp outside tests so it is important that go-cmp does not inhibit the DCE inadvertently.