Implement an AllowAllUnexported Option

[jeromefroe]

Hi, I was wondering if you would consider adding a new option which would apply the AllowUnexported Option to all types and would not require a slice of input like the current AllowUnexported function does?

My use case is that I have a lot of tests which use the github.com/stretchr/testify package, which ultimately uses reflect.DeepEqual for comparing two objects, that break in Go 1.9 because they try to compare structs which contain time.Time fields. Admittedly, I could define an Equal method for all such structs but that doesn't seem to address the root of the problem to me, which is that I would like reflect.DeepEqual functionality (which compares all fields, both private and public) with the caveat that I would like to check if the types being compared have an Equal method for testing equality. Furthermore, while for structs I could pass the structs being compared to AllowUnexported, I would like the same behavior for composite types as well (e.g. maps and slices) which is not supported by AllowUnexported currently since it specifically checks that the types of the objects passed to it are structs.

I would be more than happy to put up a PR for such an Option, a first look at the code leads me to believe that we could define an additional field on state indicating that we want to always check private fields. Then, when set to true, we would always extract a struct's fields in tryExporting. With that being said, I got the impression that you might be opposed to such an Option given the warnings for AllowUnexported already and so I thought it better to open an issue first where we could discuss.

Thanks!

[dsnet]

AllowUnexported was a controversial option during the development of cmp. When dealing with unexported fields, there are several possible policies:

