1.15.4 breaks function patcher, with error: expected float64, but got interface {}

[tw1nk]

In our codebase we have a Patcher that let's us not specify the context as a function value if the function requires a context.

However when we compile scripts with the 1.15.4 version we get errors similar to: expected float64, but got interface {}

Embedded test:

import (
	"context"
	"fmt"
	"reflect"
	"strconv"
	"testing"

	"github.com/antonmedv/expr"
	"github.com/antonmedv/expr/ast"
)

type TestStruct struct {
	Data string
}

func (ts *TestStruct) GetFloat() (float64, error) {
	return strconv.ParseFloat(ts.Data, 64)
}

func testMeFunc(arguments ...any) (any, error) {
	if len(arguments) != 2 {
		return nil, fmt.Errorf("expected 2 argument. got: %d", len(arguments))
	}

	ctx, ok := arguments[0].(context.Context)
	if !ok {
		return nil, fmt.Errorf("exprGetCurveTable() first argument should be context")
	}

	// imagine we need something from the context.
	ctxValue := ctx.Value("test").(string)
	if ctxValue != "value" {
		return nil, fmt.Errorf("invalid context value")
	}

	data, ok := arguments[1].(string)
	if !ok {
		return nil, fmt.Errorf("expected argument 2 to be a string. was: %T", arguments[0])
	}

	return &TestStruct{
		Data: data,
	}, nil
}

type EvaluationData struct {
	Ctx context.Context `expr:"ctx"`
}

type ContextPatcher struct{}

func (p *ContextPatcher) Visit(node *ast.Node) {
	contextType := reflect.TypeOf((*context.Context)(nil)).Elem()

	//nolint:nestif
	if callNode, ok := (*node).(*ast.CallNode); ok {
		// patch context calls so we don't have to pass the context in the code
		if callNode.Func == nil {
			return
		}

		funcType := callNode.Func.Types[0]
		if funcType.NumIn() > 0 {
			firstIn := funcType.In(0)

			if firstIn.Implements(contextType) {
				// prepend patch context calls so we don't need to specify the context in the function invocation
				args := append([]ast.Node{
					&ast.IdentifierNode{
						Value: "ctx",
					},
				}, callNode.Arguments...)

				newNode := &ast.CallNode{
					Callee:    callNode.Callee,
					Arguments: args,
					Typed:     callNode.Typed,
					Fast:      callNode.Fast,
					Func:      callNode.Func,
				}

				ast.Patch(node, newNode)
			}
		}
	}
}

func TestFunctionCallReturnsFloat(t *testing.T) {
	opts := []expr.Option{
		expr.Function("TestMe",
			testMeFunc,
			new(func(context.Context, string) *TestStruct),
		),

		expr.Env(&EvaluationData{}),

		expr.AsFloat64(),

		expr.Patch(&ContextPatcher{}),
	}

	program, err := expr.Compile(`TestMe("1.33").GetFloat()`, opts...)
	if err != nil {
		t.Fatalf("failed to compile script. %v", err)
	}

	ctx := context.WithValue(context.Background(), "test", "value")

	evalData := &EvaluationData{
		Ctx: ctx,
	}

	result, err := expr.Run(program, evalData)
	if err != nil {
		t.Fatalf("failed to run program. %v", err)
	}

	value, ok := result.(float64)
	if !ok {
		t.Errorf("expected result to be a float. was: %T", result)
	}

	if value != 1.33 {
		t.Errorf("expected result to be 1.33, was: %v", value)
	}
}

[rajagopal-chinnaraj]

+1 We have overriden the binary operators (&&, ||) in our code. For simple expression (Func1() || Func2()) it is working as expected, but when we introduce a complex(not so) expression (Func1() || Func2() || Func3()), it is erroring out.

interface conversion: interface {} is CustomEntity, not bool (1:180)

[antonmedv]

I've fixed the error. Now original code also works. See #548

[antonmedv]

expected float64, but got interface {}

First about this issue. Before 1.15.4, AsFloat64() would accept float64 || any. Now it is more explicit, and accepts only float64.

You need to add AsAny():

	opts := []expr.Option{
		expr.AsFloat64(),
+		expr.AsAny(),
	}

More info in #347.

We have overriden the binary operators (&&, ||) in our code. For simple expression (Func1() || Func2()) it is working as expected, but when we introduce a complex(not so) expression (Func1() || Func2() || Func3()), it is erroring out.

Without code, it is unclear. I maybe some bug in your Patcher implementation. Tr to dump ast after patching.

[rajagopal-chinnaraj]

Without code, it is unclear. I maybe some bug in your Patcher implementation. Tr to dump ast after patching.

This is a sample version of the code that we use

package main

import (
	"fmt"

	"github.com/antonmedv/expr"
	"github.com/antonmedv/expr/ast"
)


type RuleEvaluation struct {
	Field1 string
	Field2 int
}


type Env struct {
	Evaluator *Evaluator
}


func (Env) RuleEvaluationOr(a, b *RuleEvaluation) *RuleEvaluation {
	return a
}

func (Env) RuleEvaluationAnd(a, b *RuleEvaluation) *RuleEvaluation {
	return b
}

type Evaluator struct {
	Field1 string
	Field2 int
}

func (e Env) Function1(a int, b string) *RuleEvaluation {
	// read Evaluator struct values and compute response
	return &RuleEvaluation {
		Field1: e.Evaluator.Field1 + b,
		Field2: e.Evaluator.Field2 + a,
	}
}


func main() {
	evaluator := &Evaluator {
		Field1: "abc",
		Field2: 1,
	}

	env := Env {
		Evaluator: evaluator,
	}

	option := []expr.Option{
		expr.AsKind(reflect.TypeOf(&RuleEvaluation{}).Kind()),
		expr.Env(env),
		expr.Operator("||", "RuleEvaluationOr"),
		expr.Operator("&&", "RuleEvaluationAnd"),
	}

	// expression := "Function1(1, \"hello\") || Function1(2, \"hi\")" // works
	expression := "Function1(1, \"hello\") || Function1(2, \"hi\") || Function1(3, \"world\")" // fails

	program, err := expr.Compile(expression, option...)
	if err != nil {
		fmt.Println("complie error ", err.Error())
		return
	}

	expressionResult, err := expr.Run(program, env)
	if err != nil {
		fmt.Println("expr execution error  ", err.Error())
		return
	}

	result := expressionResult.(*RuleEvaluation)
	fmt.Println(result)
}

I am getting the following error

expr execution error   interface conversion: interface {} is main.RuleEvaluation, not bool (1:45)
 | Function1(1, "hello") || Function1(2, "hi") || Function1(3, "world")
 | ............................................^

[tw1nk]

You need to add AsAny():

The script and code it calls in my example is explicitly returning a float, not an any, so I don't understand why we need to add an AsAny, as that would lessen the type safety?

[rajagopal-chinnaraj]

