support "is nil" and "is not nil" and make "is empty" safer by tgross · Pull Request #129 · hashicorp/go-bexpr

tgross · 2026-03-05T16:37:04Z

While working on an issue to reduce load the Nomad CLI can place on the server, I discovered that go-bexpr does not handle pointers in structs usefully or even safely.

Without an option for "is nil", users are likely to try "is empty" on a pointer object. But an expression like /TopValue/MaybeNilValue is empty panics because the handler for empty only works for collections. Fortunately in Nomad we don't really trust go-bexpr not to panic and have recover handling, so this returns an error rather than crashing the control plane.

Add "is nil" and "is not nil" to the grammar. Make "is empty" handle non-collections safely and do the intuitive thing when given a pointer to a struct.

Ref: https://hashicorp.atlassian.net/browse/NMD-941
Ref: hashicorp/nomad#26653
Ref: hashicorp/nomad#27631

Testing

$ go test -count=1 ./...
ok      github.com/hashicorp/go-bexpr   0.084s
?       github.com/hashicorp/go-bexpr/examples/expr-eval        [no test files]
?       github.com/hashicorp/go-bexpr/examples/expr-parse       [no test files]
?       github.com/hashicorp/go-bexpr/examples/filter   [no test files]
?       github.com/hashicorp/go-bexpr/examples/simple   [no test files]
ok      github.com/hashicorp/go-bexpr/grammar   0.032s

(apparently there's no CI run on PRs in this repo? 😬 )

PCI review checklist

I have documented a clear reason for, and description of, the change I am making.
If applicable, I've documented a plan to revert these changes if they require more than reverting the pull request.
If applicable, I've documented the impact of any changes to security controls.

Examples of changes to security controls include using new access control methods, adding or removing logging pipelines, etc.

While working on an issue to reduce load the Nomad CLI can place on the server, I discovered that go-bexpr does not handle pointers in structs usefully or even safely. Without an option for "is nil", users are likely to try "is empty" on a pointer object. But an expression like "/TopValue/MaybeNilValue is empty" panics because the handler for empty only works for collections. Fortunately in Nomad we don't really trust go-bexpr not to panic and have recover handling, so this returns an error rather than crashing the control plane. Add "is nil" and "is not nil" to the grammar. Make "is empty" handle non-collections safely and do the intuitive thing when given a pointer to a struct. Ref: https://hashicorp.atlassian.net/browse/NMD-941 Ref: hashicorp/nomad#26653 Ref: hashicorp/nomad#27631

tgross · 2026-03-05T16:47:13Z

+	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


Note to reviewers: arguably is empty should return an error for pointers/structs, rather than be handled as we've done it here. If we remove these cases, the user will get an error but we won't be panicking anymore. I don't have strong feelings either way on this other than end-user convenience, at the trade-off of "impurity" in the API

My feelings are maybe a little stronger. To make your "arguably" argument:

I interpret this thing's (panicky) .Len() == 0 as intent to work on the same types that len() would (as you handle in the first case), and Go won't even compile if you try to len() the wrong type. So I think that deserves an error rather than treating a struct as "having length" and a nil pointer to a concrete type as "not having length".

The Invalid case seems even more like an error, because in Go that would be a runtime nil pointer panic, if I understand that right?

We can loosen constraints more easily than we can tighten them, so we could un-error this later if needed. Although, going from panic->error feels safer than going from error->not-error... I'm a little waffly on this point.

Ok, I was feeling like I would be pedantic to say "well technically this isn't empty" but you've sold me on all of this. 😁

The Invalid case seems even more like an error, because in Go that would be a runtime nil pointer panic, if I understand that right?

Right! That's actually the case where I hit the panic in Nomad.

Fixed in 8a4f137

tgross · 2026-03-05T16:59:09Z

 		},
 		"default max expressions": {
 			expression: "((((((((foo == 1))))))))",
-			// typo in pigeon code-gen


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.

gulducat

Nice addition, seems useful!

I have a ramble in the missing-key case, and I vote for IsEmpty to error on things that can not have length, as it seems more in-line with the intent of the method, and now there's an alternative in IsNil.

gulducat · 2026-03-05T18:40:39Z

+	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


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?

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. 👍

Comment fixed in 8a4f137

gulducat · 2026-03-05T18:47:03Z

-			pos:         position{line: 113, col: 1, offset: 3209},
+			pos:         position{line: 113, col: 1, offset: 3238},


Note for other reviewers: it took me a moment to orient myself and recognize that grammar.go is a generated file, so all these little changes are not relevant for review.

gulducat · 2026-03-05T19:28:24Z

+	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


My feelings are maybe a little stronger. To make your "arguably" argument:

I interpret this thing's (panicky) .Len() == 0 as intent to work on the same types that len() would (as you handle in the first case), and Go won't even compile if you try to len() the wrong type. So I think that deserves an error rather than treating a struct as "having length" and a nil pointer to a concrete type as "not having length".

The Invalid case seems even more like an error, because in Go that would be a runtime nil pointer panic, if I understand that right?

We can loosen constraints more easily than we can tighten them, so we could un-error this later if needed. Although, going from panic->error feels safer than going from error->not-error... I'm a little waffly on this point.

gulducat

Assuming tests are happy, I'm happy!

tgross · 2026-03-05T20:38:15Z

Assuming tests are happy, I'm happy!

Yup! From the current head of this PR:

$ go test -count=1 ./...
ok      github.com/hashicorp/go-bexpr   0.100s
?       github.com/hashicorp/go-bexpr/examples/expr-eval        [no test files]
?       github.com/hashicorp/go-bexpr/examples/expr-parse       [no test files]
?       github.com/hashicorp/go-bexpr/examples/filter   [no test files]
?       github.com/hashicorp/go-bexpr/examples/simple   [no test files]
ok      github.com/hashicorp/go-bexpr/grammar   0.024s

I'll see if I can get CI fixed as well.

tgross requested review from a team as code owners March 5, 2026 16:37

tgross mentioned this pull request Mar 5, 2026

batch: reduce server overhead when dispatching jobs from CLI hashicorp/nomad#27631

Merged

7 tasks

tgross requested review from allisonlarson, gulducat and pkazmierczak March 5, 2026 16:38

tgross commented Mar 5, 2026

View reviewed changes

gulducat reviewed Mar 5, 2026

View reviewed changes

address comments on code review

8a4f137

gulducat approved these changes Mar 5, 2026

View reviewed changes

tgross merged commit 2fcb6f9 into main Mar 5, 2026
4 checks passed

tgross deleted the handle-nil-pointer-to-struct branch March 5, 2026 20:38

tgross mentioned this pull request Mar 5, 2026

changelog for 0.1.16 #130

Merged

3 tasks

hc-github-team-nomad-core mentioned this pull request Mar 6, 2026

Backport of batch: reduce server overhead when dispatching jobs from CLI into release/1.11.x hashicorp/nomad#27634

Merged

7 tasks

tgross mentioned this pull request Mar 11, 2026

Nomad: document nil checks in filter expressions hashicorp/web-unified-docs#1986

Merged

15 tasks

campaigner-prod Bot mentioned this pull request Apr 23, 2026

fix(deps): vuln minor upgrades — 15 packages (minor: 7 · patch: 8) DataDog/vault#87

Closed

4 tasks

		pos: position{line: 113, col: 1, offset: 3209},
		pos: position{line: 113, col: 1, offset: 3238},

Conversation

tgross commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

PCI review checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gulducat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gulducat left a comment

Choose a reason for hiding this comment

Uh oh!

tgross commented Mar 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tgross commented Mar 5, 2026 •

edited

Loading