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

Static name binding makes migrations difficult #7428

Closed
dbabkin opened this issue Feb 14, 2019 · 17 comments
Closed

Static name binding makes migrations difficult #7428

dbabkin opened this issue Feb 14, 2019 · 17 comments
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Starlark-Interpreter Issues involving the Starlark interpreter used by Bazel type: feature request

Comments

@dbabkin
Copy link
Contributor

dbabkin commented Feb 14, 2019

Please make Starlark resolve global accessible variable on runtime instead of on script loading phase.
This code fails if proto_common is not accessible as global variable. :

if False:
  proto_common.some_field

But in fact this line is ignored always during execution.

@dbabkin dbabkin changed the title Please stop Starlark make eager evaluation of variable. Please stop Starlark making eager evaluation of variable. Feb 14, 2019
@lberki lberki changed the title Please stop Starlark making eager evaluation of variable. Starlark evaluates top-level symbols eagerly Feb 14, 2019
@lberki
Copy link
Contributor

lberki commented Feb 14, 2019

/cc @lberki @hlopko

This causes a few issues, two of which we ran into over the course of the last ~three days:

  1. If package A has a visibility dependency on a package group in package B and package B loads a .bzl file, that .bzl file must not use unknown identifiers. This extra unexpected because such dependencies go in the "wrong" direction (if B should be able to depend on A, it sometimes happens that A depends on a package_group in B)

  2. This makes it impossible to test for the presence of new top-level identifiers, which complicates, and sometimes makes impossible, to write code that uses a new identifier while being tolerant to the lack of it (for example, in old Bazel versions)

This is exacerbated by the fact that official guidance is for native providers to be referenced using top-level symbols (e.g. ProtoInfo instead of proto_common.ProtoInfo)

Theoretical reasons to not do eager evaluation of symbols:

  1. That's not how Python works:
if False:
  unknown_symbol.unknown_method()

executes perfectly well.

  1. It's inconsistent: if I have a statement foo.bar(), typos are much better tolerated in bar than in foo. I can also check for the existence of the symbol bar, but not for the existence of the symbol foo.

This issue also surfaces with symbols in .bzl files: load("foo.bzl", "bar") always fails if bar doesn't exist in that file and there is not way to import a symbol but only when it exists. This results in authors of .bzl files (look at our Apple rules) resorting to the pattern where they export only a few top-level symbols and adding new symbols under it.

@laurentlb
Copy link
Contributor

It's not eager evaluation, it's static name resolution and it's by design (https://github.com/bazelbuild/starlark/blob/master/design.md#static-name-resolution).

  1. That's not how Python works:

Indeed. Starlark is not as dynamic as Python. This has multiple advantages, in terms of tooling (IDE support, refactoring...), performance, and detecting errors early.

  1. It's inconsistent: if I have a statement foo.bar(), typos are much better tolerated in bar than in foo.

No language detects all errors statically, you have to draw a boundary. Starlark uses dynamic typing, so we cannot possibly know the methods of an object in the general case.
(We could detect problems in the simplest cases; many people asked for some kind of static type checking.)

@lberki
Copy link
Contributor

lberki commented Feb 18, 2019

My bad. I keep not remembering the term for, apparently "static name resolution".

This has probably been discussed to death so I'll do my best not to re-litigate it, but is there a thread about the benefits of static name resolution to tooling? Naively, I'd think that pretending that Starlark has static name resolution if you are a tool would get you 99% of the way.

@laurentlb
Copy link
Contributor

I think the main argument is to detect errors early. We found lots of errors in the code when migrating out of Python. It's very difficult to get any kind of confidence when maintaining/updating/refactoring code, if you have no static checks.

@lberki
Copy link
Contributor

lberki commented Feb 19, 2019

Naively, I would expect that all code paths are exercised, preferably in tests, but at least in code you run before committing your change... at least that's the usual wisdom dispensed about more dynamic languages when people ask about validity checks done by the compiler in more rigid languages.

So is this a tradeoff between catching typos/refactoring issues early and ease of upgrading?

@laurentlb
Copy link
Contributor

catching typos/refactoring issues early

which is very important for maintenance, especially as a code base gets bigger.

and ease of upgrading?

What you were asking for is to have the same file for multiple incompatible versions of Bazel. That's quite different. I certainly don't recommend to have code paths that depend on the Bazel version.
You also asked for a way to detect if a global symbol exists or not.

So overall, you pretty much ask to change the language, just to handle your special-case - which is probably not going to happen.

@laurentlb
Copy link
Contributor

I've just had a look at the Wikipedia article.
https://en.wikipedia.org/wiki/Name_resolution_(programming_languages)#Static_versus_dynamic

It mentions a recommendation from the Python manual, which says:

the actual search for names is done dynamically, at run time — however, the language definition is evolving towards static name resolution, at “compile” time, so don’t rely on dynamic name resolution! (In fact, local variables are already determined statically.)

@hlopko
Copy link
Member

hlopko commented Feb 20, 2019

FTR, I don't think what Lukacs asks for is a special case. We see version checks in rules rust, rules_go, rules apple changed their design so they don't use top level symbols at all. All of this because of static name resolution. For these, I'll go as far as to say that the value of static name resolution is less than the cost of workarounds.

I think Starlark could/should improve here, so its users don't have to go to great lengths fighting the language. If you think bigger codebases and better tooling are the focus of the Starlark language, consider #7468. I'd still want to see some way of type-safe reflection like the good old mirrors or smth similar.

Or maybe all we need is for hasattr to have hasglobal and getglobal siblings.

@lberki
Copy link
Contributor

lberki commented Feb 20, 2019

I'm reopening this issue because the actual problem (inability to determine whether a particular top-level symbol exists for compatibility between different Bazel releases) persists. I can imagine a number of other things that would help:

  1. Discouraging people from creating top-level symbols in the first place (and then just suffering through eliminating the legacy ones). This already has some precedent in Starlark rules: in order to be able to check for presence of new symbols, the usual pattern is to expose one top-level symbol to load() then use hasattr on it.
  2. Active enforcement (with teeth!) that Bazel versions later than a particular one are not used (but even then, you want to make migration easier, not harder and that means giving people some flexibility)

We are currently at a bad place: official policy is to prefer top-level symbols, they are hard to migrate to (or from, if need be) and we don't have any way to deter people from using old versions of Bazel. (the latter issue will get worse once we'll start having LTS versions, but let's not go that far...)

I'm not asking for a language feature to solve my special case. I'm asking to reconcile the tension between (1) what Starlark does (static symbol resolution), (2) official policy (the encouragement of top-level symbols) and (3) reality (people don't upgrade Bazel all that often). It's just that the easiest solution I can think of is changing (1).

If ProtoInfo and PACKAGE_NAME is any indication, my plight is not the last instance of this problem.

I would be fine with the hasglobal / getglobal solution suggested by @hlopko above, although that looks like an invitation to transform statements like foo[ProtoInfo] to foo[getglobal("ProtoInfo")] and the proliferation of that particular idiom isn't good for anyone.

How does Python want to solve the "I want to figure out if a top-level symbol exists" problem in a world where they are resolved statically?

@lberki lberki reopened this Feb 20, 2019
@laurentlb laurentlb added type: feature request P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) labels Feb 20, 2019
@brandjon
Copy link
Member

The choice of static name resolution is now baked pretty deeply into the interpreter design. Going dynamic would require a spec change in the language, and presumably a performance impact since we'd have to resort to hashing for name lookups instead of indexing a symbol table.

The workaround of adding a get_global() builtin feels hackish and open to abuse. In the general case where it could access any global, it'd need knowledge of the symbol table information, so it'd have to live inside the interpreter and not just Bazel. A more limited form that could only retrieve from a registered set of possibly-present symbols would be better behaved, but still questionable.

In the case where a top-level symbol is deleted by an incompatible change, why not just migrate everyone before flipping the flag? Is the point to use the same .bzl to support even versions of Bazel that predate the introduction of the new API? That may cover this class of problems, but it's very difficult to generalize a way to write .bzl code that works across a wide range of interpreter / Bazel versions as both continue to evolve. (The traditional solution to that problem is to simply stop making breaking changes.)

@brandjon brandjon changed the title Starlark evaluates top-level symbols eagerly Static name binding makes migrations difficult Feb 17, 2021
@brandjon brandjon added team-Build-Language team-Starlark-Interpreter Issues involving the Starlark interpreter used by Bazel and removed team-Starlark labels Feb 17, 2021
@lberki
Copy link
Contributor

lberki commented Feb 18, 2021

re: "why not just migrate everyone": that works in theory, but it makes it impossible to provide a version of Starlark code that works with a wide range of Bazel versions. The straightforward way would be something like "if symbol is present, do X, else do Y", but the static name binding makes that impossible. So we are stuck with fragmenting the ecosystem.

It's certainly a problem, but how big a problem, I can't tell. We haven't ran into it recently.

I think Bazel is different from Starlark-without-Bazel because it has a much wider interface and therefore changes to it are more likely. This problem is also exacerbated by the fact that we are pretty liberal with adding new top-level symbols (e.g. *Info)

@hlopko
Copy link
Member

hlopko commented Feb 18, 2021

One approach we took in the past was to create separate bzl files, each with function implementations specific to a bazel version, and remote repository into which we symlinked one of these files depending on the actual Bazel version. Buildkite is pretty good at testing Starlark rules against multiple Bazel versions, but still maintaining these files gets hard very quickly as there are more incompatible flags. rules_foreign_cc were heavy users of this code generation pattern, and I honestly dreaded working on that project because of that.

Second approach that can be taken by the API owner (rules_swift do this, and rules_rust are migrating towards this too) is to provide a single struct with functions that encapsulate all the possible global symbols (https://github.com/bazelbuild/rules_swift/blob/master/swift/internal/swift_common.bzl#L57 or https://github.com/bazelbuild/rules_rust/blob/main/rust/private/common.bzl#L29). This way users can use hasattr.

It may make sense to take a step back and look how users are using the language and see if all assumptions that led us to designing Starlark still hold. Does it still make sense to have a dynamically typed language when all the potential productivity wins are trumped by juggling with repositories and delegating functions?

@lberki
Copy link
Contributor

lberki commented Feb 18, 2021

/cc @comius

@github-actions
Copy link

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 2+ years. It will be closed in the next 14 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

@github-actions github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Apr 26, 2023
@github-actions
Copy link

This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please reach out to the triage team (@bazelbuild/triage). Thanks!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 11, 2023
@fmeum
Copy link
Collaborator

fmeum commented May 11, 2023

@Wyverald I think this is resolved by bazel_features.

@Wyverald
Copy link
Member

I don't think this should be treated as "fully resolved" by bazel_features. It certainly unblocks a lot of usage, but specifically the global name detection mechanism in bazel_features feels like a workaround. I'd vote to keep this open, but leaving it up to @brandjon to decide. (See also bazelbuild/starlark#218 (comment))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) stale Issues or PRs that are stale (no activity for 30 days) team-Starlark-Interpreter Issues involving the Starlark interpreter used by Bazel type: feature request
Projects
None yet
Development

No branches or pull requests

8 participants