Correctly add OpDeref if needed

Fixes #739

Author: antonmedv

builtin/builtin.go

@@ -493,6 +493,9 @@ var Builtins = []*Function{
 			}
 			return anyType, fmt.Errorf("invalid number of arguments (expected 0, got %d)", len(args))
 		},
+		Deref: func(i int, arg reflect.Type) bool {
+			return false
+		},
 	},
 	{
 		Name: "duration",
@@ -567,6 +570,12 @@ var Builtins = []*Function{
 			}
 			return timeType, nil
 		},
+		Deref: func(i int, arg reflect.Type) bool {
+			if arg.AssignableTo(locationType) {
+				return false
+			}
+			return true
+		},
 	},
 	{
 		Name: "timezone",

builtin/builtin_test.go

@@ -642,11 +642,17 @@ func Test_int_unwraps_underlying_value(t *testing.T) {
 func TestBuiltin_with_deref(t *testing.T) {
 	x := 42
 	arr := []any{1, 2, 3}
+	arrStr := []string{"1", "2", "3"}
 	m := map[string]any{"a": 1, "b": 2}
+	jsonString := `["1"]`
+	str := "1,2,3"
 	env := map[string]any{
-		"x":   &x,
-		"arr": &arr,
-		"m":   &m,
+		"x":      &x,
+		"arr":    &arr,
+		"arrStr": &arrStr,
+		"m":      &m,
+		"json":   &jsonString,
+		"str":    &str,
 	}
 
 	tests := []struct {
@@ -669,6 +675,10 @@ func TestBuiltin_with_deref(t *testing.T) {
 		{`uniq(arr)`, []any{1, 2, 3}},
 		{`concat(arr, arr)`, []any{1, 2, 3, 1, 2, 3}},
 		{`flatten([arr, [arr]])`, []any{1, 2, 3, 1, 2, 3}},
+		{`toJSON(arr)`, "[\n  1,\n  2,\n  3\n]"},
+		{`fromJSON(json)`, []any{"1"}},
+		{`split(str, ",")`, []string{"1", "2", "3"}},
+		{`join(arrStr, ",")`, "1,2,3"},
 	}
 
 	for _, test := range tests {

builtin/function.go

@@ -11,6 +11,7 @@ type Function struct {
 	Safe      func(args ...any) (any, uint, error)
 	Types     []reflect.Type
 	Validate  func(args []reflect.Type) (reflect.Type, error)
+	Deref     func(i int, arg reflect.Type) bool
 	Predicate bool
 }
 

checker/checker.go

@@ -975,6 +975,14 @@ func (v *checker) checkFunction(f *builtin.Function, node ast.Node, arguments []
 			lastErr = err
 			continue
 		}
+
+		// As we found the correct function overload, we can stop the loop.
+		// Also, we need to set the correct nature of the callee so compiler,
+		// can correctly handle OpDeref opcode.
+		if callNode, ok := node.(*ast.CallNode); ok {
+			callNode.Callee.SetType(t)
+		}
+
 		return outNature
 	}
 	if lastErr != nil {

checker/nature/nature.go

@@ -25,6 +25,24 @@ type Nature struct {
 	FieldIndex      []int             // Index of field in type.
 }
 
+func (n Nature) IsNil() bool {
+	return n.Nil
+}
+
+func (n Nature) IsAny() bool {
+	return n.Kind() == reflect.Interface && n.NumMethods() == 0
+}
+
+func (n Nature) IsUnknown() bool {
+	switch {
+	case n.Type == nil && !n.Nil:
+		return true
+	case isAny(n):
+		return true
+	}
+	return false
+}
+
 func (n Nature) String() string {
 	if n.Type != nil {
 		return n.Type.String()

compiler/compiler.go

@@ -750,17 +750,15 @@ func (c *compiler) CallNode(node *ast.CallNode) {
 		}
 		for i, arg := range node.Arguments {
 			c.compile(arg)
-			if k := kind(arg.Type()); k == reflect.Ptr || k == reflect.Interface {
-				var in reflect.Type
-				if fn.IsVariadic() && i >= fnNumIn-1 {
-					in = fn.In(fn.NumIn() - 1).Elem()
-				} else {
-					in = fn.In(i + fnInOffset)
-				}
-				if k = kind(in); k != reflect.Ptr && k != reflect.Interface {
-					c.emit(OpDeref)
-				}
+
+			var in reflect.Type
+			if fn.IsVariadic() && i >= fnNumIn-1 {
+				in = fn.In(fn.NumIn() - 1).Elem()
+			} else {
+				in = fn.In(i + fnInOffset)
 			}
+
+			c.derefParam(in, arg)
 		}
 	} else {
 		for _, arg := range node.Arguments {
@@ -1059,8 +1057,19 @@ func (c *compiler) BuiltinNode(node *ast.BuiltinNode) {
 
 	if id, ok := builtin.Index[node.Name]; ok {
 		f := builtin.Builtins[id]
-		for _, arg := range node.Arguments {
+		for i, arg := range node.Arguments {
 			c.compile(arg)
+			argType := arg.Type()
+			if argType.Kind() == reflect.Ptr || arg.Nature().IsUnknown() {
+				if f.Deref == nil {
+					// By default, builtins expect arguments to be dereferenced.
+					c.emit(OpDeref)
+				} else {
+					if f.Deref(i, argType) {
+						c.emit(OpDeref)
+					}
+				}
+			}
 		}
 
 		if f.Fast != nil {
@@ -1218,6 +1227,18 @@ func (c *compiler) derefInNeeded(node ast.Node) {
 	}
 }
 
+func (c *compiler) derefParam(in reflect.Type, param ast.Node) {
+	if param.Nature().Nil {
+		return
+	}
+	if param.Type().AssignableTo(in) {
+		return
+	}
+	if in.Kind() != reflect.Ptr && param.Type().Kind() == reflect.Ptr {
+		c.emit(OpDeref)
+	}
+}
+
 func (c *compiler) optimize() {
 	for i, op := range c.bytecode {
 		switch op {

expr.go

@@ -244,6 +244,7 @@ func Eval(input string, env any) (any, error) {
 	if err != nil {
 		return nil, err
 	}
+	println(program.Disassemble())
 
 	output, err := Run(program, env)
 	if err != nil {

patcher/value/value_test.go

@@ -88,7 +88,6 @@ func Test_valueAddInt(t *testing.T) {
 
 	program, err := expr.Compile("ValueOne + ValueTwo", expr.Env(env), ValueGetter)
 	require.NoError(t, err)
-
 	out, err := vm.Run(program, env)
 
 	require.NoError(t, err)

test/deref/deref_test.go

@@ -320,3 +320,20 @@ func TestDeref_ignore_struct_func_args(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, "UTC", out)
 }
+
+func TestDeref_keep_pointer_if_arg_in_interface(t *testing.T) {
+	x := 42
+	env := map[string]any{
+		"x": &x,
+		"fn": func(p any) int {
+			return *p.(*int) + 1
+		},
+	}
+
+	program, err := expr.Compile(`fn(x)`, expr.Env(env))
+	require.NoError(t, err)
+
+	out, err := expr.Run(program, env)
+	require.NoError(t, err)
+	require.Equal(t, 43, out)
+}

test/issues/739/issue_test.go

@@ -0,0 +1,19 @@
+package issue_test
+
+import (
+	"testing"
+
+	"github.com/expr-lang/expr"
+	"github.com/expr-lang/expr/internal/testify/require"
+)
+
+func TestIssue739(t *testing.T) {
+	jsonString := `{"Num": 1}`
+	env := map[string]any{
+		"aJSONString": &jsonString,
+	}
+
+	result, err := expr.Eval("fromJSON(aJSONString)", env)
+	require.NoError(t, err)
+	require.Contains(t, result, "Num")
+}