Allow load statements in conditionals and loops

[edigaryev]

The commit https://github.com/google/starlark-go/commit/cd131d1ce9d424fe23fac85e99953ad099021287 disables the load statements in conditionals and loops, saying that:

This is a potentially breaking language change, but I suspect few users care. If you are one of them, please report the issue.

Looking at the official spec, namely, the Load statements and Grammar reference sections, nothing prevents the load statement to be in a conditional or a loop.

We need this feature at Cirrus Labs for our Starlark tasks, and it seems that fellows at Tilt need it too for their core product's Tiltfile.

Since this is the current behavior for almost 4 years now, perhaps a good compromise would be giving the users of this package an option to re-enable this behavior using the syntax.FileOptions.

[laurentlb]

Regarding the spec:

A load statement within a function is a static error.

a for loop is permitted only within a function definition. A for loop at top level results in a static error.

An if statement is permitted only within a function definition. An if statement at top level results in a static error.

Why can't you move the load statement to the top level? Do you need conditional loads?

[edigaryev]

A load statement within a function is a static error.

Why can't you move the load statement to the top level?

We're already at the top level.

a for loop is permitted only within a function definition. A for loop at top level results in a static error. An if statement is permitted only within a function definition. An if statement at top level results in a static error.

This seems to be allowed in this project by the TopLevelControl in syntax.FileOptions, though.

Do you need conditional loads?

Exactly.

Our use-case is gracefully falling back to using a stub identifier when loading of the module that contains a non-stub identifier is not possible (e.g. due to lack of permissions, or just the lack of module itself).

Here's an example that is demonstrates this for module existence:

# .cirrus.star

load("cirrus", "fs")

if fs.exists("private.star"):
    load("private.star", "greeting")
else:
    def greeting():
        return "Goodbye, cruel World!"

def main(ctx):
    print(greeting())

    return []

# private.star

def greeting():
    return "Hello, World!"

Frankly, having the load statements in functions would be the most cleanest solution, but this is currently not allowed by the spec.

Another way we've tried to work around this is to return a StringDict with the loadable identifier set to nil, however, this requires us to know the identifiers that will be loaded in advance, and this information is not available in the Load callback.

[adonovan]

Looking at the official spec, namely, the Load statements and Grammar reference sections, nothing prevents the load statement to be in a conditional or a loop.

As @laurentlb points out, this is a misreading of the spec; the intended behavior is clear from the portions he quoted, and the rationale is clearly expressed in #275.

We need this feature at Cirrus Labs for our Starlark tasks, and it seems that fellows at Tilt need it too for their https://github.com/tilt-dev/tilt/issues/3524.

Reading the attached Tilt issue, it would appear that they found an alternative approach some years ago and don't actually need this feature. Have you explored the approach they took?

Since this is the current behavior for almost 4 years now, perhaps a good compromise would be giving the users of this package an option to re-enable this behavior using the syntax.FileOptions.

We try hard to avoid adding new options that change the language semantics. Most that exist were necessary to enable compatibility between different implementations and gradual harmonization; many that used to exist (e.g. float, nesteddef, bitwise, lambda) have become obsolete due to spec standardization. Some exist to support the REPL. Ideally we would never add more.

Since this is a proposal to change the core semantics of the language, it should be proposed at bazelbuild/starlark.

[discentem]

@edigaryev Did you ever file this as an issue to bazelbuild/starlark? I looked for any recent issue about loading but couldn't find one. I'm happy to file one if not. Restoring load statements inside conditionals and loops would be very useful for a tool I'm building.

Also in the meantime: are you maintaining a fork that re-enables load statements in conditions/loops?

[discentem]

@edigaryev Did you ever file this as an issue to bazelbuild/starlark? I looked for any recent issue about loading but couldn't find one. I'm happy to file one if not. Restoring load statements inside conditionals and loops would be very useful for a tool I'm building.

Also in the meantime: are you maintaining a fork that re-enables load statements in conditions/loops?

Actually it seems pretty trivial for consumers to implement dynamic loading themselves if they need it. Here's the implementation I came up with: https://gist.github.com/discentem/b6afae321df6821326f0675630b9eb6b

Not sure if it's possible to dynamically load symbols, as I don't need it.