Skip to content

check struct field name is a string in Fetch and get - #973

Open
netliomax25-code wants to merge 1 commit into
expr-lang:masterfrom
netliomax25-code:struct-key-string-check
Open

netliomax25-code wants to merge 1 commit into
expr-lang:masterfrom
netliomax25-code:struct-key-string-check

Conversation

@netliomax25-code

Copy link
Copy Markdown

runtime.Fetch (runtime.go:76) and the get builtin (lib.go:598) read a struct field name with an unchecked i.(string), so indexing a struct held in an any value with a non-string key panics with a raw interface conversion. Use a checked assertion so Fetch falls through to its usual "cannot fetch" error and get returns nil per its no-panic contract.

@netliomax25-code

Copy link
Copy Markdown
Author

any update?

@sanmaxdev sanmaxdev left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The struct-key guard is consistent across get and runtime field access, and both paths have regression coverage.

Passed:

  • go test ./...
  • git diff origin/master...HEAD --check

@netliomax25-code

Copy link
Copy Markdown
Author

Lines up with what I see locally. Reran the full suite on the current head and it's green, including test/gen and test/fuzz, which is the part I wanted to confirm: the struct branch only breaks out when the key isn't a string, so Fetch falls through to its existing cannot fetch %v from %T error and get returns nil per its no-panic contract. No new error strings, so the fuzz skip list doesn't need a change and generated expressions aren't affected.

The branch is two commits behind master but still merges cleanly. I can rebase it if a maintainer wants it current before merging.

@antonmedv

Copy link
Copy Markdown
Member

Will take a look. I need to run local benches.

@netliomax25-code

Copy link
Copy Markdown
Author

Rebased onto master first so the numbers are against current code: #952 rewrote the struct branch of Fetch after I opened this. The unchecked assertion is still there on master (vm/runtime/runtime.go:76) and the raw text still surfaces:

env := map[string]any{"s": any(struct{ A int }{A: 1}), "i": 2}
expr.Eval(`s[i]`, env)
// master: interface conversion: interface {} is int, not string (1:2)
// patch:  cannot fetch 2 from struct { A int } (1:2)

On the benches, my first pass was useless at ±35% run to run, so I built master and patch test binaries up front and interleaved them A/B, 14 rounds each (Apple M4, go1.25.5):

                    │   master    │    patch    │  vs base
_expr-10              44.43n ± 3%   44.55n ± 4%    ~ (p=0.743 n=14)
_envStruct-10         31.37n ± 4%   31.48n ± 3%    ~ (p=0.323 n=14)
_envStruct_noEnv-10   93.01n ± 2%   93.08n ± 2%    ~ (p=0.734 n=14)
_envMap-10            35.44n ± 4%   35.20n ± 3%    ~ (p=0.269 n=14)
_callField-10         41.21n ± 4%   41.29n ± 5%    ~ (p=0.594 n=14)
geomean               45.23n        45.25n        +0.04%

B/op and allocs/op are byte-identical on all five. A micro-bench calling runtime.Fetch straight on the struct path gives 25.96n vs 25.77n (p=0.874).

Benchmark noise isn't great evidence either way, so the deterministic version: go tool objdump of Fetch is 416 instructions on both sides, and the only difference is one runtime.panicdottypeE call site instead of two. The comma-ok form compiles to the same type-word compare, it just replaces the panic edge with a branch. The typed OpFetchField path isn't involved.

Two other notes:

  1. I dropped an unrelated gofmt hunk in builtin_test.go that had crept into the diff, so it's now just the two guards plus the two regression tests.
  2. go test ./..., -race, go vet, and test/gen plus test/fuzz all pass on the rebased branch. No new error strings, so the fuzz skip list is unchanged.

Worth mentioning in case it saves you a step: no checks have ever reported on this branch, so the diff workflow's benchstat never ran here.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants