starlark icon indicating copy to clipboard operation
starlark copied to clipboard

Clarify behavior of load statement

Open stepancheg opened this issue 4 years ago • 2 comments
trafficstars

  • loaded variables cannot be bound twice
  • loaded variables cannot be reassigned
  • loaded variables are not exported

These are the rules from the Java Starlark implementation.

Note "loaded variables cannot be reassigned" rule is somewhat inconsistent: loaded variables behave the same as builtins: they are available to the current module but not exported.

But builtins can be reassigned.

This might need to be changed for greater consistency, but also for practical reasons: reexports.

Suppose there's a module which imports a function foo and exports it. Currently it can be written like this:

load("module.star", _foo="foo")
foo = _foo # reexport

It is desirable to be able to write this shorter, as:

load("module.star", "foo")
foo = foo  # reexport

This resolves issue #37.

stepancheg avatar Mar 02 '21 00:03 stepancheg

When you have load(..., "foo"), the symbol foo will be defined in the file. When you have the global definition foo = ..., the symbol foo will also be defined in the file.

If you allow both, how do you resolve foo?

load(..., "foo")

def f(): print(foo)

foo = 1

The motivation for the change is to allow foo = foo, but this line can be confusing for a reader. It can look like a no op or a recursive definition.

If you write range = range in the code, you'll get a dynamic error:

global variable range referenced before assignment

This happens because the range builtin is shadowed (and not visible in the file). The right-hand side of the assignment is a reference to the range global variable, which hasn't any value yet.

laurentlb avatar Mar 02 '21 08:03 laurentlb

Note "loaded variables cannot be reassigned" rule is somewhat inconsistent [...] This might need to be changed for greater consistency

It is inconsistent, but it's also a deliberate choice.

Assigning to a global foo while also binding it in a load is clearly a contradiction by the author of the .bzl file, and can therefore be resolved by that author. If we did allow it, and if we tried to be strictly consistent with our shadowing rules (and the rule that load-bound variables are in a child block relative to globally bound variables), we end up with the following counterintuitive example:

load("...", "foo")
foo = "new_value"
print(foo)  # prints the original loaded value of foo, not the new value

This is because, just as in Python, the relative location of a use of a name within a block does not change which variable the name refers to. So the loaded foo shadows the global one throughout the file.

The decision to prohibit this kind of name reuse / shadowing does make re-exports uglier. But foo = foo is not a cleaner alternative. Not only is it confusing to read, but it also violates the aforementioned pattern about all uses of a name in a block referring to the same variable. Even in languages that don't follow Python's scoping rules, you still generally need to qualify one of those foos, e.g. Java's this.foo = foo initialization statements.

Deferring this PR until I can revisit the discussion context and chat with Alan. (The reason this isn't a trivial 'accept' is that we don't necessarily want to codify every Java interpreter behavior into the spec, particularly since even the Java interpreter behaves differently in different contexts, like BUILD vs .bzl files.)

brandjon avatar Mar 02 '21 18:03 brandjon