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.

[nhooyr]

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.

It's not clear to me how the data proves the current policy is correct. There aren't many situations where it actually protects you from doing something you don't want to do. The data makes this clear as 98% of the time, the types were in control of the user so cmp.AllowUnexported was boilerplate.

The other 2% of the time, I don't think its fair to the usage was incorrect. It could very well have been correct if the type implemented Equal or was just a data type that is ok to compare. Sometimes this is documented, sometimes not, but its usually pretty clear. If it isn't supposed to be comparable, eventually their comparison will probably fail, which is fine. They were probably doing something they weren't supposed to do. The current policy would make this clear to them immediately but at the cost of being boilerplate at least 98% of the time, probably more. I don't think thats worth it.

Furthermore, the tradeoff with the current policy is to favour library authors at the expense of users but there are usually more library users than authors. Thus this seems to be the wrong tradeoff.

[dsnet]

Had the number of improper usages of AllowUnexported been higher (>20% or something), then that indicates that people are using it improperly anyways and we might as well make comparing unexported fields the default behavior. However, that is not the case.

Also, there were twice as many custom Comparer options as AllowUnexported where most of them were comparing on a type that the user did not own, but were properly written in terms of the public API of the remote type. I contend that users knew to add these custom Comparer options in the first place because they saw the panic on the unexported field (of course this assertion is difficult to prove).

It could very well have been correct if the type implemented Equal or was just a data type that is ok to compare

The discussion here should not involve the Equal method since if every type had it, we wouldn't even need to define a policy on how unexported fields are compared.

Furthermore, the tradeoff with the current policy is to favour library authors at the expense of users but there are usually more library users than authors. Thus this seems to be the wrong tradeoff.

Yes, the current policy favors library authors. More users than authors does not imply that user convenience is the right policy. The fact that there are more users of a library than the authors of the library suggests that the library author carry more weight than the individual users.

[nhooyr]

Had the number of improper usages of AllowUnexported been higher (>20% or something), then that indicates that people are using it improperly anyways and we might as well make comparing unexported fields the default behavior. However, that is not the case.

I don't follow this logic. This just proves it's easy to use, not that its the right policy. After all, its just adding an option.

Also, there were twice as many custom Comparer options as AllowUnexported where most of them were comparing on a type that the user did not own, but were properly written in terms of the public API of the remote type. I contend that users knew to add these custom Comparer options in the first place because they saw the panic on the unexported field (of course this assertion is difficult to prove).

Interesting. Could you show me an example of when someone needed a custom Comparer? The only time I ran into using a custom comparer was for comparing err.Error() of two different errors instead of their concrete values. I'm curious to see why they needed it and if AllowUnexported would have been just as good.

The discussion here should not involve the Equal method since if every type had it, we wouldn't even need to define a policy on how unexported fields are compared.

That is true but that wasn't the purpose of me saying that. We should ignore incorrect usage results in the analysis involving types with a Equal method because they have a defined comparison for their private fields.

Yes, the current policy favors library authors. More users than authors does not imply that user convenience is the right policy. The fact that there are more users of a library than the authors of the library suggests that the library author carry more weight than the individual users.

Why does the library author carry more weight when the library has more users? All it shows is the library author has a popular library, and if anything, a lot of code is being written using it and thus the boilerplate should be minimal.

Also how exactly did you conduct your analysis? What is a type a user owned versus one that isn't?

Interesting related discussion at: golang/go#30450

[dsnet]

Why does the library author carry more weight when the library has more users?

An improvement to a widely used library benefits most (if not all) users, while more users of a library does not bring direct benefit back to the library (or other users). Allowing users to depend on unexported fields by default prevents library authors from being able to make changes to the internal representation of their packages. I am the owner of several widely used packages and I see this problem happening over and over again.

An example might be something like this:

