syntax: parameterize API over FileOptions, avoid globals #477
Conversation
| return &FileOptions{ | ||
| Set: resolverAllowSet, | ||
| While: resolverAllowGlobalReassign, | ||
| TopLevelControl: resolverAllowGlobalReassign, |
There was a problem hiding this comment.
Why are While and TopLevelControl determined by resolverAllowGlobalReassign? (I vaguely remember there was a reason for this but not what it was.)
There was a problem hiding this comment.
I decided to split the flag into finer features in this CL. The -recursion flag is required to use while statements anywhere because potentially unbounded loops are expressively equivalent to recursive calls with arbitrary arguments.
As for -globalreassign, the flag is required only to use 'while' (or if or for) at top-level. I think the reason for this was so that Bazel could enforce the invariant that every top-level variable had exactly one assignment, also at top-level. Allowing re-declaration of globals (e.g. x=1; x=2) obviously breaks this invariant. More subtly, so does a loop for x in seq: pass: although there is only one static assignment, it is executed dynamically many times, including zero. And so too does if cond: x = 1, because it can cause a variable to be initialized zero times. (An if statement is an at-most-once loop!) Obviously while is like for. Personally I don't think the concern is important, and this flag is too hard to explain. @laurentlb may remember more.
|
Thanks, Jay. |
This change eliminates the need for client applications to set global variables to control dialect options, as globals are error-prone and concurrency hostile. All relevant API functions Foo now have a variant FooOptions that takes an explicit FileOptions; the original function accesses the legacy options by reading the global variables. Fixes #435
No worries! Between furious gopls work and now a lengthy vacation I haven't been spending much time thinking about Starlark. (Or the other matter, for that matter.) Thanks for reviews, as always. |
This change eliminates the need for client applications to set global variables to control dialect options, as globals are error-prone and concurrency hostile.
All relevant API functions Foo now have a variant FooOptions that takes an explicit FileOptions; the original function accesses the legacy options by reading the global variables.
Fixes #435