Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add FilterPriority to prioritize some set of options over another #36

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dsnet
Copy link
Collaborator

@dsnet dsnet commented Aug 3, 2017

EDIT (2017-08-09): Renamed Continue as Default

Added API:

Default() Option
FilterPriority(opts ...Option) Option

FilterPriority returns a new Option where an option, opts[i],
is only evaluated if no fundamental options remain after applying all filters
in all prior options, opts[:i]. In order to prevent further options from being evaluated,
the Default option can be used to ensure that some fundamental option remains.

Suppose you have a value tree T, where T1 and T2 are sub-trees within T.
Prior to the addition of FilterPriority, it was impossible to do certain things.

Example 1: You could not make the following compose together nicely.

  • Have a set of options OT1 to affect only values under T1.
  • Have a set of options OT2 to affect only values under T2.
  • Have a set of options OT to affect only T, but not values under T1 and T2.
  • Have a set of options O to affect all other values, but no those in T (and by extension those in T1 and T2).

Solution 1:

FilterPriority(
	// Since T1 and T2 do not overlap, they could be placed within the
	// same priority level by grouping them in an Options group.
	FilterTree(T1, OT1),
	FilterTree(T2, OT2),
	FilterTree(T, OT),
	O,
)

Example 2: You could not make the following compose together nicely.

  • Have a set of options O apply on all nodes except those in T1 and T2.
  • Instead, we want the default behavior of cmp on T1 and T2.

Solution 2:

FilterPriority(
	// Here we show how to group T1 and T2 together to be on the same
	// priority level.
	Options{
		FilterTree(T1, Default()),
		FilterTree(T2, Default()),
	},
	O,
)

Example 3: You have this: type MyStruct struct { *pb.MyMessage; ... }

  • Generally, you want to use Comparer(proto.Equal) to ensure that all proto.Messages within the struct are properly compared.
    However, this type has an embedded proto (generally a bad idea),
    which causes the MyStruct to satisfy the proto.Message interface and
    unintentionally causes cmp.Equal to use proto.Equal on MyStruct, which crashes.
  • How can you have Comparer(proto.Equal) apply to all other proto.Message without applying just to MyStruct?

Solution 3:

FilterPriority(
	// Only for MyStruct, use the default behavior of Equal,
	// which is to recurse into the structure of MyStruct.
	FilterPath(func(p Path) bool {
		return p.Last().Type() == reflect.TypeOf(MyStruct{})
	}, Default()),

	// Use proto.Equal for all other cases of proto.Message.
	Comparer(proto.Equal),
)

@dsnet
Copy link
Collaborator Author

dsnet commented Aug 3, 2017

\cc @bcmills @neild

This is an idea that I have been kicking around in my mind for a while. One of the major functionality lacking from cmp is the ability to say "these options apply here, but not over there". The solution I came up with is a form of explicit priority ordering without requiring the user to specify priority numbers or anything. (Bryan, this is the idea that we briefly spoke about at GopherCon).

The implementation is nice that it only involves adding a new type priorityFilter with it's own implementation of the filter method. Only Options.filter needs to be updated to understand the concept of noop.

I'm not happy with the name Continue, so any suggestions for that "noop" option is welcome.

This pull request has no tests! I will add them if we think these options are a good idea.

@dsnet dsnet changed the title Add Options to prioritize some set of options over another Add PriorityFilter to prioritize some set of options over another Aug 3, 2017
@neild
Copy link
Collaborator

neild commented Aug 3, 2017

I haven't thought about this in detail yet, so these are just a few ideas:

  • FilterTree(T1, Continue()) makes me think of find's -prune.
  • Instead of having a way to apply a set of options to a subtree of the comparison, would it be simpler to apply a custom comparison function to the subtree root that recursively compares that subtree? (This would require support for recursive accumulation of diffs.)
  • For example 3, it seems like the solution is to use FilterPath(f, Comparer(proto.Equal)), where f matches on types which implement proto.Message, but not structs which embed a proto.Message.

@dsnet
Copy link
Collaborator Author

dsnet commented Aug 3, 2017

Instead of having a way to apply a set of options to a subtree of the comparison, would it be simpler to apply a custom comparison function to the subtree root that recursively compares that subtree?

