Skip to content

fix: rework nilsafe logic with subProperty ops #210

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

fix: rework nilsafe logic with subProperty ops #210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions ast/node.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,7 @@ type NilNode struct {

type IdentifierNode struct {
base
Value string
NilSafe bool
Value string
}

type IntegerNode struct {
Expand Down
11 changes: 4 additions & 7 deletions checker/checker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,7 @@ func (v *visitor) IdentifierNode(node *ast.IdentifierNode) reflect.Type {
}
return interfaceType
}
if !node.NilSafe {
return v.error(node, "unknown name %v", node.Value)
}
return nilType
return v.error(node, "unknown name %v", node.Value)
}

func (v *visitor) IntegerNode(*ast.IntegerNode) reflect.Type {
Expand Down Expand Up @@ -348,9 +345,9 @@ func (v *visitor) FunctionNode(node *ast.FunctionNode) reflect.Type {
fn.NumIn() == inputParamsCount &&
((fn.NumOut() == 1 && // Function with one return value
fn.Out(0).Kind() == reflect.Interface) ||
(fn.NumOut() == 2 && // Function with one return value and an error
fn.Out(0).Kind() == reflect.Interface &&
fn.Out(1) == errorType)) {
(fn.NumOut() == 2 && // Function with one return value and an error
fn.Out(0).Kind() == reflect.Interface &&
fn.Out(1) == errorType)) {
rest := fn.In(fn.NumIn() - 1) // function has only one param for functions and two for methods
if rest.Kind() == reflect.Slice && rest.Elem().Kind() == reflect.Interface {
node.Fast = true
Expand Down
2 changes: 0 additions & 2 deletions compiler/compiler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,8 +180,6 @@ func (c *compiler) IdentifierNode(node *ast.IdentifierNode) {
v := c.makeConstant(node.Value)
if c.mapEnv {
c.emit(OpFetchMap, v...)
} else if node.NilSafe {
c.emit(OpFetchNilSafe, v...)
} else {
c.emit(OpFetch, v...)
}
Expand Down
12 changes: 6 additions & 6 deletions expr_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -946,6 +946,7 @@ func TestExpr_map_default_values(t *testing.T) {

func TestExpr_nil_safe(t *testing.T) {
env := map[string]interface{}{
"foo": struct{}{},
"bar": map[string]*string{},
}

Expand All @@ -961,21 +962,19 @@ func TestExpr_nil_safe(t *testing.T) {

func TestExpr_nil_safe_first_ident(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still a valid use case. If foo? is nil the entire chain should be considered safe

Copy link
Contributor Author

@tufitko tufitko Dec 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really, just type foo and you will get an error unknown name foo, to avoid this error you can use AllowUndefinedVariables option. (or we can support something like ?.foo)

foo?.bar - here operator ?. should be affect bar property only
foo.bar?.baz - and here only on baz, bar (like foo above) isn't nilsafe

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not about foo being the first variable, it's about the rest of the chain being safe after the nil operator.

If you have foo.missing?.test.err having missing be nil should keep the rest of the chain safe, a nil test shouldn't cause the compiler to error on err unless missing is not nil

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO not supporting the nil operator on the first identifier would be a weird inconsistency but at the very least you need to support this use case: https://github.com/antonmedv/expr/pull/211/files#diff-c7a89724039da4beba41207c0d28a3d27c3474ceee39acedf5ff5eb3207691e4R997

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, maybe we need to add option to js-like fetch from objects?
in go getting an unknown property is forbidden and returned error
in js getting an unknown property returned undefined

env := map[string]interface{}{
"foo": struct{}{},
"bar": map[string]*string{},
}

input := `foo?.missing.test == '' && bar['missing'] == nil`

program, err := expr.Compile(input, expr.Env(env))
require.NoError(t, err)

output, err := expr.Run(program, env)
require.NoError(t, err)
require.Equal(t, false, output)
_, err := expr.Compile(input, expr.Env(env))
require.Error(t, err)
}

func TestExpr_nil_safe_not_strict(t *testing.T) {
env := map[string]interface{}{
"foo": struct{}{},
"bar": map[string]*string{},
}

Expand Down Expand Up @@ -1011,6 +1010,7 @@ func TestExpr_nil_safe_valid_value(t *testing.T) {

func TestExpr_nil_safe_method(t *testing.T) {
env := map[string]interface{}{
"foo": struct{}{},
"bar": map[string]*string{},
}

Expand Down
13 changes: 4 additions & 9 deletions parser/parser.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ func (p *parser) parsePrimaryExpression() Node {
node.SetLocation(token.Location)
return node
default:
node = p.parseIdentifierExpression(token, p.current)
node = p.parseIdentifierExpression(token)
}

case Number:
Expand Down Expand Up @@ -334,7 +334,7 @@ func (p *parser) parsePrimaryExpression() Node {
return p.parsePostfixExpression(node)
}

func (p *parser) parseIdentifierExpression(token, next Token) Node {
func (p *parser) parseIdentifierExpression(token Token) Node {
var node Node
if p.current.Is(Bracket, "(") {
var arguments []Node
Expand Down Expand Up @@ -367,11 +367,7 @@ func (p *parser) parseIdentifierExpression(token, next Token) Node {
node.SetLocation(token.Location)
}
} else {
var nilsafe bool
if next.Value == "?." {
nilsafe = true
}
node = &IdentifierNode{Value: token.Value, NilSafe: nilsafe}
node = &IdentifierNode{Value: token.Value}
node.SetLocation(token.Location)
}
return node
Expand Down Expand Up @@ -464,14 +460,13 @@ end:

func (p *parser) parsePostfixExpression(node Node) Node {
token := p.current
var nilsafe bool
for (token.Is(Operator) || token.Is(Bracket)) && p.err == nil {
if token.Value == "." || token.Value == "?." {
var nilsafe bool
if token.Value == "?." {
nilsafe = true
}
p.next()

token = p.current
p.next()

Expand Down
10 changes: 9 additions & 1 deletion parser/parser_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ func TestParse(t *testing.T) {
},
{
"foo?.bar",
&ast.PropertyNode{Node: &ast.IdentifierNode{Value: "foo", NilSafe: true}, Property: "bar", NilSafe: true},
&ast.PropertyNode{Node: &ast.IdentifierNode{Value: "foo"}, Property: "bar", NilSafe: true},
},
{
"foo['all']",
Expand Down Expand Up @@ -238,6 +238,14 @@ func TestParse(t *testing.T) {
"[1, 2, 3,]",
&ast.ArrayNode{Nodes: []ast.Node{&ast.IntegerNode{Value: 1}, &ast.IntegerNode{Value: 2}, &ast.IntegerNode{Value: 3}}},
},
{
"a?.b.c",
&ast.PropertyNode{Node: &ast.PropertyNode{Node: &ast.IdentifierNode{Value: "a"}, Property: "b", NilSafe: true}, Property: "c", NilSafe: false},
},
{
"a?.b?.c",
&ast.PropertyNode{Node: &ast.PropertyNode{Node: &ast.IdentifierNode{Value: "a"}, Property: "b", NilSafe: true}, Property: "c", NilSafe: true},
},
}
for _, test := range parseTests {
actual, err := parser.Parse(test.input)
Expand Down
1 change: 0 additions & 1 deletion vm/opcodes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ const (
OpPop
OpRot
OpFetch
OpFetchNilSafe
OpFetchMap
OpTrue
OpFalse
Expand Down
3 changes: 0 additions & 3 deletions vm/program.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,9 +73,6 @@ func (program *Program) Disassemble() string {
case OpFetch:
constant("OpFetch")

case OpFetchNilSafe:
constant("OpFetchNilSafe")

case OpFetchMap:
constant("OpFetchMap")

Expand Down
3 changes: 0 additions & 3 deletions vm/vm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,9 +102,6 @@ func (vm *VM) Run(program *Program, env interface{}) (out interface{}, err error
case OpFetch:
vm.push(fetch(env, vm.constant(), false))

case OpFetchNilSafe:
vm.push(fetch(env, vm.constant(), true))

case OpFetchMap:
vm.push(env.(map[string]interface{})[vm.constant().(string)])

Expand Down