feat: add disable short-circuiting option to compiler

[lvliangxiong]

#831

[lvliangxiong]

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

[antonmedv]

Nice! Let me review this more carefully.

[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.

[lvliangxiong]

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.

[antonmedv]

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