Skip to content

support "is nil" and "is not nil" and make "is empty" safer #129

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

Merged
tgross merged 2 commits into from
Mar 5, 2026
Merged

support "is nil" and "is not nil" and make "is empty" safer #129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
72d68f2
support "is nil" and "is not nil" and make "is empty" safer
tgross Mar 5, 2026
8a4f137
address comments on code review
tgross Mar 5, 2026
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 bexpr_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ func TestCreateEvaluator(t *testing.T) {
},
"default max expressions": {
expression: "((((((((foo == 1))))))))",
// typo in pigeon code-gen
Copy link
Copy Markdown
Member Author

@tgross tgross Mar 5, 2026

Choose a reason for hiding this comment

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

Note to reviewers: not sure which version of pigeon fixed this bug. We don't have the version pinned in this repo, but that's out of scope for this PR.

err: "max number of expresssions parsed",
err: "max number of expressions parsed",
},
}

Expand Down
15 changes: 8 additions & 7 deletions common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,13 +83,14 @@ type testNestedLevel2_2 struct {
}

type testNestedLevel1 struct {
Map map[string]string
MapOfStructs map[string]testNestedLevel2_1
MapInfInf map[interface{}]interface{}
SliceOfInts []int
SliceOfStructs []testNestedLevel2_2
SliceOfMapInfInf []map[interface{}]interface{}
SliceOfInfs []interface{}
Map map[string]string
MapOfStructs map[string]testNestedLevel2_1
MapInfInf map[interface{}]interface{}
SliceOfInts []int
SliceOfStructs []testNestedLevel2_2
SliceOfMapInfInf []map[interface{}]interface{}
SliceOfInfs []interface{}
SliceOfPointersToStructs []*testNestedLevel2_2
}

type testNestedTypes struct {
Expand Down
39 changes: 37 additions & 2 deletions evaluate.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,35 @@ func doMatchIn(expression *grammar.MatchExpression, value reflect.Value) (bool,
}

func doMatchIsEmpty(matcher *grammar.MatchExpression, value reflect.Value) (bool, error) {
// NOTE: see preconditions in evaluategrammar.MatchExpressionRecurse
return value.Len() == 0, nil
switch kind := value.Kind(); kind {
case reflect.Map, reflect.Slice, reflect.Array, reflect.Chan, reflect.String:
return value.Len() == 0, nil
default:
return false, fmt.Errorf(
"cannot perform is-empty operations on type %s for selector: %q", kind, matcher.Selector)
}
}

func doMatchIsNil(matcher *grammar.MatchExpression, value reflect.Value) (bool, error) {
switch kind := value.Kind(); kind {
case reflect.Map, reflect.Slice, reflect.Array, reflect.Chan:
return value.IsNil(), nil
case reflect.Struct:
// a non-nil pointer to a struct will also have this type when
// dereferenced by the caller
return false, nil
case reflect.Invalid:
// a nil pointer to a struct will have this type when dereferenced by
// the caller
return true, nil
case reflect.Pointer:
// the caller should be chasing pointers-to-pointers but handle this for
// robustness
return value.IsNil(), nil
default:
return false, fmt.Errorf(
"cannot perform is-nil operations on type %s for selector: %q", kind, matcher.Selector)
}
}

func getMatchExprValue(expression *grammar.MatchExpression, rvalue reflect.Kind) (interface{}, error) {
Expand Down Expand Up @@ -369,6 +396,14 @@ func evaluateMatchExpression(expression *grammar.MatchExpression, datum interfac
return !result, nil
}
return false, err
case grammar.MatchIsNil:
return doMatchIsNil(expression, rvalue)
case grammar.MatchIsNotNil:
result, err := doMatchIsNil(expression, rvalue)
if err == nil {
return !result, nil
}
return false, err
default:
return false, fmt.Errorf("invalid match operation: %d", expression.Operator)
}
Expand Down
10 changes: 10 additions & 0 deletions evaluate_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,10 @@ var evaluateTests map[string]expressionTest = map[string]expressionTest{
},
},
SliceOfInfs: []interface{}{"foobar", 1, true},
SliceOfPointersToStructs: []*testNestedLevel2_2{
{X: 1, Y: 2, z: 10},
nil,
},
},
TopInt: 5,
},
Expand Down Expand Up @@ -357,6 +361,12 @@ var evaluateTests map[string]expressionTest = map[string]expressionTest{
{expression: `any Nested.Map as k, v { k == "foo" and v == "bar" }`, result: true},
{expression: `any Nested.Map as k { k.Color == "red" }`, err: "/k references a string so /k/Color is invalid"},
{expression: `any Nested.SliceOfInts as i, _ { i.Color == "red" }`, err: "/i references a int so /i/Color is invalid"},
{expression: `Nested.SliceOfPointersToStructs.0 is empty`, err: `cannot perform is-empty operations on type struct for selector: "Nested.SliceOfPointersToStructs.0"`},
{expression: `Nested.SliceOfPointersToStructs.1 is empty`, err: `cannot perform is-empty operations on type invalid for selector: "Nested.SliceOfPointersToStructs.1"`},
{expression: `Nested.SliceOfPointersToStructs.0 is nil`, result: false},
{expression: `Nested.SliceOfPointersToStructs.1 is nil`, result: true},
{expression: `Nested.SliceOfPointersToStructs.0 is not nil`, result: true},
{expression: `Nested.SliceOfPointersToStructs.1 is not nil`, result: false},
},
},
}
Expand Down
12 changes: 12 additions & 0 deletions grammar/ast.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ const (
MatchIsNotEmpty
MatchMatches
MatchNotMatches
MatchIsNil
MatchIsNotNil
)

