search

google / starlark-go

Starlark in Go: the Starlark configuration language, implemented in Go
BSD 3-Clause "New" or "Revised" License
2.3k stars 209 forks source link

starlarkstruct does not handle nil values #348

Closed milas closed 3 years ago

milas commented 3 years ago

Expected Behavior

When creating a starlarkstruct.Struct from Go code using FromKeywords or FromStringDict, if a value is nil, it should be treated as None.

Example:

s := starlarkstruct.FromKeywords(starlarkstruct.Default, []starlark.Tuple{ { "key", nil } })

I'd expect that I could use this as a normal struct, and the key field would have a value of None.

Actual Behavior

This will panic on Freeze() (due to trying to recursively freeze entries) and if you try to access key, you'll get an error:

Error: struct has no .key field or method (did you mean .key?)

Other notes

This came up along with #347 because we're creating some starlarkstructs with (possibly None) embedded starlarkstructs.

I think this could be a pretty straightforward change to ensure that entry is always constructed with the provided value if non-nil, otherwise, starlark.None. I'm happy to open a PR along with that change + expanded test case if you agree with that approach.

alandonovan commented 3 years ago

https://pkg.go.dev/go.starlark.net/starlark says:

Starlark's None value is not equal to Go's nil. Go's nil is not a legal Starlark value,
but the compiler will not stop you from converting nil to Value.
Be careful to avoid allowing Go nil values to leak into Starlark data structures.

Constructing a Tuple containing nil values is a mistake. It should not be the job of FromKeywords to spot it. Even if it did split "shallow" nils, it could not spot nils that lie deeper in the data structure.

Either omit the name and value from the tuple, or pass None explicitly.

milas commented 3 years ago

Unfortunate, but understandable! Thanks for the quick response 👍