I see two complications with this:

  • (As you mentioned) You would need some way to report the accumulated diffs back up the stack.
  • The user may want to continue performing comparisons using the set of options passed into the original call to cmp.Equal. We could also plumb those options through, but one of the factors that affect the result of comparison is FilterPath, which is dependent on the current Path, which cannot be reliably reproduced by the user. Even if we expose the current path to the user, I don't trust them to update it properly.

EDIT: The messagediff API takes this approach with:

type Comparator interface {
	Compare(v1, v2 interface{}, path *Path, dc Comparator) ComparisonResult
}

The Compare method has the current Path and Comparator passed in, but it is easy to forget updating the Path when calling Comparator.Compare recursively on elements of a slice, for example.

@neild
Copy link
Collaborator

neild commented Aug 3, 2017

I think that recursive comparison may become a necessary feature at some point, but perhaps I'm wrong about that.

I'd envision something like:

type EqualFunc func(x, y interface{}, opts ...Option) bool

Comparer(func(x, y T, eq EqualFunc) bool {
  return eq(x, y, additionalOptions...)
})

The EqualFunc inherits the options and path of the parent comparison. I'm not sure if going down this path is a simplification over complex option trees as in this CL or not.

@dsnet
Copy link
Collaborator Author

dsnet commented Aug 3, 2017

The EqualFunc inherits the options and path of the parent comparison.

I think this is problematic when you do something like:

Comparer(func(x, y []MyStruct, eq EqualFunc) bool {
    ... // Check that len(x) == len(y)
    for i := 0; i < len(x); i++ {
        // This is subtly wrong since eq doesn't know that
        // a SliceIndex path step should be added.
        if !eq(x[i], y[i]) {
            return false
        }
    }
    return true
})

It is also problematic when you do:

Comparer(func(x, y []MyStruct, eq EqualFunc) bool {
    // What if eq already has an EquateApprox option?
    // Does mine take precedence?
    return eq(x, y, EquateApprox(...))
})

Also, if we had the API suggested, then it is unfortunate that we introduced Transformers since those can be effectively done with:

Comparer(func(x, y T, eq EqualFunc) bool {
    x2, y2 := transform(x), transform(y)
    // But how does eq know that a transform occurred?
    return eq(x2, y2)
})

I think that recursive comparison may become a necessary feature at some point, but perhaps I'm wrong about that.

I can't think of any use-case that this can accomplish that can't be done with the current set of options and FilterPriority+Continue. I argue, but have not proven, that the proposed API is equivalent to providing an EqualFunc, only it is less error-prone since the only time control is given back to user is for custom Comparers and Transformers where those don't recurse back into the logic of cmp.Equal (as opposed to adding EqualFunc which does allow Comparers to recurse back into cmp.Equal). This ensures that cmp.Equal can properly maintain all the bookkeeping without relying on the user to do that work correctly. It also ensures that reporting of differences is more sensible since all steps are properly recorded.

@dsnet
Copy link
Collaborator Author

dsnet commented Aug 3, 2017

Actually, the EqualFunc feature can do something that my proposal can't do. In #28, Bryan suggested that we "discard all values that are equal to the zero value, not just values identical to the zero value", where equality is determined by the set of options used in the initial cmp.Equal.

Thus, (ignoring filters to prevent infinite cycles), this would look like:

cmp.Transformer(func(src []MyStruct, eq EqualFunc) (dst []MyStruct) {
	for i := range src {
		// Again, how does eq know that a SliceIndex step occurred here?
		if !eq(src[i], MyStruct{}) {
			dst = append(dst, src[i])
		}
	}
	return dst
})

My concern about incorrect paths still remains.

Unless there is a satisfiable answer to EqualFunc where users can't easily shoot themselves in the foot with the wrong Path, I'm okay with cmp never having a good solution to these arguably rare use-cases.

My current solution to #28, was to provide func DiscardElements(rm func(T) bool) Option where it is the user's responsibility to define what elements they want. Their implementation of rm may call a different instance of cmp.Equal and pass in their own set of Options, but at least they have control over it and it is clear to them what options they are passing in.

DiscardElements(func(v MyStruct) bool {
	// EquateEmpty and proto.Equal may well have been already options passed to the
	// original invocation of cmp.Equal, but at least there's no question here what
	// options are applicable here.
	return cmp.Equal(v, MyStruct{}, EquateEmpty(), cmp.Comparer(proto.Equal))
})