1. Ignore unexported by default; require use of AllowUnexported to actually compare them
2. Panic on unexported by default; require use of AllowUnexported to actually compare them
3. Always check unexported by default (AllowUnexported doesn't exist)

Policy 1 would cause failing tests to unexpectedly pass because cmp.Equal is not comparing a field the user thought they were comparing. So this idea was dropped early on.

Policy 2 (the camp I fall under), goes by the philosophy that the test writer should know explicitly what they are comparing, and provide an explicit whitelist (via AllowUnexported). Note that an unexported field added to a struct that formerly had only exported fields can cause cmp to panic. However, analysis of code history showed that this was rare, and if it did happen, I believe the test writer should think carefully about how they want to handle this failure. Fixing the test for these rare cases is probably a 1-line change to where they invoke cmp.Equal, which I am okay with (before cmp, I often had to write dozens if not hundreds of lines to fix a bad comparison).

Policy 3, goes by the philosophy that comparing on unexported fields generally works (even if unexported fields have absolutely no compatibility guarantees). In the event where it would have failed (e.g., time.Time, or proto.Message), then cmp can use the time.Time.Equal method or the user can explicitly pass in cmp.Comparer(proto.Equal). One downside is that this always requires use of unsafe, which is problematic for AppEngine, GopherJS, and other specialized Go environments.

For the time being, I took the stance of policy 2, but am sympathetic to policy 3. However, I want to be driven by more data that policy 2 is causing more problems than it is solving.

The use case you are suggesting seems to be one where you want to provide a blanket way to compare all types, which was precisely the type of problem we were trying to avoid with this package. I heavily believe users must know what they are comparing and should take at least some minimal steps to ensure that their comparisons are forward compatible.

That being said, if you want to experiment with an AllowAllUnexported like option, try using DeepAllowUnexported:

func DeepAllowUnexported(vs ...interface{}) cmp.Option {
	m := make(map[reflect.Type]struct{})
	for _, v := range vs {
		structTypes(reflect.ValueOf(v), m)
	}
	var typs []interface{}
	for t := range m {
		typs = append(typs, reflect.New(t).Elem().Interface())
	}
	return cmp.AllowUnexported(typs...)
}

func structTypes(v reflect.Value, m map[reflect.Type]struct{}) {
	if !v.IsValid() {
		return
	}
	switch v.Kind() {
	case reflect.Ptr:
		if !v.IsNil() {
			structTypes(v.Elem(), m)
		}
	case reflect.Interface:
		if !v.IsNil() {
			structTypes(v.Elem(), m)
		}
	case reflect.Slice, reflect.Array:
		for i := 0; i < v.Len(); i++ {
			structTypes(v.Index(i), m)
		}
	case reflect.Map:
		for _, k := range v.MapKeys() {
			structTypes(v.MapIndex(k), m)
		}
	case reflect.Struct:
		m[v.Type()] = struct{}{}
		for i := 0; i < v.NumField(); i++ {
			structTypes(v.Field(i), m)
		}
	}
}

Thus, to perform the equivalent of AllowAllUnexported, you would do:

cmp.Equal(x, y, DeepAllowUnexported(x, y), cmp.Options(opts))

[jeromefroe]

@dsnet thanks! DeepAllowUnexported looks to be exactly what I want, I'll give it a try.

Furthermore, I understand and agree with the concerns that users should know exactly what they are testing. With that being said though there are already many tests (any tests using the github.com/stretchr/testify package being but one example) that rely on reflect.DeepEqual for testing equality between types, and thus rely on comparing private fields and it's hard for me to envision migrating those tests from using reflect.DeepEqual to using go-cmp without providing a feature like DeepAllowUnexported.

[mikedanese]

Any chance of DeepAllowUnexported being added to cmpopts?

[dsnet]

As a counter-proposal to AllowAllUnexported, I believe a better approach is to extend AllowUnexported to allow whitelisting a package path namespace. We could define a PackagePath string type that is specially allowed in AllowUnexported, where unexported fields is allowed in types defined in any package where the PackagePath is a path prefix match.

For example, AllowUnexported(PackagePath("github.com/alice/example")) allows unexported fields to be used in any types found in:

• github.com/alice/example
• github.com/alice/example/foo

but not struct types types found in:

• github.com/alice/example2
• github.com/bob/example

Note: although not advertised, AllowUnexported(PackagePath("")) would be equivalent to the originally proposed AllowAllUnexported.

However, I don't want to act on my proposal yet. The cmp package is a little over a year old and I don't think enough use of it has happened to really inform the right approach to take here.

[nhooyr]

How about all unexported fields of structs in the current package (from where cmp was called) are allowed by default. This seems really natural to me. It would ensure they do not depend on private fields from external packages but still allow them to compare private fields in the current package, where they most often would expect to do so.

[dsnet]

How about all unexported fields of structs in the current package (from where cmp was called) are allowed by default. This seems really natural to me.

I can see the benefit of doing this and we considered this approach during the development of cmp, but there are some implications:

• The package with the test may not be where the type is declared. This is common in sufficiently large repositories (e.g., Kubernetes or golang/protobuf), where it is sensible to have access to unexported fields for any type declared in the repository since you are the owners of those types. Thus, package boundaries seem like the wrong granularity, but something more akin to module boundaries.
• Moving the type (via type aliases) or moving the test results in unexpected test breakages.
• This functionality relies on runtime magic to work, which makes me uncomfortable. That is, there isn't a way to reliably correlate the stack frame of the caller with some Go package. The implementation of how AllowUnexported works relies on the unsafe package, which I'm already uncomfortable with.

[nhooyr]

Why does AllowUnexported require the unsafe package? reflection allows you to read unexpected fields, just not modify them.

Regarding the comparison of unexpected fields, I think the present approach is too restrictive. Any time its bad idea to compare a struct directly, library authors will usually provide a Equal() method. I cannot think of a single time this was not true for me. With clients using this library, library authors can always add an Equal method at a later date if they need custom comparison logic and things will just work.

Given == and reflect.DeepEqual both compare unexported fields by default, the behaviour of this library is also very unexpected and makes using this library comparatively verbose.

Given the verbosity and surprise with the behaviour of this library in regards to unexported fields and in my opinion, relatively little benefit, I'd say its better to allow unexported fields to be comparable by default.

[dsnet]

Why does AllowUnexported require the unsafe package? reflection allows you to read unexpected fields, just not modify them.

Reflection only allows you to interact with unexported fields through the reflect API, but does not allow you to obtain an interface{} value for the underlying value (i.e., calling reflect.Value.Interface panics). This functionality was originally added only for human debugging so that fmt could print unexported fields. However, the cmp package does more than just interact with unexported fields through the reflect API. It needs the ability to call methods on the type and pass the type to other functions (e.g., transformers and comparers). Thus, unsafe is necessary.

In regards to the rest of your comment, it may be time to re-evaluate the policy on unexported fields now that more usage of cmp exists. However, decisions are best done on hard data, rather than subjective thoughts or assumptions. I'll do static analysis on existing usages of cmp when I'm back in the office later this month.

[dsnet]

I analyzed a large corpus of Go code without tens of thousands of imports of cmp and I believe the current policy is working as intended.

Of all the usages cmp.AllowUnexported, approximately 98% are using them correctly (i.e., on types that are clearly in control of the user). This seems to me to suggest that the current policy is doing exactly what it was intended to do. That is, it is properly warning users about comparing unexported fields and getting them to deliberately say they want to compare on the type OR getting them to provide a proper custom comparer for the type. As such, I'm inclined to say that the data supports keeping the current policy.

That said there is a minority that wants the ability to compare all unexported regardless of the risks it presents. As such, I'm thinking about adding support for #40 (comment), which would also allow the user to explicitly say that they want to compare on all unexported fields.