Separate patching phases for patchers that require multiple passes. (expr-lang#659)

* Reset tracking of applied operator overloads before every walk of the ast tree.

* Remove limit on operator overload resolution

* Do separate patcher phase for those that require multiple passes (Currently only need for Operator overloading). This patcher phase is run after other patchers.

* Add test to track how many times a patcher is run. Used to detect if patching phases is erroneously applying the patcher multiple times.

---------

Co-authored-by: Anton Medvedev <[email protected]>

Author: 2 people

checker/checker.go

@@ -13,6 +13,40 @@ import (
 	"github.com/expr-lang/expr/parser"
 )
 
+// Run visitors in a given config over the given tree
+// runRepeatable controls whether to filter for only vistors that require multiple passes or not
+func runVisitors(tree *parser.Tree, config *conf.Config, runRepeatable bool) {
+	for {
+		more := false
+		for _, v := range config.Visitors {
+			// We need to perform types check, because some visitors may rely on
+			// types information available in the tree.
+			_, _ = Check(tree, config)
+
+			r, repeatable := v.(interface {
+				Reset()
+				ShouldRepeat() bool
+			})
+
+			if repeatable {
+				if runRepeatable {
+					r.Reset()
+					ast.Walk(&tree.Node, v)
+					more = more || r.ShouldRepeat()
+				}
+			} else {
+				if !runRepeatable {
+					ast.Walk(&tree.Node, v)
+				}
+			}
+		}
+
+		if !more {
+			break
+		}
+	}
+}
+
 // ParseCheck parses input expression and checks its types. Also, it applies
 // all provided patchers. In case of error, it returns error with a tree.
 func ParseCheck(input string, config *conf.Config) (*parser.Tree, error) {
@@ -22,26 +56,32 @@ func ParseCheck(input string, config *conf.Config) (*parser.Tree, error) {
 	}
 
 	if len(config.Visitors) > 0 {
-		// We need to perform types check, because some visitors may rely on
-		// types information available in the tree.
-		_, _ = Check(tree, config)
-
-		// TODO fubang 1000 count?
-		for i := 0; i < 1000; i++ {
-			more := false
-			for _, v := range config.Visitors {
-				ast.Walk(&tree.Node, v)
-
-				if v, ok := v.(interface {
-					ShouldRepeat() bool
-				}); ok {
-					more = more || v.ShouldRepeat()
-				}
-			}
-			if !more {
-				break
-			}
-		}
+		// // We need to perform types check, because some visitors may rely on
+		// // types information available in the tree.
+		// _, _ = Check(tree, config)
+
+		// // TODO fubang 1000 count?
+		// for i := 0; i < 1000; i++ {
+		// 	more := false
+		// 	for _, v := range config.Visitors {
+		// 		ast.Walk(&tree.Node, v)
+
+		// 		if v, ok := v.(interface {
+		// 			ShouldRepeat() bool
+		// 		}); ok {
+		// 			more = more || v.ShouldRepeat()
+		// 		}
+		// 	}
+		// 	if !more {
+		// 		break
+		// 	}
+		// }
+
+		// Run all patchers that dont support being run repeatedly first
+		runVisitors(tree, config, false)
+
+		// Run patchers that require multiple passes next (currently only Operator patching)
+		runVisitors(tree, config, true)
 	}
 	_, err = Check(tree, config)
 	if err != nil {

patcher/operator_override.go

@@ -43,6 +43,11 @@ func (p *OperatorOverloading) Visit(node *ast.Node) {
 	}
 }
 
+// Tracking must be reset before every walk over the AST tree
+func (p *OperatorOverloading) Reset() {
+	p.applied = false
+}
+
 func (p *OperatorOverloading) ShouldRepeat() bool {
 	return p.applied
 }

test/patch/patch_count_test.go

@@ -0,0 +1,61 @@
+package patch_test
+
+import (
+	"testing"
+
+	"github.com/expr-lang/expr/internal/testify/require"
+
+	"github.com/expr-lang/expr"
+	"github.com/expr-lang/expr/ast"
+	"github.com/expr-lang/expr/test/mock"
+)
+
+// This patcher tracks how many nodes it patches which can 
+// be used to verify if it was run too many times or not at all
+type countingPatcher struct {
+	PatchCount int
+}
+
+func (c *countingPatcher) Visit(node *ast.Node) {
+	switch (*node).(type) {
+	case *ast.IntegerNode:
+		c.PatchCount++
+	}
+}
+
+// Test over a simple expression
+func TestPatch_Count(t *testing.T) {
+	patcher := countingPatcher{}
+
+	_, err := expr.Compile(
+		`5 + 5`,
+		expr.Env(mock.Env{}),
+		expr.Patch(&patcher),
+	)
+	require.NoError(t, err)
+
+	require.Equal(t, 2, patcher.PatchCount, "Patcher run an unexpected number of times during compile")
+}
+
+// Test with operator overloading
+func TestPatchOperator_Count(t *testing.T) {
+	patcher := countingPatcher{}
+
+	_, err := expr.Compile(
+		`5 + 5`,
+		expr.Env(mock.Env{}),
+		expr.Patch(&patcher),
+		expr.Operator("+", "_intAdd"),
+		expr.Function(
+			"_intAdd",
+			func(params ...any) (any, error) {
+				return params[0].(int) + params[1].(int), nil
+			},
+			new(func(int, int) int),
+		),
+	)
+
+	require.NoError(t, err)
+
+	require.Equal(t, 2, patcher.PatchCount, "Patcher run an unexpected number of times during compile")
+}