search: revamp visitor internals

Created by: rvantonder

This is a revamp of the visitor internals that allows overriding methods for specific visitor functions, in the same flavor as #9752. This simplifies Visit and VisitNode functions from before (now there is only VisitNodes).

It's awkward trying to do method overrides in Go in the presence of mutual recursion, where you end up with visitor.<method-call>(pass-visitor-as-this, <other things>). Without the recursion part, method overrides is not as weird. After looking into things in various places, this is the best I came up with and I'm happy with it. Now though, the visitor has to get its own state for the callback, because there doesn't appear to be a way to create a closure over a new type/struct that implements a method override. Anyway, it's an improvement on the previous implementation that'll simplify other things.

Existing tests cover this change.

Created by: codecov[bot]

Codecov Report

Merging #9753 into master will decrease coverage by 4.24%. The diff coverage is 92.85%.

@@            Coverage Diff             @@
##           master    #9753      +/-   ##
==========================================
- Coverage   46.00%   41.76%   -4.24%     
==========================================
  Files        1352     1337      -15     
  Lines       76847    73424    -3423     
  Branches     6587     6625      +38     
==========================================
- Hits        35355    30667    -4688     
- Misses      38110    39914    +1804     
+ Partials     3382     2843     -539     

Flag	Coverage Δ
#go	?
#storybook	?
#typescript	?
#unit	41.76% <92.85%> (-4.06%)

Impacted Files	Coverage Δ
internal/search/query/visitor.go	93.10% <92.85%> (-6.90%)
.../enterprise/productSubscription/ExpirationDate.tsx	0.00% <0.00%> (-100.00%)
...enterprise/dotcom/productSubscriptions/features.ts	0.00% <0.00%> (-100.00%)
...erprise/productSubscription/ProductCertificate.tsx	0.00% <0.00%> (-100.00%)
...erprise/productSubscription/ProductLicenseTags.tsx	0.00% <0.00%> (-100.00%)
...ductSubscription/ProductLicenseInfoDescription.tsx	0.00% <0.00%> (-100.00%)
...r/productSubscriptions/PaymentTokenFormControl.tsx	0.00% <0.00%> (-100.00%)
...om/productSubscriptions/ProductLicenseValidity.tsx	0.00% <0.00%> (-100.00%)
...oductSubscriptions/SiteAdminProductLicenseNode.tsx	0.00% <0.00%> (-100.00%)
shared/src/highlight/contributions.ts	0.00% <0.00%> (-97.37%)
... and 713 more

Created by: unknwon

Is this Draft ready for review?

Created by: rvantonder

@unknwon it actually is :D but I was prioritizing review for other PRs. Next time I'll add a note to description when I do that. Will put it up now since you already have context from #9752.

Created by: rvantonder

I'm curious to know what was not working, to kick off the discussion, say:

You are most welcome to try this change that erases the visitor from the first argument (I mean this sincerely, report back if you learn something). I spent a couple of hours to converge on what's here, and based on a couple of sources, and my understanding of the language, this is one of the better ways to go about it.

If you make that change you suggested and erase the first arg, you essentially erase the 'this' member in all of the methods that allows for method overrides. The whole interface changes, for example, you'll see that you will no longer have any visitor member to call in the func *BaseVisitor VisitNodes(...) function, and neither will any deriving visitor. So what will you do to call the next visit methods from within VisitNodes? You can try putting things into the visitor struct, but you'll probably run into one of many dead ends, like I did when I tried (and I tried a lot).

