Panic/nil when using properties and functions with same name

[scosman]

Thanks for the great project!

I'm getting a invalid memory address or nil pointer dereference panic on the line switch fn.Kind() { in checker.CallNode.

It happens when I have an unknown function, but a variable/property of the same name, with a nil value. This is totally a weird case, but it should probably error and not panic. A nil check on fn and error is probably in order. I've worked around it, but it would be good for future folks.

[antonmedv]

Please, provide an example.

[scosman]

Simplest version here. This panics instead of returning error.

func TestExprDemo(t *testing.T) {
	env := map[string]interface{}{
		"foo": nil,
	}
	code := "foo() > 0"

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

	output, err := expr.Run(program, env)
	if err == nil {
		t.Fatal("expected error from expr, missing function")
	}
}

[scosman]

Somewhat related: what are your thoughts on panic vs error. I see expr using panic throughout the codebase. I believe in golang, returning errors is more idiomatic. I wouldn't want a panic taking out a larger program. Should I wrap expr in defer/recover?

Maybe not directly applicable here (this isn't an intentional call to panic).

[antonmedv]

This is definitely a bug. expr.Compile should not panic. I'm investigating right now.

Regardless panics vs errors: as a library API it is good to only return errors in one way: via errors.

Internally, I prefer to use a thing that makes my life easier. In some cases return error, in others panic with a recover.

Another interesting observation: VM uses panic to increase execution speed. I benchmarked with the same VM but with errors and it was 25% slower.

[scosman]

Got it -- you always aim to wrap your panics with recovers, this is just real bug. That makes sense!