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

spec: clarify behavior of load statements within toplevel if/for blocks #275

Closed
alandonovan opened this issue Jun 5, 2020 · 4 comments · Fixed by #276
Closed

spec: clarify behavior of load statements within toplevel if/for blocks #275

alandonovan opened this issue Jun 5, 2020 · 4 comments · Fixed by #276

Comments

@alandonovan
Copy link
Contributor

alandonovan commented Jun 5, 2020

The spec is unclear about the behavior of load statements outside a function but inside a top-level if/for block. (The Java implementation does not permit top-level if/for blocks at all, so it doesn't provide any precedent.)

Currently, it is not an error:

$ starlark -globalreassign
Welcome to Starlark (go.starlark.net)
>>> for x in ():
...   load("x", "f")
... 
>>> if False:
...   load("x", "f")
... 

Disallowing it would make load statements behave more like imports in a static language (Go/C++/Java): as declarations of the program structure. Allowing them would allow for delayed or conditional loading, as in Python. However, it would complicate applications (like Bazel) that preprocess all the load statements as they would have to search within if/for blocks to find them.

My inclination is to disallow it, and I'm sure @laurentlb would agree, but I'm a little worried I have inadvertently allowed Go clients to start depending on this feature. Oops.

@alandonovan alandonovan changed the title reject load statements within if/for blocks reject load statements within toplevel if/for blocks Jun 5, 2020
@alandonovan alandonovan changed the title reject load statements within toplevel if/for blocks spec: clarify behavior of load statements within toplevel if/for blocks Jun 5, 2020
@fenollp
Copy link
Contributor

fenollp commented Jun 6, 2020

How about a package flag that defaults to preserve current behavior for go clients?

I’d like my Go client to set the flag to false so would love to have this upstream.

Thanks for catching this

@alandonovan
Copy link
Contributor Author

Pierre, do you mean you would set the flag to preserve the current behavior, or to enable the stricter check? If the latter, could you explain what you use this feature for?

@fenollp
Copy link
Contributor

fenollp commented Jun 8, 2020

Ah I can see how I was unclear, sorry about that.
I'm voting to disallow it and I'd like a way to make sure such loads are forbidden.

@alandonovan
Copy link
Contributor Author

Glad to hear it. I'm tempted to make the change and see if anyone complains.

alandonovan pushed a commit that referenced this issue Jun 9, 2020
This is a potentially breaking language change, but I suspect
few users care. If you are one of them, please report the issue.

Fixes #275

Change-Id: I53da7c6224bafe95753df996b141ac5349915c7e
alandonovan pushed a commit that referenced this issue Jun 9, 2020
This is a potentially breaking language change, but I suspect
few users care. If you are one of them, please report the issue.

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

Successfully merging a pull request may close this issue.

2 participants