`integer divide by zero` detection broken on master branch #277

Dentrax · 2022-11-08T11:12:01Z

I just updated the expr to latest commit on @master and one of the my unit test is failed. Here is a simple, reproducible code snippet:

package main

import (
	"fmt"
	"github.com/antonmedv/expr"
)

func main() {
	env := map[string]uint64{
		"foo":  7,
		"bar":  0,
	}

	code := `(foo / bar) < 10`

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

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

	fmt.Println(output)
}

I was expecting the following error:

panic: runtime error: integer divide by zero (1:6)
 | (foo / bar) < 10
 | .....^

Works as expected: v1.9.0
Broken (tested at): v1.9.1-0.20221106120435-3d4c21954310

If bar is 0 in the following expression: (foo / bar) < 10, should be resulting an error.

The text was updated successfully, but these errors were encountered:

xio812 · 2022-11-08T19:19:29Z

I can confirm that. As of commit bd73b5a it is broken; for commit before that, i.e. 848ff39e27, it works as expected.

To me it`s not obvious what the problem is.

antonmedv · 2022-11-08T19:27:47Z

This is because of the change: now "/" operation always returns a float. (same as pow ** or ^ also always returns float).

This was done to not confuse non-programmers: why 1 / 2 is equal to 0.

Dentrax · 2022-11-08T19:31:44Z

I am the author of that commit and I think the problem is sort of type changes as Antos said. Can you show me where I need to do apply fix? I can submit a PR if it's a low-hanging fruit. We also need to cover this case by a unit-test.

xio812 · 2022-11-08T19:36:17Z

FTR While float(1)/float(0) returns a valid value, i.e. +Inf, the program panics for int(1)/int(0).

Dentrax · 2022-11-08T19:36:31Z

I think here: https://github.com/antonmedv/expr/blob/6f43069eb7b4982627cec6072b974f4cdb99066a/optimizer/fold.go#L66-L78

Maybe we should check both of n.Left and n.Right either an IntegerNode or FloatNode. Would it be a fix candidate?

xio812 · 2022-11-08T19:56:17Z

Personally, I think it has been a mistake to force the user into floating point arithmetic for division. 1/2 should have remained zero.

If infinity is a valid value in division, why should expr not support it?

antonmedv · 2022-11-08T19:59:46Z

Fixed constant folding for float/ints: b26d4b7

I'm not sure what is the best approach here. For "normal" people (managers, etc), it seems reasonable that 1 / 2 is equals to 0.5.

Dentrax · 2022-11-08T20:01:54Z

Thanks for such a prompt action! 🚀

1/2 should have remained zero.

Why you think 1/2 should be zero? Isn't that also a mistake?

antonmedv · 2022-11-08T20:02:58Z

Why you think 1/2 should be zero? Isn't that also a mistake?

This is how int behaves. Not a mistake from the programmer's perspective. But kinda a mistake from user's perspective.

antonmedv · 2022-11-08T20:05:48Z

For example in https://julialang.org/ 1 / 1 == 1.0.

Dentrax · 2022-11-08T20:07:56Z

I noticed you just changed the following test: https://github.com/antonmedv/expr/blob/b26d4b7339ed214b829ec0a22670941702e59828/expr_test.go#L1388-L1389

But I didn't expect this. Shouldn't 1 / (1 - 1) result in an error? (Since 1/0 is an error) I think it should be as-is.

xio812 · 2022-11-08T20:36:01Z

As of bd73b5a there's no more "integer division". It's all floating point. So, +1/0 == +1.0/0.0 == +Inf, -1/0 == -1.0/0.0 == -Inf, 0/0 == 0.0/0.0 == NaN, and so on. Hence, expr now behaves much more like Julia (which is fine).

antonmedv · 2022-11-08T21:40:15Z

@Dentrax 1 / (1 - 1) is 1/0 is +Inf. So no error should be here.

On the other hand 1 % 0 is an error still.

@roqenrox I'm also thinking to add 1 < 2 < 3 chained comparison. Same as in Julia.

Dentrax · 2022-11-12T23:09:32Z

Hey @antonmedv, we already resolved this issue, but the new behavior still seems a bit wrong to me. And I have some concerns about it:

For example, if both of Foo and Bar is 0, then Foo / Bar < 5 expression resulting false without any error. In this case, this should be resulting in true, obviously. IMHO, the much better UX would be the following:

Foo: 0 and Bar: 0: false, integer divide by zero
Foo > 0 and Bar: 0: false, integer divide by zero
Foo: 0 and Bar > 0: true, nil (true since 0<5)
Foo > 0 and Bar > 0: true, nil (true since 0<5)

I have been struggling to provide a nice and clean UX in current situation; and thinking some possible workaround methods to tackle this problem.

One of possible workaround is: Check if *vm.Program contains any OpDivide operation. If it is, traverse all Constants values to check if all is 0. And return true instead.

In my use case, I print out a value to explain why a given expression is failed. Eventually, mySomeExtractorFunc("Foo / Bar < 5") returns [0 0]. And use might think, "Oh, why 0<5 is false?".

Wdyt?

antonmedv · 2022-11-12T23:38:22Z

Well, 0 / 0 is NaN in floats arithmetics. This is by design. We should stick to IEEE 754.

Is case we want to return an error, simply add a check in OpDevide itself =)

Dentrax · 2022-11-13T06:53:56Z

Hmmm, I see. Thanks!

simply add a check in OpDevide itself

Can you please clarify this a bit, you mean I should pass a custom patch function to my expression or submit a PR to add this check at switch case?

antonmedv · 2022-11-13T07:21:11Z

Right now only option is to replace / to a func call via Patch.

Dentrax · 2022-11-14T09:39:26Z

Right now only option is to replace / to a func call via Patch.

Can you please share a code snippet, how can I tackle with this problem? I struggled here:

type divPatcher struct{}

func (p *divPatcher) Visit(node *ast.Node) {
	n, ok := (*node).(*ast.BinaryNode)
	if !ok {
		return
	}
	if n.Operator == "/" {
		// TODO: Check if the right operand is a _number_ with value 0.
		// If so, expr.Compile will return an error.
	}
}

xio812 · 2022-11-14T09:57:48Z

Isn't the code here a good starting point to write that custom patch function?

If you still have problems, I will take a look at it again later from my machine.

Dentrax · 2022-11-14T10:09:29Z

Isn't the code here a good starting point to write that custom patch function?

Thanks for the ref. Patch funcs seems easy to write but the problem is Visit(ast.Node) not returning any error. I'm not sure how can I handle x/0 case using patch.

antonmedv · 2022-11-14T10:19:28Z

During the patch you don’t know if var a zero.

You need to replace / with a func call. In a func call throw an error.

Dentrax · 2022-11-14T11:05:29Z

I created CallNode but couldn't figure out how to pass my function to Node itself:

divFn := func(left, right float64) float64 {
	if right == 0 {
		panic("integer divide by zero")
	}
	return left / right
}

ast.Patch(node, &ast.CallNode{
	// Callee:    ???,
	Arguments: []ast.Node{n.Left, n.Right},
})

I don't want to pass like &ast.IdentifierNode{Value: "Div"} to here because otherwise I would have to mutate input env map[string]any to add such Div() func as a new key in every request.

antonmedv · 2022-11-14T12:23:20Z

Yes, right now it’s the only option to pass a func from env.

We can add something to CallNode, maybe func field.

Dentrax · 2022-11-14T12:32:55Z

Created a separate issue to discuss further about this: #287

Maybe I can get to it if you enlight my way.

Dentrax · 2023-09-17T13:28:22Z

Hi @antonmedv, thank you for adding Func support to CallNode. I just hit this issue again and I was wondering how can I tackle with that new Func field. I created simple snipped that currently returns:

true
false
true
false
false

But what I'm trying to accomplish is to return an error instead of evaluate those. As you said already, IEEE 754 mentions that diving to zero for floats are not error. But I'm mostly working with integer values. So in my case, it resulting a bug.

You need to replace / with a func call. In a func call throw an error.

package main

import (
	"fmt"
	"github.com/antonmedv/expr"
	"github.com/antonmedv/expr/ast"
)

func main() {
	env := map[string]uint64{
		"foo": 7,
		"bar": 0,
	}

	tests := []struct {
		input string
	}{
		{
			input: "(foo / bar) > 10",
		},
		{
			input: "10 > (foo / bar)",
		},
		{
			input: "10 + 20 + 30 < (foo / bar)",
		},
		{
			input: "10 + 20 + 30 > (foo / bar) - 40",
		},
		{
			input: "10 > (foo / bar) + 20 + 30 + 40 + 50",
		},
	}

	for _, t := range tests {
		program, err := expr.Compile(t.input, expr.Env(env))
		if err != nil {
			panic(err)
		}

		// Patch
		if n, ok := (program.Node).(*ast.BinaryNode); ok {
			div := &ast.Function{
				Name: "integer divide by zero",
				Func: func(args ...any) (any, error) {
					// TODO
					return nil, nil
				},
				//Fast:      nil,
				//Types:     nil,
				//Validate:  nil,
				//Predicate: false,
			}

			ast.Patch(&program.Node, &ast.CallNode{
				Func:      div,
				Arguments: []ast.Node{n.Left, n.Right},
			})
		}

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

		fmt.Println(output)
	}
}

Could you please enlighten my way here? What do put in // TODO section? All 5 tests should throw an error in this case.

antonmedv · 2023-09-17T14:07:38Z

Here is the patch doc: https://expr.medv.io/docs/Visitor-and-Patch

			Func: func(args ...any) (any, error) {
				return args[0] / args[1], nil
			},

Dentrax · 2023-09-17T14:23:23Z

Thanks, I checked and I'm not sure if it covers my scenario. Tried this one but it does not throw any error:

package main

import (
	"fmt"
	"github.com/antonmedv/expr"
	"github.com/antonmedv/expr/ast"
)

func main() {
	env := map[string]uint64{
		"foo": 7,
		"bar": 0,
	}

	tests := []struct {
		input string
	}{
		{
			input: "(foo / bar) > 10",
		},
		{
			input: "10 > (foo / bar)",
		},
		{
			input: "10 + 20 + 30 < (foo / bar)",
		},
		{
			input: "10 + 20 + 30 > (foo / bar) - 40",
		},
		{
			input: "10 > (foo / bar) + 20 + 30 + 40 + 50",
		},
	}

	for _, t := range tests {
		program, err := expr.Compile(t.input, expr.Env(env))
		if err != nil {
			panic(err)
		}

		// Patch
		if n, ok := (program.Node).(*ast.BinaryNode); ok {
			div := &ast.Function{
				Name: "integer divide by zero",
				Func: func(args ...any) (any, error) {
					// TODO: how to handle if len(args) != 2?
					v1, ok1 := args[0].(uint64)
					if !ok1 {
						return nil, fmt.Errorf("first argument is not integer")
					}
					v2, ok2 := args[1].(uint64)
					if !ok2 {
						return nil, fmt.Errorf("second argument is not integer")
					}
					if v2 == 0 {
						return nil, fmt.Errorf("integer divide by zero")
					}
					return v1 / v2, nil
				},
			}

			ast.Patch(&program.Node, &ast.CallNode{
				Func:      div,
				Arguments: []ast.Node{n.Left, n.Right},
			})
		}

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

		fmt.Println(output)
	}
}

Prints:

true
false
true
false
false

My some questions here:

Since args is any type, should I brute-forcely check every primitive built-in types? (int, uint, etc.)
What would happen if args len is not 2? How to skip this Func in that case?

Thanks.

Dentrax · 2023-09-17T14:30:26Z

Tried visitor way as well but it throws panic: undefined node type (<nil>).

package main

import (
	"fmt"
	"github.com/antonmedv/expr"
	"github.com/antonmedv/expr/ast"
)

func main() {
	env := map[string]uint64{
		"foo": 7,
		"bar": 0,
	}

	tests := []struct {
		input string
	}{
		{
			input: "(foo / bar) > 10",
		},
		{
			input: "10 > (foo / bar)",
		},
		{
			input: "10 + 20 + 30 < (foo / bar)",
		},
		{
			input: "10 + 20 + 30 > (foo / bar) - 40",
		},
		{
			input: "10 > (foo / bar) + 20 + 30 + 40 + 50",
		},
	}

	for _, t := range tests {
		program, err := expr.Compile(t.input, expr.Env(env), expr.Patch(&visitor{}))
		if err != nil {
			panic(err)
		}

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

		fmt.Println(output)
	}
}

type visitor struct{}

func (v *visitor) Visit(node *ast.Node) {
	if n, ok := (*node).(*ast.BinaryNode); ok {
		div := &ast.Function{
			Name: "integer divide by zero",
			Func: func(args ...any) (any, error) {
				// TODO: how to handle if len(args) != 2?
				v1, ok1 := args[0].(uint64)
				if !ok1 {
					return nil, fmt.Errorf("first argument is not integer")
				}
				v2, ok2 := args[1].(uint64)
				if !ok2 {
					return nil, fmt.Errorf("second argument is not integer")
				}
				if v2 == 0 {
					return nil, fmt.Errorf("integer divide by zero")
				}
				return v1 / v2, nil
			},
		}

		ast.Patch(node, &ast.CallNode{
			Func:      div,
			Arguments: []ast.Node{n.Left, n.Right},
		})
	}
}

antonmedv · 2023-09-17T14:55:25Z

Your first example is incorrect. It will not work. Visitor with Patch is the correct way.

Dentrax · 2023-09-17T18:41:23Z

I couldn't figure it out what to put into Callee field. Is my Visit function wrong? It panics: panic: unknown name _div

func (v *visitor) Visit(node *ast.Node) {
	n, ok := (*node).(*ast.BinaryNode)
	if !ok {
		return
	}

	lid, lok := n.Left.(*ast.IdentifierNode)
	rid, rok := n.Right.(*ast.IdentifierNode)

	switch {
	case lok && rok && n.Operator == "/":
		ast.Patch(node, &ast.CallNode{
			Callee: &ast.IdentifierNode{Value: "_div", Method: true},
			Func: &ast.Function{
				Name: "_div",
				Func: func(args ...any) (any, error) {
					return -1, nil
				},
			},
			Arguments: []ast.Node{lid, rid},
		})
	}
}

antonmedv · 2023-09-17T19:03:07Z

You can also just try: https://expr.medv.io/docs/Operator-Overloading

Dentrax · 2023-09-17T19:44:44Z

You can also just try: expr.medv.io/docs/Operator-Overloading

Thanks! I didn't pass expr.Operator to []expr.Option but adding dummy "_div": func(a, b any) any { return nil }, value to my env make my patch work like a charm!

But adding this dummy function really necessary? Is there any other way without including a function in env?

antonmedv · 2023-09-17T19:52:17Z

Yeap, it is possible via expr.Function. But right now there is a bug #408

antonmedv closed this as completed Nov 8, 2022

Dentrax mentioned this issue Nov 14, 2022

Ability to pass custom Func to CallNode #287

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`integer divide by zero` detection broken on master branch #277

`integer divide by zero` detection broken on master branch #277

Dentrax commented Nov 8, 2022 •

edited

Loading

xio812 commented Nov 8, 2022 •

edited

Loading

antonmedv commented Nov 8, 2022

Dentrax commented Nov 8, 2022 •

edited

Loading

xio812 commented Nov 8, 2022

Dentrax commented Nov 8, 2022

xio812 commented Nov 8, 2022

antonmedv commented Nov 8, 2022

Dentrax commented Nov 8, 2022

antonmedv commented Nov 8, 2022

antonmedv commented Nov 8, 2022

Dentrax commented Nov 8, 2022 •

edited

Loading

xio812 commented Nov 8, 2022 •

edited

Loading

antonmedv commented Nov 8, 2022

Dentrax commented Nov 12, 2022 •

edited

Loading

antonmedv commented Nov 12, 2022

Dentrax commented Nov 13, 2022 •

edited

Loading

antonmedv commented Nov 13, 2022

Dentrax commented Nov 14, 2022

xio812 commented Nov 14, 2022 •

edited

Loading

Dentrax commented Nov 14, 2022

antonmedv commented Nov 14, 2022

Dentrax commented Nov 14, 2022

antonmedv commented Nov 14, 2022

Dentrax commented Nov 14, 2022

Dentrax commented Sep 17, 2023

antonmedv commented Sep 17, 2023 •

edited

Loading

Dentrax commented Sep 17, 2023 •

edited

Loading

Dentrax commented Sep 17, 2023

antonmedv commented Sep 17, 2023

Dentrax commented Sep 17, 2023 •

edited

Loading

antonmedv commented Sep 17, 2023

Dentrax commented Sep 17, 2023 •

edited

Loading

antonmedv commented Sep 17, 2023

integer divide by zero detection broken on master branch #277

integer divide by zero detection broken on master branch #277

Comments

Dentrax commented Nov 8, 2022 • edited Loading

xio812 commented Nov 8, 2022 • edited Loading

antonmedv commented Nov 8, 2022

Dentrax commented Nov 8, 2022 • edited Loading

xio812 commented Nov 8, 2022

Dentrax commented Nov 8, 2022

xio812 commented Nov 8, 2022

antonmedv commented Nov 8, 2022

Dentrax commented Nov 8, 2022

antonmedv commented Nov 8, 2022

antonmedv commented Nov 8, 2022

Dentrax commented Nov 8, 2022 • edited Loading

xio812 commented Nov 8, 2022 • edited Loading

antonmedv commented Nov 8, 2022

Dentrax commented Nov 12, 2022 • edited Loading

antonmedv commented Nov 12, 2022

Dentrax commented Nov 13, 2022 • edited Loading

antonmedv commented Nov 13, 2022

Dentrax commented Nov 14, 2022

xio812 commented Nov 14, 2022 • edited Loading

Dentrax commented Nov 14, 2022

antonmedv commented Nov 14, 2022

Dentrax commented Nov 14, 2022

antonmedv commented Nov 14, 2022

Dentrax commented Nov 14, 2022

Dentrax commented Sep 17, 2023

antonmedv commented Sep 17, 2023 • edited Loading

Dentrax commented Sep 17, 2023 • edited Loading

Dentrax commented Sep 17, 2023

antonmedv commented Sep 17, 2023

Dentrax commented Sep 17, 2023 • edited Loading

antonmedv commented Sep 17, 2023

Dentrax commented Sep 17, 2023 • edited Loading

antonmedv commented Sep 17, 2023

`integer divide by zero` detection broken on master branch #277

`integer divide by zero` detection broken on master branch #277

Dentrax commented Nov 8, 2022 •

edited

Loading

xio812 commented Nov 8, 2022 •

edited

Loading

Dentrax commented Nov 8, 2022 •

edited

Loading

Dentrax commented Nov 8, 2022 •

edited

Loading

xio812 commented Nov 8, 2022 •

edited

Loading

Dentrax commented Nov 12, 2022 •

edited

Loading

Dentrax commented Nov 13, 2022 •

edited

Loading

xio812 commented Nov 14, 2022 •

edited

Loading

antonmedv commented Sep 17, 2023 •

edited

Loading

Dentrax commented Sep 17, 2023 •

edited

Loading

Dentrax commented Sep 17, 2023 •

edited

Loading

Dentrax commented Sep 17, 2023 •

edited

Loading