Bring sets in line with language spec and enable by default #584
Comments
|
I agree, and have been meaning to do this (though would welcome a CL). However, I'm worried that it might be a breaking change for some users, so I propose that we use the existing |
Match language spec: https://github.com/bazelbuild/starlark/blob/master/spec.md#set%C2%B7add Working towards google#584
Match language spec: https://github.com/bazelbuild/starlark/blob/master/spec.md#set%C2%B7add Working towards google#584
|
I'd suggest flag-guarding the differences in operator behavior (comparison, |
|
I agree the frozen thing is an edge case we can ignore. After further reflection, it's not clear to me how to implement flag guarding for the relevant operators since the flag is conceptually a compile-time (and thus per-file) setting, not a per data structure setting nor a per thread setting. Operators don't have access to the files from which they came (nor the thread that calls them, for that matter); also, binary operators and functions have to make a choice about what to do in the (admittedly unlikely) event that exactly one but not both of their operands is covered by the flag semantics. (The interesting case of I'm not sure we can do much more than emit a dynamic warning message: the first time a deprecated operation is called we log a message. |
…ted (#585) Match language spec: https://github.com/bazelbuild/starlark/blob/master/spec.md#set%C2%B7add Working towards #584
|
In Bazel this is gated with If you really do need to make it per-file or per-data structure, what about creating an internal second variant of the set datatype and having the flag gate which version you get via set-producing operations (in particular, the |
The Go implementation of BinaryOp (for example) doesn't accept a StarlarkSemantics, but if it had done so, then it would be a reasonable way to implement the desired behavior: of course, the behavior of the operator would be determined by the Semantics of the thread executing the operation, if any, or by an explicit semantics if called outside any thread. (Grumbly aside: at one point I succeeded in making the Java implementation of binaryOp not depend on StarlarkThread, but unfortunately it seems to have since backslid so that operations can no longer be evaluated outside the context of a thread.)
The Go implementation used global variables in the past but--as always happens eventually--we regretted it and switched to explicit passing of a FileOptions parameter. (The legacy code continues to work by immediately creating a FileOptions from a snapshot of the global variables' current state.)
Unfortunately the Set data type is part of the public API, so it would be disruptive to create two different flavors Set, (the legacy one) and NewSet. One could represent the two flavors as a boolean inside Set. The question is: how does it get its value? When the set is constructed by a |
|
Most Starlark language changes in google3 have not required interoperability between the old and new semantics within the same invocation. The changes in question here seem minor enough that we should be able to migrate offending code within google3, and flip a flag that applies globally to the interpreter. I would expect backsliding to be unlikely and addressable with forward-fixes. But this might not be possible via hacking in a global variable if, for instance, there are multiple usages of the interpreter embedded into the same binary that correspond to separate states of the code base. I don't know whether that's the case for any users either inside or outside of Google.
Looking back at the code history, this seems to be support a toolchain-related API being able to resolve repository labels correctly -- the label is given as the key in the index expression, and the repo mapping information is obtained from the application data stored in the thread. I guess the alternative would've been to bake a reference to this information into the toolchain-api object at its construction. There's a number of application-defined operations in Bazel that require a thread for evaluation, label resolution being chief among them. I think the difference here is whether those operations are just function calls, or extend to overloaded operators too. I don't know that we lose much by, say, not being able to guarantee that evaluation of |
Multiple independent uses of the Go-based interpreter within a single executable are indeed likely to be rare. Given the API constraints (which don't plumb a "semantics" everywhere) I don't think we're going to be able to provide completely smooth transitions. A global variable may be the best we can do. |
|
The sets feature was always considered experimental for Starlark-Go, no? Wouldn't the global var only live in the codebase temporarily? |
It was non-standard, and behind a flag, but Starlark/Go, like well behaved Go modules in general, has no such concept of experimental features: if an API is published, removing it is a breaking change, and we don't make breaking changes without changing the major version number of the module, which is the same as renaming the module. |
…forms Also update the docs and tests. Working towards google#584
Uh oh!
There was an error while loading. Please reload this page.
Now that sets are part of the Starlark language spec (see bazelbuild/starlark#290), we should bring starlark-go's implementation in line with the spec and enable them by default.
Specifically:
add()anddiscard()|,&,-, and^operators (and their augmented forms) to be sets if lhs is a setunion(),intersection(),difference(),update()isdisjoint(),intersection_update(),difference_update(),symmetric_difference_update()@aranguyen @brandjon FYI
The text was updated successfully, but these errors were encountered: