Mutable package-level vars are a poor choice for configuration

[ash2k]

Hello there! I have a project where I get this library as a transitive dependency. It's used for "stuff X". I also want to use this library for "stuff Y", unrelated to "stuff X", in the same codebase/binary. I'd like to configure the library and modules differently for different use cases. But in a few places the library uses package-level variables for "global" configuration and I cannot do what I want.

Examples:

• starlark-go/lib/time/time.go

  Line 74 in acb66ad

  var NowFunc = time.Now

• starlark-go/starlark/library.go

  Line 34 in acb66ad

  var Universe StringDict

• starlark-go/resolve/resolve.go

  Lines 100 to 111 in acb66ad

  	var (
  	AllowSet = false // allow the 'set' built-in
  	AllowGlobalReassign = false // allow reassignment to top-level names; also, allow if/for/while at top-level
  	AllowRecursion = false // allow while statements and recursive functions
  	LoadBindsGlobally = false // load creates global not file-local bindings (deprecated)
  	// obsolete flags for features that are now standard. No effect.
  	AllowNestedDef = true
  	AllowLambda = true
  	AllowFloat = true
  	AllowBitwise = true
  	)

• Maybe a few more places I didn't find now.

In addition to the above, global mutable variables are sometimes abused by third-party libraries. They assume they are the center of the world and mutate such variables in other packages in their init(). No mutable globals -> no such problems.

p.s. thank you for the library! It's great!

[adonovan]

Yes, global state is bad, and the decision to use it here assumes there's only one Starlark client app in the address space, which isn't necessarily true. The safer choice would be to have required extensible configuration structs as parameters to Parse, Resolve, Execute, etc, which is a nuisance but creates more leeway.

I suppose we could add configurable API variants to the existing code, and make the existing API read the globals and put them in the config struct before calling the new API.

NowFunc could be easily fixed by defining a Thread-local variable called "starlark.time.now" and making the time package use that if it exists. Then any client application can override the clock on a per-thread basis.

Universe never needs to be modified, since you can add to the Predeclared set in a hygienic way.

But the Allow* flags are unfortunate, I agree.

[ash2k]

An alternative approach, which is probably a breaking change (or maybe this could be done as a "parallel" API) and I'm not saying it's better, is to introduce an "interpreter" object that is instantiated with all the language-affecting configuration provided (e.g. those Allow* flags). This object provides methods to spawn threads and do other things that need to (now or potentially in the future) use those global config flags (current or any future flags). This object (or something like it) can be injected into callbacks where this library calls use code. Just an idea.

[ash2k]

For modules and for the "interpreter" object I'd suggest to have constructors with functional options, like what gRPC-go/etc does.

[adonovan]

It would be reasonable to construct a parallel API that used pure functions (not globals) for configuration and accepted a FileOptions struct whose zero value embodied the defaults. Each syntax.File would retain its options so that you wouldn't need to repeat it for later stages.

I've sketched it in #477. Please download the patch and try it out and let me know if it works for you.

[ash2k]

Looks good to me! I'll try it next week or so. I think it is what's needed. Thank you!

[ash2k]

I've tried the branch, it works well for my needs. The unaddressed bit is the module (or modules?) that has a global var as config. Thanks!