search

google / starlark-go

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

struct: Add SetField method for mutable fields #400

Closed emcfarlane closed 2 years ago

emcfarlane commented 2 years ago

Modifies the struct definition to implement starlark.SetField. It changes the storage of fields from an []entries to a starlark.StringDict, I've added a benchmark on Attr. On setting non-existing fields the new method errors with NoSuchAttrError.

This makes the struct mutable, fields can be edited after creation but no new fields can be added. A frozen struct behaves similar to before. Couldn't see if this violates the starlark spec? If so please feel free to close.

adonovan commented 2 years ago

A mutable struct is a reasonable idea, and perhaps there should be one in the Starlark library---though there are a surprising number of design questions to consider: flat or hash-based representation? Restrictions on allowed field names? Restrictions on allowed values or types of fields? And so on.

However, this struct has been defined as an immutable data type, and we can't change that now.

emcfarlane commented 2 years ago

I separated out some of the changes for field storage here: https://github.com/google/starlark-go/pull/401

It does feel like struct should have reassignable fields on creation and only when frozen become immutable. Understand this is a behavior change though, any way to support both?

adonovan commented 2 years ago

I separated out some of the changes for field storage here: #401

Thanks; that change looks basically sound.

It does feel like struct should have reassignable fields on creation and only when frozen become immutable. Understand this is a behavior change though, any way to support both?

You can always add a second data type to your application that supports both. Bazel relies on the existing struct contract and doesn't have a mutable struct (or namedtuple, as Python calls it), but there's no reason you can't create one.