@jba
Copy link
Collaborator

jba commented Aug 3, 2017

Apologies for being too busy (OK, lazy) to read this whole proposal in detail. But from the POV of an interested consumer of this API, it is getting seriously complicated.

I'd urge you all to consider three things:

  1. Are there actual examples that have come up in practice where the current API falls short?
  2. Is there some (possibly cumbersome) way to achieve the needed effects with the existing API? (Maybe losing composition. It's a nice property, but might not be worth this level of complexity.) For instance, in example 2, couldn't I put the options behind a filter that returned false for T1 and T2?
  3. Is there some simpler mechanism that does most of the job without most of the complexity? For example, a primitive Default option that used the default cmp.Equal. That would allow you to express some negative patterns. Or maybe reconsider the ordering of options in the list.

Basically, adding priorities feels like jumping the shark to me.

@dsnet
Copy link
Collaborator Author

dsnet commented Aug 3, 2017

Are there actual examples that have come up in practice where the current API falls short?

Yes. All three examples in the description were driven by real cases. The solution to work around them involved some really ugly set of filters to negate some set while applying another.

Thus, example 1 would look like:

FilterTree(T1, OT1),
FilterTree(T2, OT2),
FilterTree(T, FilterTree(!T1, FilterTree(!T2, OT)))
FilterTree(!T, O),

In reality, this becomes nasty since filters are not standalone values that you can compose nicely together.

For example, a primitive Default option that used the default cmp.Equal

Isn't that what Continue is? (although Default sounds like a good name).

Or maybe reconsider the ordering of options in the list.

Isn't that what FilterPriority is?

@jba
Copy link
Collaborator

jba commented Aug 3, 2017

Or maybe reconsider the ordering of options in the list.

Isn't that what FilterPriority is?

I wasn't clear. I meant reconsider the idea that the ordering of options in the list matters. Use the ordering as a priority.

@dsnet
Copy link
Collaborator Author

dsnet commented Aug 3, 2017

It's a large benefit that generally the ordering of options do not matter and that I don't need to think about it. In a sense, all FilterPriority does is allow you to manually opt-in to saying "I want ordering to matter" as a way to resolve conflicts.

If that wasn't what you thought FilterPriority did, a better name can be suggested.

@dsnet dsnet changed the title Add PriorityFilter to prioritize some set of options over another Add FilterPriority to prioritize some set of options over another Aug 3, 2017
Added API:
	Default() Option
	FilterPriority(opts ...Option) Option

FilterPriority returns a new Option where an option, opts[i],
is only evaluated if no fundamental options remain after applying all filters
in all prior options, opts[:i].

In order to prevent further options from being evaluated, the Default option
can be used to ensure that some fundamental option remains.

Suppose you have a value tree T, where T1 and T2 are sub-trees within T.
Prior to the addition of FilterPriority, it was impossible to do certain things.

Example 1: You could not make the following compose together nicely.
	* Have a set of options OT1 to affect only values under T1.
	* Have a set of options OT2 to affect only values under T2.
	* Have a set of options OT to affect only T, but not values under T1 and T2.
	* Have a set of options O to affect all other values, but no those in T
	  (and by extension those in T1 and T2).
Solution 1:
	FilterPriority(
		// Since T1 and T2 do not overlap, they could be placed within the
		// same priority level by grouping them in an Options group.
		FilterTree(T1, OT1),
		FilterTree(T2, OT2),
		FilterTree(T, OT),
		O,
	)

Example 2: You could not make the following compose together nicely.
	* Have a set of options O apply on all nodes except those in T1 and T2.
	* Instead, we want the default behavior of cmp on T1 and T2.
Solution 2:
	FilterPriority(
		// Here we show how to group T1 and T2 together to be on the same
		// priority level.
		Options{
			FilterTree(T1, Default()),
			FilterTree(T2, Default()),
		},
		O,
	)

Example 3: You have this: type MyStruct struct { *pb.MyMessage; ... }
	* Generally, you want to use Comparer(proto.Equal) to ensure
	  that all proto.Messages within the struct are properly compared.
	  However, this type has an embedded proto (generally a bad idea),
	  which causes the MyStruct to satisfy the proto.Message interface
	  and unintentionally causes Equal to use proto.Equal on MyStruct,
	  which crashes.
	* How can you have Comparer(proto.Equal) apply to all other
	  proto.Message without applying just to MyStruct?
Solution 3:
	FilterPriority(
		// Only for MyStruct, use the default behavior of Equal,
		// which is to recurse into the structure of MyStruct.
		FilterPath(func(p Path) bool {
			return p.Last().Type() == reflect.TypeOf(MyStruct{})
		}, Default()),

		// Use proto.Equal for all other cases of proto.Message.
		Comparer(proto.Equal),
	)
@dsnet
Copy link
Collaborator Author

dsnet commented Feb 8, 2018

(I'm not resurrecting this PR yet)

Note to self: Think carefully about whether Default is only applicable for the immediate closing FilterPriority, or is applicable regardless of the number of nested filters.

By analogy, if evaluation for FilterPriority was treated as for-looping through a list of options, looking for the first applicable one, then is Default acting as a break or return?

if o1, ok := opts.(*priorityFilter); ok {
	for _, o2 := range o1.opts {
		if o2, ok := o2.(*priorityFilter); ok {
			for _, o3 := range o2.opts {
				if IsDefault(o3) {
					// break or return?
				}
				...
			}
		}
		...
	}
}
...

As of 5b174e0, the behavior of Default is equivalent to return.

@dnephin
Copy link

dnephin commented Mar 28, 2018

Since it seems this may be a blocker for #75 I've read over this issue. Looking at the examples, it seems most of this could be implemented already.

Example 3 (already mentioned in #36 (comment)) I believe could be implemented this way:

FilterPath(func(p Path) bool {
    return p.Last().Type() != reflect.TypeOf(MyStruct{})
}, Comparer(proto.Equal)),

I believe the other two examples could use the same idea (a negative path filter). This could be made more convenient by exposing more path filters (as suggested in #75), along with a PathNot():

func PathNot(f func(Path) bool) f func(Path) bool {
    return func(path Path) bool {
        return !f(path)
    }
}

So example 3 becomes:

FilterPath(PathNot(PathType(MyStruct{})), Comparer(proto.Equal))

For examples 1 and 2 you would use some other Path filter function that matches the root of the tree (instead of PathType).

@dsnet
Copy link
Collaborator Author

dsnet commented Mar 28, 2018

In issue #75, we discussed how func(cmp.Path) bool and func(interface{}) bool doesn't compose well. Given PathNot, you would also define ValueNot for symmetry. However, PathNot and ValueNot do not compose well for the same reasons discussed earlier.

The benefit of FilterPriority is that it accomplishes a "not" filter without needing to know how the filter was constructed.

@dnephin
Copy link

dnephin commented Mar 28, 2018

we discussed how func(cmp.Path) bool and func(interface{}) bool doesn't compose well. Given PathNot, you would also define ValueNot for symmetry. However, PathNot and ValueNot do not compose well for the same reasons discussed earlier.

I thought about that a bit more, and I'm not really seeing the problem. There are two interfaces:

  • "path filter": func(Path) bool
  • options: Option

For the majority of cases a simple path filter seems to be appropriate. In the rare case when you want to filter by something more than paths you can create something that returns Option instead of a path filter.

PathNot and ValueNot can be composed using FilterPath as long as ValueNot returns an Option:

func Something() Option {
    return FilterPath(PathNot(x), ValueNot(y))
}

Any reason that won't work?

Edit: I believe FilterValues() already exists as a way to turn ValueNot into an Option

The benefit of FilterPriority is that it accomplishes a "not" filter without needing to know how the filter was constructed.

Possibly just me, but "not this path" seems easier to understand than a list of filter priorities. "not path" can be understood in isolation, it doesn't depend on other filters and options. To understand FilterPriority you have to first figure out what paths/values would match every previous Option in the list.

xosmig added a commit to xosmig/placeholders that referenced this pull request Jul 18, 2022
Comparer options cannot be used with other Comparer options or
Transformer options. Unfortunatelly, go-cmp currently doesn't
provide a nice way to compose several such options
(see: google/go-cmp#36).
An Ignore option seems to be more suitable semantically in this
case and it does not conflict with other options.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants