Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify behavior of load statement #180

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

stepancheg
Copy link
Contributor

  • 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.

* 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 bazelbuild#37.
@stepancheg stepancheg requested a review from laurentlb as a code owner March 2, 2021 00:46
@google-cla google-cla bot added the cla: yes label Mar 2, 2021
@laurentlb
Copy link
Contributor

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 laurentlb requested a review from brandjon March 2, 2021 08:29
@brandjon
Copy link
Member

brandjon commented Mar 2, 2021

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.)

@@ -2903,6 +2903,40 @@ load("module.sky", "x", "y", "z") # assigns x, y, and z
load("module.sky", "x", y2="y", "z") # assigns x, y2, and z
```

Varaibles bound by `load` statement are local to the current module:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Variables

Loaded variables cannot be rebound, similar to global variables:
* it is a static error to bind a variable more than once with load statements
* it a static or runtime error to bind a global variable if a variable
with the same name name previously loaded
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"name name" => "name was"


Loaded variables cannot be rebound, similar to global variables:
* it is a static error to bind a variable more than once with load statements
* it a static or runtime error to bind a global variable if a variable
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we drop "or runtime"?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants