-
-
Notifications
You must be signed in to change notification settings - Fork 414
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ExprNative interface - Allow easier use of custom types that can be represented by go builtin types #460
Conversation
…endly value for evaluation.
I understand your idea, clever solution. But penalty for hot path is way too big. Also, the API naming is little bit confusing. Lets try to find a solution without compromising on speed. I see you used
And getValue should be of type Use a expr.Patch to find all variable what implement the interface and patch with a function call. |
Thank you @antonmedv, that is really good feedback and guidance. I wasn't very happy with the hot path change but wasn't aware of a better approach, so I appreciate it. I'll work on switching this over to a solution using expr.Patch. |
Here is the rough idea of what I'm thinking (https://go.dev/play/p/l75AtVEryUe): package main
import (
"fmt"
"reflect"
"github.com/antonmedv/expr"
"github.com/antonmedv/expr/ast"
)
type Value struct {
Int int
}
type Env struct {
Value Value
}
var valueType = reflect.TypeOf((*Value)(nil)).Elem()
type getValuePatcher struct{}
func (getValuePatcher) Visit(node *ast.Node) {
id, ok := (*node).(*ast.IdentifierNode)
if !ok {
return
}
if id.Type() == valueType {
ast.Patch(node, &ast.CallNode{
Callee: &ast.IdentifierNode{Value: "getValue"},
Arguments: []ast.Node{id},
})
}
}
func main() {
code := `Value + 1`
program, err := expr.Compile(code,
expr.Env(Env{}),
expr.Function("getValue", func(params ...any) (any, error) {
return params[0].(Value).Int, nil
}),
expr.Patch(getValuePatcher{}),
)
if err != nil {
panic(err)
}
env := Env{
Value: Value{1},
}
output, err := expr.Run(program, env)
if err != nil {
panic(err)
}
fmt.Println(output)
} |
The problem that I'm running into with this approach is that type mismatches are not caught at compile time and only at runtime. It's very important for me that as many issues be caught at compile time. Is there a way to do this that would also hint the compiler about the returned type so type issues could be caught? |
It is possible! You can set type in the patcher. Or you can set type in expr.Function: expr.Function("getValue", func(params ...any) (any, error) {
return params[0].(Value).Int, nil
},
+new(func (Value) int)), |
Here is an example pattern that I came up with using interfaces:
Still need to run some benchmarks and see how it compares. |
The patching methods works very well for my use case and performance is good. I'm going to close this PR since it can be accomplished already via patching. If you think it would be helpful to have a builtin interface type for others to use, or want to have some builtin patchers available by default I'd be happy to redo this using the patching method. Thanks for all the help! |
Builtin patches seem like a good idea. |
Do you have an example of how to set the type in the patcher instead of the expr.Function? |
Sure, try: node.SetType() |
That's what I was trying to use, but it doesn't seem to help catch type mismatches at compile time. I just assume I'm doing something wrong. https://go.dev/play/p/hori9Xtni-k Panic happens at I also tried using a type of |
I see. This is definitely strange. I will expect what setting type in Patch will take effect here. Created a issue to track this bug. |
Fixed. Correct code: package set_type_test
import (
"reflect"
"testing"
"github.com/stretchr/testify/require"
"github.com/antonmedv/expr"
"github.com/antonmedv/expr/ast"
)
func TestPatch_SetType(t *testing.T) {
_, err := expr.Compile(
`Value + "string"`,
expr.Env(Env{}),
expr.Function(
"getValue",
func(params ...any) (any, error) {
return params[0].(Value).Int, nil
},
// We can set function type right here,
// but we want to check what SetType in
// getValuePatcher will take an effect.
),
expr.Patch(getValuePatcher{}),
)
require.Error(t, err)
}
type Value struct {
Int int
}
type Env struct {
Value Value
}
var valueType = reflect.TypeOf((*Value)(nil)).Elem()
type getValuePatcher struct{}
func (getValuePatcher) Visit(node *ast.Node) {
id, ok := (*node).(*ast.IdentifierNode)
if !ok {
return
}
if id.Type() == valueType {
newNode := &ast.CallNode{
Callee: &ast.IdentifierNode{Value: "getValue"},
Arguments: []ast.Node{id},
}
newNode.SetType(reflect.TypeOf(0))
ast.Patch(node, newNode)
}
} |
If you wouldn't mind having a look at: And give feedback if you think the overall approach looks good. Still working on adding more testing and documentation. Wasn't sure where to put it. Maybe just 'expr/patchers/value' instead of 'expr/builtin/patchers/value'? Or something else entirely. Thanks! |
This patch set adds the ExprNative interface which allows a type to present itself as a expr native/friendly type for evaluation.
This interface currently lives in the
vm
package. Not sure if there is a better placement for it.It is controlled by the ExprNative() Option. This is set at compile time and is a Program level flag.
The goal is to make it easier to use custom types in expr when they can be represented as builtin go types.
My personal use case is that I am using map[string]interface{} to represent a database record, but the values stored are custom types based on the database column type with additional metadata.
Something along the lines of:
In order to use them with expr there are a few options:
With the ExprNative interface they can be addressed directly in expressions, which allows me to reuse my already present map[string]interface{} and not have to rebuild or book-keep a separate env.
Performance and Benchmarks
We add a check in vm.push() so that before a value is pushed to the stack it is converted to the native type. vm.push() is hot path location, so we guard the type cast behind a configuration setting to prevent a as much of a performance regression as possible
I'm not sure if there is any way to reduce this further. Or a better place in the pipeline to do the conversion.
I also added some benchmarks to compare it on/off and against calling a function to get the value.
The benchmark shows that most of the performance hit from enabling and disabling the option are from the type cast check, and that it is much faster than using a function call on the type.
Anecdotally it is also about 2x faster than creating a new map and converting values for each call to env.
I think it provides a nice usability enhancement to
expr
and look forward to your feedback.