Also, having looked at other visitor frameworks like the Go AST visitor: this is too basic, and it has the same limitations that caused me to change this implementation. Basically, you could only use a function with the definition f func(node Node) to walk/visit the query. In a real visitor framework, you want this function f to return the specific members of the node, and provide ways to selectively override this f based on customized visitors. You can get away with the basic AST visitor framework where you don't care about the return type (like unit), and inspect every node in customized visitors but that's unsatisfying, and as soon as you care about the specific values to manipulate, and the return type for each f (see #9752 ), you need a real visitor framework.

Created by: unknwon

Here is my attempt (field name isn't important):

package main

type Node interface{}
type operatorKind interface{}
type Parameter struct {
	Field   string
	Value   string
	Negated bool
}
type Operator struct {
	Kind     operatorKind
	Operands []Node
}

type Visitor interface {
	VisitNodes(node []Node)
	VisitOperator(kind operatorKind, operands []Node)
	VisitParameter(field, value string, negated bool)
}

var _ Visitor = (*BaseVisitor)(nil)

type BaseVisitor struct {
	mockVisitNodes     func(nodes []Node)
	mockVisitOperator  func(kind operatorKind, operands []Node)
	mockVisitParameter func(field, value string, negated bool)
}

func (v *BaseVisitor) VisitNodes(nodes []Node) {
	if v.mockVisitNodes != nil {
		v.mockVisitNodes(nodes)
		return
	}

	for _, node := range nodes {
		switch node := node.(type) {
		case Parameter:
			v.VisitParameter(node.Field, node.Value, node.Negated)
		case Operator:
			v.VisitOperator(node.Kind, node.Operands)
		default:
			panic("unreachable")
		}
	}
}

func (v *BaseVisitor) VisitOperator(kind operatorKind, operands []Node) {
	if v.mockVisitOperator != nil {
		v.mockVisitOperator(kind, operands)
	}
}

func (v *BaseVisitor) VisitParameter(field, value string, negated bool) {
	if v.mockVisitParameter != nil {
		v.mockVisitParameter(field, value, negated)
	}
}

type OperatorVisitor struct {
	callback func(kind operatorKind, operands []Node)
	BaseVisitor
}

func VisitOperator(nodes []Node, callback func(kind operatorKind, operands []Node)) {
	visitor := &OperatorVisitor{
		callback: callback,
	}
	visitor.mockVisitOperator = func(kind operatorKind, operands []Node) {
		visitor.callback(kind, operands)
		visitor.VisitNodes(operands)
	}
	visitor.VisitNodes(nodes)
}

type ParameterVisitor struct {
	callback func(field, value string, negated bool)
	BaseVisitor
}

func VisitParameter(nodes []Node, callback func(field, value string, negated bool)) {
	visitor := &ParameterVisitor{
		callback: callback,
	}
	visitor.mockVisitParameter = func(field, value string, negated bool) {
		visitor.callback(field, value, negated)
	}
	visitor.VisitNodes(nodes)
}

type FieldVisitor struct {
	field    string
	callback func(value string, negated bool)
	BaseVisitor
}

func VisitField(nodes []Node, field string, callback func(value string, negated bool)) {
	visitor := &FieldVisitor{
		field:    field,
		callback: callback,
	}
	visitor.mockVisitParameter = func(field, value string, negated bool) {
		if visitor.field == field {
			visitor.callback(value, negated)
		}
	}
	visitor.VisitNodes(nodes)
}

Created by: keegancsmith

Are there examples of a visitor which needs to implement visit for operators and parameters? If not why not just take a closure and always do a DFS?

func Visit(Node, func(...)) // closure is responsible for inspecting Node
func VisitOperators(Node, func(...))
func VisitParameters(Node, func(...))
func VisitFields(Node, func(...))

If you do need to have both, rather than an interface (which can be clunky to define) you can still rely on closures

type Visitor struct {
  Operators func(...)
  Parameters func(...)
  Fields func(...)
}

/// usage. Closures can mutate local state
v := &Visitor{
  Operators: func(...)
  Fields: func(...)
}
v.Visit(node)

Created by: rvantonder

Will merge this after I'm back, needs a rebase.

Created by: rvantonder

To come back to this :-) I'm going ahead with this merge to keep it consistent with mapper and may change things up again in future.

Comments:

My read of @unknwon solution is that it does something conceptually equivalent, using a different mechanism in Go (embedding) by allowing method override (via if nil check on whether the method exists/is set in BaseVisitor).

For @keegancsmith first suggestion, the helper functions I added exist for the case where only one term kind matters. The bit I want to support generically is to have some scaffolding in place for customizing visitors. Your second part gets into that, and I didn't explore this recently but back when I attempted to do a fully worked solution using that method I ran into some limitations. I know that sounds vague--the main thing I think I ran into is that declaring a visitor like:

v := &Visitor{
  Operators: func(...)
  Fields: func(...)
}

means that whatever logic is defined in Operators and Fields "methods" (i.e., functions of poor man's object) is only local in scope, and so cannot be reused (i.e., inherited) by any subsequent visitor definition. So this solution encourages a one-off definition (which, yes, suffices in many cases). And yeah, I recognize that reusable parts of one-off definitions means you could just copy the visitor bodies into your 'new' visitor and do extra stuff, and ta-da now you don't need to deal with overriding anything. At the end of the day I'm in favor of a proper visitor abstraction to cover any sort of generic changes via some kind of composition/inheritance relation that I may be interested in down the line.

When I have more time I may look into this more and report back.