func (op MatchOperator) String() string {
Expand All @@ -79,6 +81,10 @@ func (op MatchOperator) String() string {
return "Matches"
case MatchNotMatches:
return "Not Matches"
case MatchIsNil:
return "Is Nil"
case MatchIsNotNil:
return "Not Nil"
default:
return "UNKNOWN"
}
Expand Down Expand Up @@ -113,6 +119,12 @@ func (op MatchOperator) NotPresentDisposition() bool {
case MatchNotMatches:
// M["x"] not matches <anything> is true. Nothing matches a missing key
return true
case MatchIsNil:
// M["x"] is nil is true. Missing keys have no value.
return true
case MatchIsNotNil:
// M["x"] is not nil is false. Missing keys have no value.
return false
Comment on lines +122 to +127
Copy link
Copy Markdown
Member

@gulducat gulducat Mar 5, 2026

Choose a reason for hiding this comment

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

Up front, I won't lose any sleep on this, but for pedantic conversation, MatchIsNil => true here has me torn.

I see this true/false binary aligns with the Is/Not pairs above, but here the positive case feels subtly off to me: The the absence of a value is not nil, but this reads to me like "it's there, and it's nil." I suppose if we assume M is a map of pointers, then the zero value (of a v, ok := M["x"]) will be nil, but if it's map[string]string, then the zero value is "", which is not nil! So really MatchIsNil depends on the map type, which we don't know here, so it's more like return 🤷

But a shrug isn't a bool, and I imagine that people holding this new expression will get the results they expect with these values as they are, so as I said, I won't lose sleep over it.

With that, I do think the comment on MatchIsNotNil is a tad off:

Suggested change
case MatchIsNil:
// M["x"] is nil is true. Missing keys have no value.
return true
case MatchIsNotNil:
// M["x"] is nil is false. Missing keys have no value.
return false
case MatchIsNil:
// M["x"] is nil is true. Missing keys have no value.
return true
case MatchIsNotNil:
// M["x"] is not nil is false. Missing keys have no value.
return false

right?

Copy link
Copy Markdown
Member Author

@tgross tgross Mar 5, 2026

Choose a reason for hiding this comment

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

I suppose if we assume M is a map of pointers, then the zero value (of a v, ok := M["x"]) will be nil, but if it's map[string]string, then the zero value is "", which is not nil!

There are other cases where it the value could be nil, like a map where the values are other non-primitive types that are pointers under the hood, like maps or slices (i.e. map[string]map[string]string).

I kind of think a lot of the existing behavior of this function is bogus. Ex. MatchIfEmpty returns true if the key is missing. A missing key isn't the same as the key is unset, after all, as you've noted. In my mind all of these cases should return an error. But for compatibility's sake I didn't want to have is nil work different.

Definitely right on the comment though. 👍

Copy link
Copy Markdown
Member Author

@tgross tgross Mar 5, 2026

Choose a reason for hiding this comment

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

Comment fixed in 8a4f137

default:
// Should never be reached as every operator should explicitly define its
// behavior.
Expand Down
4 changes: 4 additions & 0 deletions grammar/ast_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ func TestAST_Dump(t *testing.T) {
expr: &MatchExpression{Selector: Selector{Type: SelectorTypeBexpr, Path: []string{"foo", "bar"}}, Operator: MatchOperator(42), Value: nil},
expected: "UNKNOWN {\n Selector: foo.bar\n}\n",
},
"MatchIsNil": {
expr: &MatchExpression{Selector: Selector{Type: SelectorTypeBexpr, Path: []string{"foo", "bar"}}, Operator: MatchIsNil, Value: nil},
expected: "Is Nil {\n Selector: foo.bar\n}\n",
},
"UnaryOpNot": {
expr: &UnaryExpression{Operator: UnaryOpNot, Operand: &MatchExpression{Selector: Selector{Type: SelectorTypeBexpr, Path: []string{"foo", "bar"}}, Operator: MatchIsEmpty, Value: nil}},
expected: "Not {\n Is Empty {\n Selector: foo.bar\n }\n}\n",
Expand Down
Loading