expr.AsInt allows any, expr.AsBool does not allow any

[patrickdappollonio]

Hey @antonmedv, great library, and appreciate your contribution to the Open Source world!

I've been trying to port my own tabloid app to an expression library like yours (I tried cel-go, but it's still too cumbersome to use) since the library I'm using seems abandonware.

I tried to create a function called ready that would take in a field. If the field says "ready", then it would return true, or false otherwise.

When I tried to plug it in and tell the compiler that the returned expression should be a boolean, I got an error because the return value of an expr.Function is an interface{}, not a boolean (even if the underlying interface value is a boolean).

Here's an example code. This code works, but it does not provide the help of expr.AsBool() (as in, it won't pre-check if the expression does return a boolean or not):

	_, err := expr.Compile("ready(ready)", expr.Function("ready", func(params ...any) (any, error) {
		return params[0].(string) == "ready", nil
	}))

	if err != nil {
		log.Fatal(err.Error())
	}

This code, on the other hand, does not work (note the expr.AsBool() call). It fails with expected bool, but got interface {}:

	_, err = expr.Compile("ready(ready)", expr.AsBool(), expr.Function("ready", func(params ...any) (any, error) {
		return params[0].(string) == "ready", nil
	}))

	if err != nil {
		log.Fatal(err.Error())
	}

Runnable Go playground: https://go.dev/play/p/kAqIUjaLJhX

I'm working around it by simply compiling without the AsBool call, and just checking the returned value once the expression executes, but I feel it could be improved by checking the interface value. I don't know enough about the library though to know whether this would have any side effect.

[patrickdappollonio]

One side note that leds me to believe it might be a bug is the fact that if you negate the expression, it now works. The code below does not return an error:

	_, err = expr.Compile("!ready(ready)", expr.AsBool(), expr.Function("ready", func(params ...any) (any, error) {
		return params[0].(string) == "ready", nil
	}))

[antonmedv]

Hi! Thanks.
I'm going to explain what is going on:

expr.Function("ready", func(params ...any) (any, error) {
     return params[0].(string) == "ready", nil
})

Expr creates a function with a signature: func(params ...any) (any, error).

You specify return type to be bool. Expression ready(ready) return any.

any is not boolean. Expr returns an error.

This is expected behavior.

You can change signature of ready function.

expr.Function(
    "ready", 
    func(params ...any) (any, error) {
        return params[0].(string) == "ready", nil
    },
    new(func(param any) bool),
)

Now Expr knows what signature is func(param any) bool.

Hope my explanation is clear.

[patrickdappollonio]

Hey @antonmedv!

Overall the explanation makes sense and I do get the fact it is returning an interface. Three things make me not believe though it should be intended behaviour:

• The previous comment (negating the expression yields the expected output)
• The fact other types have this intrinsic interface-to-type check right above the one I added for boolean (and you don't have to change the "signature" for those
• All unit tests seem to pass, which would imply there's no breaking change, so nobody was expecting a boolean vs an interface which contains a boolean

It's your library, so I can't do much more there, but it feels off there's different treatment for other types and not this one, so a bit of an inconsistency, and I realize there's the signature piece to make it work so it has a workaround but the inconsistency still remains.

Appreciate your help, I'll keep an eye on the library to see how that evolves over time!

Have a great day!

[antonmedv]

It really depends. Let’s discuss more.

[patrickdappollonio]

Let me know what else you would want in terms of feedback! My statements above still remain, and the fact that this code is here:

https://github.com/antonmedv/expr/blob/master/checker/checker.go#L35-L38

And this code wasn't:

https://github.com/patrickdappollonio/expr/blob/e2216ffedf79bcbf8575c597b85c44c2e8fe46e9/checker/checker.go#L39-L42

In either way, it's your call, though! It's okay if the answer is "yeah, that disparity is there but working as intended".

[antonmedv]

Looks like int and bool got different treatment)

we need to unify it.

I more for removing the any from it.

[patrickdappollonio]

Not sure I understand, int has a treatment there, bool didn't.

[antonmedv]

Yes, this is bad. What about allowing to specify a few return types?

 expr.AsBool(), expr.AsAny()

[patrickdappollonio]

For my case scenario, expr.AsBool() is required. It's expecting to say "here's some data, give me a boolean expression to validate against".

If I add AsAny(), that validation is lost... I can still do it some time after, but it would be good to have the pre-check.

[antonmedv]

In your #348 you just allowed any. The same as the AsAny() option will do.

In your use case you can specify types of your function like this:

expr.Function(
    "ready",
    isready,  
+   new(func(string) bool),
)

As an alternative solution, I propose to add:

expr.AsBoolOrAny()

[patrickdappollonio]

I might be reading it wrong, but it's negated. When I tested that patch against the runnable example above, the code runs correctly (it can detect it was a boolean output, so expr.AsBool() works as intended) and it does reject any other type.

Edit: I honestly don't have a strong opinion on this, so you really don't need my input. The patch seems to work and I'm not sure what solution I'll use so far for my own purposes, I just thought about reporting the inconsistency.

Feel free to close it, take it over, patch it or anything else, without waiting for my input.

[antonmedv]

I'd going to think a little bit more about the correct solution.

[antonmedv]

I've fixed AsBool and added extra option expr.AsAny() which can work with other expr.As* checks.

See ae70b65