Merge branch 'master' into refactor/env-type-assert

Author: antonmedv

.gitattributes

@@ -0,0 +1 @@
+*\[generated\].go linguist-language=txt

.github/scripts/coverage.mjs

@@ -2,13 +2,15 @@
 
 const expected = 90
 const exclude = [
-  'expr/test',
-  'checker/mock',
-  'vm/func_types',
-  'vm/runtime/helpers',
-  'internal/difflib',
-  'internal/spew',
-  'internal/testify',
+  'expr/test', // We do not need to test the test package.
+  'checker/mock', // Mocks only used for testing.
+  'vm/func_types', // Generated files.
+  'vm/runtime/helpers', // Generated files.
+  'internal/difflib', // Test dependency. This is vendored dependency, and ideally we also have good tests for it.
+  'internal/spew', // Test dependency.
+  'internal/testify', // Test dependency.
+  'patcher/value', // Contains a lot of repeating code. Ideally we should have a test for it.
+  'pro', // Expr Pro is not a part of the main codebase.
 ]
 
 cd(path.resolve(__dirname, '..', '..'))

.github/workflows/build.yml

@@ -11,7 +11,7 @@ jobs:
     runs-on: ubuntu-latest
     strategy:
       matrix:
-        go-versions: [ '1.18', '1.22' ]
+        go-versions: [ '1.18', '1.22', '1.24' ]
         go-arch: [ '386' ]
     steps:
       - uses: actions/checkout@v3

.github/workflows/diff.yml

@@ -13,7 +13,9 @@ jobs:
         with:
           go-version: 1.18
       - name: Install benchstat
-        run: go install golang.org/x/perf/cmd/benchstat@latest
+        # NOTE: benchstat@latest requires go 1.23 since 2025-02-14 - this is the last go 1.18 ref
+        # https://cs.opensource.google/go/x/perf/+/c95ad7d5b636f67d322a7e4832e83103d0fdd292
+        run: go install golang.org/x/perf/cmd/benchstat@884df5810d2850d775c2cb4885a7ea339128a17d
 
       - uses: actions/checkout@v3
       - name: Benchmark new code

.github/workflows/fuzz.yml

@@ -21,14 +21,14 @@ jobs:
           fuzz-seconds: 600
           output-sarif: true
       - name: Upload Crash
-        uses: actions/upload-artifact@v3
+        uses: actions/upload-artifact@v4
         if: failure() && steps.build.outcome == 'success'
         with:
           name: artifacts
           path: ./out/artifacts
       - name: Upload Sarif
         if: always() && steps.build.outcome == 'success'
-        uses: github/codeql-action/upload-sarif@v2
+        uses: github/codeql-action/upload-sarif@v3
         with:
           # Path to SARIF file relative to the root of the repository
           sarif_file: cifuzz-sarif/results.sarif

.github/workflows/test.yml

@@ -11,7 +11,7 @@ jobs:
     runs-on: ubuntu-latest
     strategy:
       matrix:
-        go-versions: [ '1.18', '1.19', '1.20', '1.21', '1.22' ]
+        go-versions: [ '1.18', '1.19', '1.20', '1.21', '1.22', '1.23', '1.24' ]
     steps:
       - uses: actions/checkout@v3
       - name: Setup Go ${{ matrix.go-version }}

ast/print.go

@@ -50,8 +50,13 @@ func (n *UnaryNode) String() string {
 		op = fmt.Sprintf("%s ", n.Operator)
 	}
 	wrap := false