I think the problem is with the operator overloading.
I slightly modified the operator overloading example (https://expr.medv.io/docs/Operator-Overloading) to overload || operator, got same kind of error

package main

import (
	"fmt"

	"github.com/antonmedv/expr"
)

func main() {
	code := `Now() || Now() || CreatedAt`

	options := []expr.Option{
		expr.Env(Env{}),
		expr.Operator("||", "Sub"), // Replace `-` operator with function `Sub`.
	}

	program, err := expr.Compile(code, options...)
	if err != nil {
		panic(err)
	}

	env := Env{
		CreatedAt: 50,
	}

	output, err := expr.Run(program, env)
	if err != nil {
		panic(err)
	}
	fmt.Print(output)
}

type Env struct {
	CreatedAt int
}

func (Env) Now() int         { return 10 }
func (Env) Sub(a, b int) int { return a - b }

response

panic: interface conversion: interface {} is int, not bool (1:16)
 | Now() || Now() || CreatedAt
 | ...............^

[antonmedv]

I've fixed the error. Now original code also works. See #548

[tw1nk]

After a long debugging session I figured out that the checker.Check modifies the ast state by setting the nodeType.

In my patcher I don't want my users to have to pass the context argument to the functions we call as that isn't ergonomic or necessary for the scripters to have to know if the function call requires an context argument or not.

From my example above our users would input: TestMe("1.33").GetFloat() but the actual script that the patcher generates will be TestMe(ctx, "1.33").GetFloat()

So when the checker runs at https://github.com/antonmedv/expr/blob/939aca18e66514ca0727c309f8df8c510a0e2fc1/expr.go#L182C1-L182C8
it will set the return value of the TestMe function to the invalid value interface{} instead of the correct value *scripting.TestStruct as the script is syntactically correct but fails to the checker before we run the patcher.

The change to checker.CallNode introduced in fcf2b55
in combination with
https://github.com/antonmedv/expr/blob/939aca18e66514ca0727c309f8df8c510a0e2fc1/checker/checker.go#L135

causes the issue I have.

Maybe what I need is to have another visitor that runs before performing the typecheck to modify the AST for my usecase?

[antonmedv]

Wow! This is a complex issue. I think you can try to set type from your patcher: node.SetType(). Let's add a test for this case in Expr.

[tw1nk]

I got it to work correctly by explicitly call (*node).SetType(nil) for all nodes in my patcher.

it's a bit weird but at least it solves my issue

type ContextPatcher struct{}

func (p *ContextPatcher) Visit(node *ast.Node) {
	(*node).SetType(nil)
	contextType := reflect.TypeOf((*context.Context)(nil)).Elem()

	//nolint:nestif
	if callNode, ok := (*node).(*ast.CallNode); ok {

		// patch context calls so we don't have to pass the context in the code
		if callNode.Func == nil {
			return
		}

		funcType := callNode.Func.Types[0]
		if funcType.NumIn() > 0 {
			firstIn := funcType.In(0)

			if firstIn.Implements(contextType) {
				// prepend patch context calls so we don't need to specify the context in the function invocation
				args := append([]ast.Node{
					&ast.IdentifierNode{
						Value: "ctx",
					},
				}, callNode.Arguments...)

				newNode := &ast.CallNode{
					Callee:    callNode.Callee,
					Arguments: args,
					Typed:     callNode.Typed,
					Fast:      callNode.Fast,
					Func:      callNode.Func,
				}

				newNode.SetType(funcType.Out(0))

				ast.Patch(node, newNode)
			}
		}
	}
}

[tw1nk]

Actually I think calling '(*node).SetType(nil)' might be required for everyone using a patcher. Including operator overloads.

[antonmedv]

FYI. I've implemented my own version of context patcher and added a config option https://github.com/antonmedv/expr/blob/f92bb7949de064a05d5b3a61d1b07abef0c521bb/expr.go#L157-L161

func ExampleWithContext() {
	env := map[string]any{
		"fn": func(ctx context.Context, _, _ int) int {
			// An infinite loop that can be canceled by context.
			for {
				select {
				case <-ctx.Done():
					return 42
				}
			}
		},
		"ctx": context.TODO(), // Context should be passed as a variable.
	}

	program, err := expr.Compile(`fn(1, 2)`,
		expr.Env(env),
		expr.WithContext("ctx"), // Pass context variable name.
	)
	if err != nil {
		fmt.Printf("%v", err)
		return
	}

	// Cancel context after 100 milliseconds.
	ctx, cancel := context.WithTimeout(context.Background(), time.Millisecond*100)
	defer cancel()

	// After program is compiled, context can be passed to Run.
	env["ctx"] = ctx

	// Run will return 42 after 100 milliseconds.
	output, err := expr.Run(program, env)
	if err != nil {
		fmt.Printf("%v", err)
		return
	}

	fmt.Printf("%v", output)
	// Output: 42
}

[rajagopal-chinnaraj]

Any tips for fixing this error

I slightly modified the operator overloading example (https://expr.medv.io/docs/Operator-Overloading) to overload || operator

Tried SetType using patcher still did not help

package main

import (
	"fmt"

	"github.com/antonmedv/expr"
)

func main() {
	code := `Now() || Now() || CreatedAt`

	options := []expr.Option{
		expr.Env(Env{}),
		expr.Operator("||", "Sub"), // Replace `-` operator with function `Sub`.
	}

	program, err := expr.Compile(code, options...)
	if err != nil {
		panic(err)
	}

	env := Env{
		CreatedAt: 50,
	}

	output, err := expr.Run(program, env)
	if err != nil {
		panic(err)
	}
	fmt.Print(output)
}

type Env struct {
	CreatedAt int
}

func (Env) Now() int         { return 10 }
func (Env) Sub(a, b int) int { return a - b }

response

panic: interface conversion: interface {} is int, not bool (1:16)
 | Now() || Now() || CreatedAt
 | ...............^

[tw1nk]

@rajagopal-chinnaraj
I think you might need to patch the return value of Callee (with the SetType function).
I'm not sure how you would do that in the current API though, without going through a Patch function

[antonmedv]

I fixed the error! Now original code also works.