1. Users report performance issues with the author's library (doesn't have to be performance related).
2. The author optimizes some internal code and data structures associated with that code (i.e., unexported fields of types owned by the author). The change makes no externally visible changes and should be safe to deploy.
3. Most users are really happy with the change. Yay!
4. A small set of users are angry about the change because it broke their tests. They accuse the author of "violating compatibility" and the author spends a significant amount of time explaining to each user that they were depending on undefined behavior.
5. What to do? Does the author roll-back the change or does the author roll-forward and break users?
   • If rolling back, then the majority of users who want the change do not benefit
   • If rolling forward, then a small set of users are potentially broken. Even worse, the broken test may have been written long ago, where the current maintainers of that test may not even remember what the test was testing for or how it works.
     • It also adds to ill-feelings between the user and the author, where the user often feels that they were slighted by the author because "something that formerly worked is now broken" and since the author made the recent change, it must be their fault. On the other hand, the author is technically in the right since most libraries follow the Go1 compatibility promise or something like it, where unexported behavior is the author's right to change.

The current policy requires all users to pay a small up-front cost either by:

• Using cmp.AllowUnexported to declare that they do own the type and that it is safe to compare on the unexported fields.
• Using cmp.Comparer or cmp.Transformer to provide a custom functionality that properly compares the external type based on the public API of that type. If it turns out that comparing that type is lots of work, perhaps the right thing to do is to file an issue with the author of that type to add an Equal method or custom cmp.Option convenience helpers so that everyone may benefit.
  • For example, generated protobuf messages have internal fields that are definitely not safe to compare. The protobuf authors are also well aware that comparing data structures with messages is a pain. For that reason, a protocmp package is in the works that provides the ability to compare protobuf messages in a cmp-compatible way.
  • The cost to the user is the addition of a protocmp.Transform() option. The benefit is the freedom of the protobuf authors to make internal changes that benefit performance for everyone.

By having the user be explicit about what they are comparing is to their benefit. It greatly reduces the chances of being broken by upstream changes and more clearly codifies what the test was even testing for. It is far better to have breakage resistance today than to break later on and try to fix an issue long after you have forgotten about the code.

In social sciences, this phenomenon is known as the "Tragedy of the Commons", which describes "a situation in a shared-resource system where individual users acting independently according to their own self-interest behave contrary to the common good of all users". Analogue to our situation:

• the "shared-resource" is a widely-used library
• the "individual user" are users of the of the widely-used library
• the "self-interest" of the user is the ability to perform comparisons with minimal thought and effort (e.g., compare unexported fields by default)
• the "common good" are improvements to the widely-used library (either through performance benefits, new features, etc).

Allowing unexported fields by default is convenient for users (and towards their individual self-interest), but it is unhelpful for the overall ecosystem since it allows for the creation of brittle tests that break on changes in dependencies that otherwise should not matter to the user. This slows down or prevents library improvements and is contrary to the good of all users.

I hope this explanation convinces you, but I also recognize that the choice of policy is also subjective. This is a fundamentally a social sciences problem where there is no "right approach".

Also how exactly did you conduct your analysis? What is a type a user owned versus one that isn't?

Unfortunately I can't release the data, but type "ownership" was a defined by a heuristic that compared the import paths for the type and where it was called from. For open-source code, if the type is declared in the same repo as the where the test, then the test owner owns the type. Within Google (which uses a massive mono-repo), ownership was fuzzily determined as long as the two paths shared a sufficiently long path prefix.

[nhooyr]

I really appreciate the thorough response.

I hope this explanation convinces you, but I also recognize that the choice of policy is also subjective. This is a fundamentally a social sciences problem where there is no "right approach".

I don't maintain widely used libraries but I do read a lot of GitHub and frequently contribute. I personally have never seen an issue with private field comparisons being broken after an update. It'd help if you could link me to such an issue.

But regardless, I'll defer to your experience. If you have seen it happen often, then yes the current policy does make sense.

[dsnet]

At least two issues come to mind:

• (time: use monotonic clock to measure elapsed time golang/go#12914) time: use monotonic clock to measure elapsed time
  • Adopting this proposal modified the unexported fields of the time.Time struct. Within Google, that change alone broke several hundred tests that were all using reflect.DeepEqual on data structures that contained a time.Time within them. Here, reflect.DeepEqual's default behavior of comparing unexported fields made it too easy for users to depend on the unexported fields of time.Time in a way that "kinda worked, but not really". Fortunately, time.Time has an Equal method, which meant we could switch almost all of these broken tests to cmp.Equal with little issue.
  • Monotonic timestamps were a greatly desired feature, and the lack of them was the result of an outage at CloudFlare. The Go community needed this change, but the release of Go1.9 was delayed. One reason was because someone (me) had to fix all of the broken tests in Google since our release policy dictates that Google is running the next version of Go in production before for RC1 can be cut. If enough tests broke for Google, then any sufficiently large corporation that wants to use Go1.9 would also run into a large number of failures. They then also need to fix failures for their own tests in addition to those within their dependencies (much harder).
• (proposal: Go 2 error values golang/go#29934) proposal: Go 2 error values
  • This is still in proposal phase, but one complaint is that it breaks users of reflect.DeepEqual. Again, here the default behavior of reflect.DeepEqual in comparing the unexported fields of errors.errorString meant that it gave an illusion of equality for errors that was never guaranteed. In fact, errrors.New returns a pointer to a string rather than the string itself precisely so that == on errors would only be true for the exact same error, rather than two errors that happened to have the same string.
  • Here's another example where reflect.DeepEqual's comparison of unexported fields causes users to be reliant on internal details of a type. On one hand, the Go community really needs better support for error value (being one of the top items on each Go survey). On the other hand, broken tests due to invalid usages of reflect.DeepEqual poses a risk and dampener on the adoption of a new and better world (for errors).
  • The proposed "solution" of modifying the definition of reflect.DeepEqual to work with errors.errorString is a complete hack to work around this problem within the Go ecosystem. All other large scale projects that run into issues because people use reflect.DeepEqual on their data types do not have this luxury of hacking the standard library.

There are many more minor issues filed, but finding them is like finding needles in a haystack.

[nhooyr]

Those issues are more regarding misuse of reflect.DeepEqual which is imo orthogonal to our discussion regarding cmp.Equal as cmp.Equal supports adding a Equal method which would have alleviated both issues easily.

[dsnet]

The discussion here is regarding whether cmp should compare unexported fields by default, for which reflect.DeepEqual is a very relevant example of what not to do.

The presence of the Equal method happened to make the transition for time.Time easier, but doesn't change the fact that people should not have been comparing upon the unexported fields of time.Time.

We could add an Equal method to errors.errorString and that may help users of cmp out, but that doesn't change the fact that comparing on the unexported fields of errors.errorString is a mistake and violation of the Go1 compatibility agreement.

The question is what to do when a type lacks an Equal method, and I still argue that defaulting to unexported field comparison is a mistake.

[nhooyr]

The discussion here is regarding whether cmp should compare unexported fields by default, for which reflect.DeepEqual is a very relevant example of what not to do.

I disagree. It's a much bigger problem with reflect.DeepEqual because you can't override the definition of equality. You can easily do so with cmp.

The presence of the Equal method happened to make the transition for time.Time easier, but doesn't change the fact that people should not have been comparing upon the unexported fields of time.Time.

But they wouldn't have been doing so if they were using cmp which makes the example less convincing.

We could add an Equal method to errors.errorString and that may help users of cmp out, but that doesn't change the fact that comparing on the unexported fields of errors.errorString is a mistake and violation of the Go1 compatibility agreement.

Likewise here. Wouldn't happen with cmp as you'd just add a Equal method.

The question is what to do when a type lacks an Equal method, and I still argue that defaulting to unexported field comparison is a mistake.

In my experience, there are very few comparisons where there is no Equal method, a type has private fields and it's also being compared by callers. It isn't worth considering. I understand you've found this to be different in your experience, but the examples you've linked to would not be a problem if callers used cmp.

[dsnet]

You can easily do so with cmp.

Under that same argument, the current policy is reasonable since anyone can easily adjust cmp to compare unexported fields if they want to. We provided AllowUnexported for a reason.

Wouldn't happen with cmp as you'd just add a Equal method.

But errors.errorString doesn't have an Equal method. Yes, we could add one, but this argument could be made for literally every other type in Go, where the solution is "just to add an Equal method".

Also, I don't think we should add an Equal method to errors.errorString because nobody passes that type around directly. Instead, it is almost always stored within the error type. If someone compares the error type with cmp today, they will likely hit an unexported field panic. Good. They see the panic and add a custom comparer properly handling the error (which may be a simple ==, involve some error unwrapping, or checking that an error occurred regardless of the exact error). At least the user was made aware of the issue and will need to make it clear how they expected those two errors to be handled.

In my experience, there are very few comparisons where there is no Equal method

If that's you're perspective, then why bother arguing for cmp to compare unexported fields by default if it doesn't matter for most cases? Supposing "very few" types lack an Equal method, then "very few" people should be running into problems with unexported fields, and we're wasting our time here debating something that occurs "very few" times.

Unfortunately, a cursory glance (both in Google and the open-source repos) of types defined in Go reveals that most types actually do not have an Equal method. Within the Go toolchain itself, there are close to 10k defined types (80% of which are structs). Of all of those, only 6 have Equal methods (less than 0.1% of all types).

I alluded to this earlier, but "just adding an Equal" method is not always the solution. For example, generated protocol buffer messages will not have a generated Equal method because there is no universal agreement upon the definition of equality. Rather, there will be a protocmp package that transforms protobuf messages to be compatible with the cmp package (which users can then further customize with cmp.Options to suit their needs). That's another example of why panicking on unexported fields is a good thing. The user sees that cmp barfed on an unexported field in their protobuf message and it forces them to do the right thing (which is to add the protocmp.Transform() option).

[nhooyr]

Under that same argument, the current policy is reasonable since anyone can easily adjust cmp to compare unexported fields if they want to. We provided AllowUnexported for a reason.

That's where we disagree, its not easy. Its verbose and messy and the general solution you've provided here works well (we use it at @cdr) but it's fairly verbose as well even if it can be wrapped in a function.

But errors.errorString doesn't have an Equal method. Yes, we could add one, but this argument could be made for literally every other type in Go, where the solution is "just to add an Equal method".

What's wrong with that argument? If a type has private fields but there is an expectation that it will be compared, which there is for all the types we've discussed, an Equal method should be provided and thus cmp will handle it correctly.

If that's you're perspective, then why bother arguing for cmp to compare unexported fields by default if it doesn't matter for most cases? Supposing "very few" types lack an Equal method, then "very few" people should be running into problems with unexported fields, and we're wasting our time here debating something that occurs "very few" times.

I apologize, I forgot to clarify that the other condition is the comparison is illegal. Most of the times when a type lacks an equal method, has private fields and is being compared, the comparison is legal.

However, after all this time has passed, I'm not strongly in favour of allowing private comparisons by default. Most of the time I'm comparing private fields are within the same package where I think it'd be a good compromise for cmp to allow such comparisons. It'll be very convenient yet still provide sufficient protection.

[dsnet]

However, after all this time has passed, I'm not strongly in favour of allowing private comparisons by default. Most of the time I'm comparing private fields are within the same package where I think it'd be a good compromise for cmp to allow such comparisons. It'll be very convenient yet still provide sufficient protection.

It seems that #116 will satisfy your use case. While I'm still opposed to an AllowAllUnexported function, I'm in favor of an function that allows comparing unexported fields under a certain package path prefix.

I'm still not satisfied with the API of #116 and hopefully will get around to thinking about it more soon.

[nhooyr]

It seems that #116 will satisfy your use case. While I'm still opposed to an AllowAllUnexported function, I'm in favor of an function that allows comparing unexported fields under a certain package path prefix.

It's similar but what I'm arguing for is that by default, if the type being compared is in the same package as where the comparison is taking place, it should be ok to allow unexpected fields to be compared.

[dsnet]

It's similar but what I'm arguing for is that by default, if the type being compared is in the same package as where the comparison is taking place, it should be ok to allow unexpected fields to be compared.

I understand the sentiment, but I don't know how to implement that:

1. It requires calling runtime.Callers to determine the caller. This dependency makes me feel uncomfortable as it depends on runtime details that not every implementation of Go may support.
2. The runtime.Frame doesn't carry sufficient information to correlate the caller with the type information. There are similarities between the caller's source file and the package path of the type, but they are not the same thing.
3. The "right" caller is not obvious. In most cases, the cmp package is directly called from the user's test code. However, there are cases where cmp is used from a helper test package. In that situation, the direct caller is not the "right" caller, but caller of the caller.

[nhooyr]

It requires calling runtime.Callers to determine the caller. This dependency makes me feel uncomfortable as it depends on runtime details that not every implementation of Go may support.

Is there any implementation of Go that does not support it? It'd make debugging hell.

The runtime.Frame doesn't carry sufficient information to correlate the caller with the type information. There are similarities between the caller's source file and the package path of the type, but they are not the same thing.

We can use Function which is documented as the package path qualified name of the function.

The "right" caller is not obvious. In most cases, the cmp package is directly called from the user's test code. However, there are cases where cmp is used from a helper test package. In that situation, the direct caller is not the "right" caller, but caller of the caller.

Fair point, I had not considered this. However, I think it'd be ok if the policy was that if there is any function in the list of callers that shares the same package as the type being compared, then comparing unexpected fields is ok.

[nhooyr]

Also for when the Frame is not known or Function is empty, we can just fallback to the current behaviour.

[dsnet]

Is there any implementation of Go that does not support it? It'd make debugging hell.

Yes (and it does make debugging hell). It's not so much that the implementation doesn't support it, but rather that it's implementation is buggy or incomplete. Accurate stack frame information is (unfortunately) more often considered a nice-to-have rather than a necessity of proper program functioning.

We can use Function which is documented as the package path qualified name of the function.

That field is only intended for human debugging. It doesn't have a formally specified grammar that can be always parsed in an unambiguous way. For example, is foo/bar/pkg.v1.Foo to be parsed as the Foo function in foo/bar/pkg.v1 package or the v1.Foo method in the foo/bar/pkg package?

Also for when the Frame is not known or Function is empty, we can just fallback to the current behaviour.

I'm not fond of any approach that has different behavior depending on where it's run from or what tool is used to build it.

[nhooyr]

Yes (and it does make debugging hell). It's not so much that the implementation doesn't support it, but rather that it's implementation is buggy or incomplete. Accurate stack frame information is (unfortunately) more often considered a nice-to-have rather than a necessity of proper program functioning.

Makes sense. We can just fall back to the current behaviour though since most users will use a Go implementation with accurate frame information.

That field is only intended for human debugging. It doesn't have a formally specified grammar that can be always parsed in an unambiguous way. For example, is foo/bar/pkg.v1.Foo to be parsed as the Foo function in foo/bar/pkg.v1 package or the v1.Foo method in the foo/bar/pkg package?

We don't need to accurately parse the package from that field. Just need to check whether it matches the package in the caller i.e foo/bar/pkg.v1.Foo could refer to a caller in package foo/bar/pkg.v1 or in foo/bar/pkg but not both. If we compare against the package path of the type and find a match for all path segments and a prefix for the last segment, then that should work.

I'm not fond of any approach that has different behavior depending on where it's run from or what tool is used to build it.

I agree in general but in this case, I'd argue it's an intuitive extension of Go's package public/private model. For most users, it'll just work transparently and as expected.

[nhooyr]

Even the package prefix function in #116 would suffer from the same faults but I think it'd be worse as you'd have to manually update the package path when renaming/moving.

[dsnet]

I sent out #176 which should address everything requested here. It simply adds an Exporter option that takes in a func(reflect.Type) bool that specifies whether to permit exporting that type.

An AllowAllUnexported option can trivially be implemented with:

Exporter(func(reflect.Type) bool { return true })

An exporter that is dependent on the current call stack can also be implemented in terms of this since it's just a function.

[nhooyr]

Seems good to me.

[thockin]

Is there an equivalent trick to get "IgnoreAllUnexported" semantics?