-	switch n.Node.(type) {
-	case *BinaryNode, *ConditionalNode:
+	switch b := n.Node.(type) {
+	case *BinaryNode:
+		if operator.Binary[b.Operator].Precedence <
+			operator.Unary[n.Operator].Precedence {
+			wrap = true
+		}
+	case *ConditionalNode:
 		wrap = true
 	}
 	if wrap {
@@ -68,10 +73,21 @@ func (n *BinaryNode) String() string {
 	var lhs, rhs string
 	var lwrap, rwrap bool
 
+	if l, ok := n.Left.(*UnaryNode); ok {
+		if operator.Unary[l.Operator].Precedence <
+			operator.Binary[n.Operator].Precedence {
+			lwrap = true
+		}
+	}
 	if lb, ok := n.Left.(*BinaryNode); ok {
 		if operator.Less(lb.Operator, n.Operator) {
 			lwrap = true
 		}
+		if operator.Binary[lb.Operator].Precedence == 
+			operator.Binary[n.Operator].Precedence && 
+			operator.Binary[n.Operator].Associativity == operator.Right {
+			lwrap = true
+		}
 		if lb.Operator == "??" {
 			lwrap = true
 		}
@@ -83,6 +99,11 @@ func (n *BinaryNode) String() string {
 		if operator.Less(rb.Operator, n.Operator) {
 			rwrap = true
 		}
+		if operator.Binary[rb.Operator].Precedence == 
+			operator.Binary[n.Operator].Precedence && 
+			operator.Binary[n.Operator].Associativity == operator.Left {
+			rwrap = true
+		}
 		if operator.IsBoolean(rb.Operator) && n.Operator != rb.Operator {
 			rwrap = true
 		}

ast/print_test.go

@@ -78,6 +78,11 @@ func TestPrint(t *testing.T) {
 		{`{("a" + "b"): 42}`, `{("a" + "b"): 42}`},
 		{`(One == 1 ? true : false) && Two == 2`, `(One == 1 ? true : false) && Two == 2`},
 		{`not (a == 1 ? b > 1 : b < 2)`, `not (a == 1 ? b > 1 : b < 2)`},
+		{`(-(1+1)) ** 2`, `(-(1 + 1)) ** 2`},
+		{`2 ** (-(1+1))`, `2 ** -(1 + 1)`},
+		{`(2 ** 2) ** 3`, `(2 ** 2) ** 3`},
+		{`(3 + 5) / (5 % 3)`, `(3 + 5) / (5 % 3)`},
+		{`(-(1+1)) == 2`, `-(1 + 1) == 2`},
 	}
 
 	for _, tt := range tests {

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,25 +56,11 @@ func ParseCheck(input string, config *conf.Config) (*parser.Tree, error) {
 	}
 
 	if len(config.Visitors) > 0 {
-		for i := 0; i < 1000; i++ {
-			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)
-
-				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 {

conf/config.go

@@ -10,31 +10,43 @@ import (
 	"github.com/expr-lang/expr/vm/runtime"
 )
 
+const (
+	// DefaultMemoryBudget represents an upper limit of memory usage
+	DefaultMemoryBudget uint = 1e6
+
+	// DefaultMaxNodes represents an upper limit of AST nodes
+	DefaultMaxNodes uint = 10000
+)
+
 type FunctionsTable map[string]*builtin.Function
 
 type Config struct {
-	EnvObject any
-	Env       nature.Nature
-	Expect    reflect.Kind
-	ExpectAny bool
-	Optimize  bool
-	Strict    bool
-	Profile   bool
-	ConstFns  map[string]reflect.Value
-	Visitors  []ast.Visitor
-	Functions FunctionsTable
-	Builtins  FunctionsTable
-	Disabled  map[string]bool // disabled builtins
+	EnvObject    any
+	Env          nature.Nature
+	Expect       reflect.Kind
+	ExpectAny    bool
+	Optimize     bool
+	Strict       bool
+	Profile      bool
+	MaxNodes     uint
+	MemoryBudget uint
+	ConstFns     map[string]reflect.Value
+	Visitors     []ast.Visitor
+	Functions    FunctionsTable
+	Builtins     FunctionsTable
+	Disabled     map[string]bool // disabled builtins
 }
 
 // CreateNew creates new config with default values.
 func CreateNew() *Config {
 	c := &Config{
-		Optimize:  true,
-		ConstFns:  make(map[string]reflect.Value),
-		Functions: make(map[string]*builtin.Function),
-		Builtins:  make(map[string]*builtin.Function),
-		Disabled:  make(map[string]bool),
+		Optimize:     true,
+		MaxNodes:     DefaultMaxNodes,
+		MemoryBudget: DefaultMemoryBudget,
+		ConstFns:     make(map[string]reflect.Value),
+		Functions:    make(map[string]*builtin.Function),
+		Builtins:     make(map[string]*builtin.Function),
+		Disabled:     make(map[string]bool),
 	}
 	for _, f := range builtin.Builtins {
 		c.Builtins[f.Name] = f