expr icon indicating copy to clipboard operation
expr copied to clipboard

feat: add disable short-circuiting option to compiler

Open lvliangxiong opened this issue 3 months ago • 6 comments

#831

lvliangxiong avatar Sep 25 '25 07:09 lvliangxiong

I just fix the code according to the check result. It'll be good now.

lvliangxiong avatar Sep 27 '25 02:09 lvliangxiong

Nice! Let me review this more carefully.

antonmedv avatar Sep 29 '25 09:09 antonmedv

We could implement this with less core changes if we follow your example here.

My suggestion is the following (Go Playground working example):

// DisableShortCircuit turns short circuiting behaviour off in `&&`, `and`, `||`, and `or` boolean operators.
func DisableShortCircuit() expr.Option {
	return func(c *conf.Config) {
		expr.Function("OR", func(params ...any) (any, error) {
			return params[0].(bool) || params[1].(bool), nil
		}, func(a, b bool) bool { return true })(c)
		expr.Function("AND", func(params ...any) (any, error) {
			return params[0].(bool) && params[1].(bool), nil
		}, func(a, b bool) bool { return true })(c)
		expr.Operator("or", "OR")(c)
		expr.Operator("||", "OR")(c)
		expr.Operator("and", "AND")(c)
		expr.Operator("&&", "AND")(c)
	}
}

This way we will not need any changes to vm and compiler packages and the core language remains smaller.

diegommm avatar Oct 25 '25 15:10 diegommm

We could implement this with less core changes if we follow your example here.

My suggestion is the following (Go Playground working example):

// DisableShortCircuit turns short circuiting behaviour off in `&&`, `and`, `||`, and `or` boolean operators.
func DisableShortCircuit() expr.Option {
	return func(c *conf.Config) {
		expr.Function("OR", func(params ...any) (any, error) {
			return params[0].(bool) || params[1].(bool), nil
		}, func(a, b bool) bool { return true })(c)
		expr.Function("AND", func(params ...any) (any, error) {
			return params[0].(bool) && params[1].(bool), nil
		}, func(a, b bool) bool { return true })(c)
		expr.Operator("or", "OR")(c)
		expr.Operator("||", "OR")(c)
		expr.Operator("and", "AND")(c)
		expr.Operator("&&", "AND")(c)
	}
}

This way we will not need any changes to vm and compiler packages and the core language remains smaller.

I also considered this solution at first glance. However, after spending some time investigating the implementation, I realized this more intuitive solution might potentially conflict with or be overridden by other patch/optimization features. I'm not sure if my concerns are unnecessary, so I've implemented this in the compiler & vm package, which I find easier to understand and might be more robust without bugs.

On the other hand, I give it a try and find something interesting. Function here needs type information to be matched with operator. So if we don't give the Env obj when compiling, this option might not take effect, illustrated by the example below:

func TestDisableShortCircuit(t *testing.T) {
	count := 0
	exprStr := "foo() or bar()"
	env := map[string]any{
		"foo": func() bool {
			count++
			return true
		},
		"bar": func() bool {
			count++
			return true
		},
	}

	// You must set expr.Env(env) to make expr.DisableShortCircuit() to be effective.
	program, _ := expr.Compile(exprStr, expr.DisableShortCircuit(), expr.Env(env))
	got, _ := expr.Run(program, env)
	assert.Equal(t, 2, count)
	assert.True(t, got.(bool))

	program, _ = expr.Compile(exprStr)
	got, _ = expr.Run(program, env)
	assert.Equal(t, 3, count)
	assert.True(t, got.(bool))
}

Since I don't have much time to figure out every detail of the implementation, I'm not sure if this operator overloading solution has other hidden bugs or limitations, or we can solve this by some other tricks? What do you think about this? Looking forward to your reply.

lvliangxiong avatar Oct 26 '25 16:10 lvliangxiong

I think proposal solution in this poor request is proper. Disable short circuit should generate different bytecode.

antonmedv avatar Oct 26 '25 17:10 antonmedv

Any progress here? Let me know if I can help.

lvliangxiong avatar Nov 13 '25 06:11 lvliangxiong