expr.ConstExpr panics if function was defined with expr.Function instead of env

[diegommm]

Short description: expr.ConstExpr requires that the given function was defined in the environment and does not take into account those functions defined with expr.Function.

Why it matters: This prevents usage of structs as environment in many cases, since it's impractical to define all the functions in the environment struct so we resort to add functionality with expr.Function.

Workaround: It is possible to directly define the signature in the configuration if we directly set the value for the function in the expr/conf.(*Config).ConstFns.

Limitations: a ConstExpr function can have a single signature because for each function name, we can only assign one value in ConstFns. Maybe worth creating a Feature Request issue?

Example (Go Playground)

package main

import (
	"fmt"

	"github.com/expr-lang/expr"
)

type MyEnv struct {
	Value string
}

func identity(x any) any {
	return x
}

func main() {
	code := `[identity(1), identity(Value)]`

	options := []expr.Option{
		expr.Env(MyEnv{}),
		expr.Function(
			"identity",
			func(params ...any) (any, error) {
				return identity(params[0]), nil
			},
			identity,
		),
		expr.ConstExpr("identity"), // Mark identity func as constant expression.
	}

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

	env := MyEnv{
		Value: "thing",
	}
	output, err := expr.Run(program, env)
	if err != nil {
		panic(err)
	}

	fmt.Println(output)

}

Workaround for single-signature (Go Playground)

package main

import (
	"fmt"
	"reflect"

	"github.com/expr-lang/expr"
	"github.com/expr-lang/expr/conf"
)

type MyEnv struct {
	Value string
}

func identity(x any) any {
	return x
}

func main() {
	code := `[identity(1), identity(Value)]`

	options := []expr.Option{
		expr.Env(MyEnv{}),
		expr.Function(
			"identity",
			func(params ...any) (any, error) {
				return identity(params[0]), nil
			},
			identity,
		),
		func(c *conf.Config) {
			c.ConstFns["identity"] = reflect.ValueOf(identity) // Mark identity func as constant expression.
		},
	}

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

	env := MyEnv{
		Value: "thing",
	}
	output, err := expr.Run(program, env)
	if err != nil {
		panic(err)
	}

	fmt.Println(output)

}

[antonmedv]

Yes, i think we need to fix it.

[diegommm]

I can fix this if you want, but I think I would need to make some non-trivial changes, so I will tell you the plan before doing any work to make sure it's ok.

Global idea

• It should be possible that any function can be made ConstExpr.
• A single function can have several signatures, and a call to a function marked as ConstExpr should be optimized regardless of the signature used in the call site.
• Then the current approach should change a bit because ConstExpr can only handle a single signature.

Plan

• Deprecate the ConstFns field of conf.Config because it implies that a single signature can be used for a function.
• Add a new boolean field Const to builtin.Function since we already have all the information that we need here.
• In the constExpr visitor of the optimizer use the functions from the Functions field of conf.Config. This field will be used to: (1) check if the function is marked as Const; and (2) then optimize for any of the signatures.

Handling deprecation of field ConstFns

Because this field is exported then it's part of the public API of the package. This means that in order to keep backward-compatibility it's not possible to just delete the field because someone could be depending on it. My plan to keep backward-compatibility is:

• Mark the field with a deprecation comment. Go doc will understand this and make it very clear that people should not use this anymore. Example from the standard library: function io/ioutil.NopCloser shows visually in a very clear way that this is deprecated. To have a similar deprecation notice it needs a comment saying Deprecated: <message> in the last line of the function comment (see source). Additionally, if you want to use a symbol marked as deprecated then vet will complain.
• The logic that constExpr visitor of the optimizer will use to know if a function is ConstExpr will be: (appears in the ConstFns map) OR (has the new Const field set to true in Functions).
• The signature in the deprecated ConstFns will not be used though, we will instead check the slice of signatures in Functions.

Future

Since builtins are also defined as Function, then a user can also do things like expr.ConstExpr("toJSON"). This would allow toJSON({"name": "John", "age": 30}) to be evaluated at compile time, which some people can find useful to avoid marshaling all the time the same thing every time it is evaluated.
In order to have this working, all we would need to do is to reuse the same logic that we use Functions in the Builtins fields of conf.Config.

[antonmedv]

Deprecate the ConstFns field of conf.Config because it implies that a single signature can be used for a function.

But how functions from env will work?

Add a new boolean field Const to builtin.Function

I think this is correct solution. I like it.

Handling deprecation of field ConstFns

Well, it is exposed but "public" API is via expr.ConstExpr() and other helper functions. So I believe it is save to modify the conf.Config object directly.

[diegommm]

But how functions from env will work?

Currently, we call expr.ConstExpr(name) and it checks the environment. I think it would be good to keep this function as it looks from the outside but internally it would do the following:

1. If the function exists in Functions, then just set the Const field to true.
2. Otherwise, if the function does not exist in Functions but exists in the environment, then first call expr.Function filling out the details for it, and setting the Const field to true.
3. If neither are found, panic (similar to today's behaviour).

Well, it is exposed but "public" API is via expr.ConstExpr() and other helper functions. So I believe it is save to modify the conf.Config object directly.

Nice! So we can make the change without the other complication. I would just add it to the release notes just to make extra sure that people are warned.

[diegommm]

Hey @antonmedv, I was wondering if you were able to check my last message, I just wanted to confirm it looks ok to you before doing any work.

[antonmedv]

I’m on vacation) will try to check it tonight