From b1fe2aaacae614630dd0ff062de8ae27c2def0ea Mon Sep 17 00:00:00 2001 From: Ryan Bullock Date: Wed, 22 May 2024 09:20:23 -0700 Subject: [PATCH 1/4] Reset tracking of applied operator overloads before every walk of the ast tree. --- checker/checker.go | 16 ++++++++++++---- patcher/operator_override.go | 5 +++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/checker/checker.go b/checker/checker.go index 0ad22f4e..36437a67 100644 --- a/checker/checker.go +++ b/checker/checker.go @@ -29,14 +29,22 @@ func ParseCheck(input string, config *conf.Config) (*parser.Tree, error) { // types information available in the tree. _, _ = Check(tree, config) + r, repeatable := v.(interface { + Reset() + ShouldRepeat() bool + }); + + if repeatable { + r.Reset() + } + ast.Walk(&tree.Node, v) - if v, ok := v.(interface { - ShouldRepeat() bool - }); ok { - more = more || v.ShouldRepeat() + if repeatable { + more = more || r.ShouldRepeat() } } + if !more { break } diff --git a/patcher/operator_override.go b/patcher/operator_override.go index b2d20ec4..308cbdba 100644 --- a/patcher/operator_override.go +++ b/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 } From cd1f83d698c7b70488ddae47d4e771c09cdae7cf Mon Sep 17 00:00:00 2001 From: Ryan Bullock Date: Wed, 22 May 2024 09:23:31 -0700 Subject: [PATCH 2/4] Remove limit on operator overload resolution --- checker/checker.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/checker/checker.go b/checker/checker.go index 36437a67..861e0590 100644 --- a/checker/checker.go +++ b/checker/checker.go @@ -22,7 +22,7 @@ func ParseCheck(input string, config *conf.Config) (*parser.Tree, error) { } if len(config.Visitors) > 0 { - for i := 0; i < 1000; i++ { + for { more := false for _, v := range config.Visitors { // We need to perform types check, because some visitors may rely on From 3e8d788c4b3f07c40250d397882c08eefa862595 Mon Sep 17 00:00:00 2001 From: Ryan Bullock Date: Wed, 22 May 2024 15:58:51 -0700 Subject: [PATCH 3/4] Do separate patcher phase for those that require multiple passes (Currently only need for Operator overloading). This patcher phase is run after other patchers. --- checker/checker.go | 64 +++++++++++++++++++++++++++------------------- 1 file changed, 38 insertions(+), 26 deletions(-) diff --git a/checker/checker.go b/checker/checker.go index 861e0590..e71f1d7e 100644 --- a/checker/checker.go +++ b/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,33 +56,11 @@ func ParseCheck(input string, config *conf.Config) (*parser.Tree, error) { } if len(config.Visitors) > 0 { - 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 { - r.Reset() - } - - ast.Walk(&tree.Node, v) - - if repeatable { - more = more || r.ShouldRepeat() - } - } + // Run all patchers that dont support being run repeatedly first + runVisitors(tree, config, false) - if !more { - break - } - } + // Run patchers that require multiple passes next (currently only Operator patching) + runVisitors(tree, config, true) } _, err = Check(tree, config) if err != nil { From 0d416ccad0f92d4b1e22696fd5e04a64f7712bc9 Mon Sep 17 00:00:00 2001 From: Ryan Bullock Date: Tue, 18 Jun 2024 12:44:31 -0700 Subject: [PATCH 4/4] Add test to track how many times a patcher is run. Used to detect if patching phases is erroneously applying the patcher multiple times. --- test/patch/patch_count_test.go | 61 ++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) create mode 100644 test/patch/patch_count_test.go diff --git a/test/patch/patch_count_test.go b/test/patch/patch_count_test.go new file mode 100644 index 00000000..e877de4a --- /dev/null +++ b/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") +}