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

BUILD- and WORKSPACE-loaded bzl files should have the same predeclared environment #11954

Closed
brandjon opened this issue Aug 15, 2020 · 7 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-Integration Issues involving Bazel's integration with Starlark, excluding builtin symbols type: feature request

Comments

@brandjon
Copy link
Member

A .bzl file should define the same set of top-level symbols regardless of whether it was loaded on behalf of a BUILD file or WORKSPACE file. Today, the set of symbol names is the same, but the native modules of the two bzl types are different.

We want to avoid spurious differences between types of bzls files, both for the sake of simplicity and to be friendlier to tooling. Clearly there are features that do not apply to both contexts -- native.glob() makes no sense during WORKSPACE evaluation, and native.register_toolchains makes no sense during BUILD evaluation. But we can address this by failing at evaluation time if called from the wrong context. We already do that, for instance, if native.glob() is called at the top level in a bzl file rather than inside a macro.

Arguably we can also have BUILD and WORKSPACE files themselves share the same environment. But if we do that, let's track it in a separate bug.

@alandonovan

@brandjon brandjon added type: bug P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Starlark labels Aug 15, 2020
@alandonovan
Copy link
Contributor

Agreed. One benefit is that the set of predeclared names is an input to the Starlark resolver (and thus compiler), so different names mean the .bzl file must be compiled differently.

bazel-io pushed a commit that referenced this issue Aug 30, 2020
This is to help protect the authors of exports.bzl from shooting themselves in the foot.

Main changes:
- Add validation of the injected builtins to ensure we don't inadvertently create new symbols, or override important symbols like `rule()`.
- Don't allow access to overridable symbols (e.g., `CcInfo`) from within @builtins bzls, since their meaning would be different than in regular bzl files.

Also:
- Spin off a new test suite for PackageFactory's management of the environment.
- Add a test that BUILD and WORKSPACE bzl files have minimal differences in their environments. They should eventually converge (#11954).
- Update comments/TODOs to reference #11954.
- Eliminate unneeded method PackageFactory#getNativeRules. It was originally intended for when injection logic was going to live inside StarlarkBuiltinsFunction.
- Add error handling and test for case where StarlarkBuiltinsFunction fails due to disallowed injections.

Work toward #11437 and #11954.

RELNOTES: None
PiperOrigin-RevId: 329207541
bazel-io pushed a commit that referenced this issue Oct 9, 2020
Previously, ConfiguredRuleClassProvider#getEnvironment returned an env that contained a binding for "native", mapping to a StarlarkNativeModule instance. This got overwritten in PackageFactory to instead map to a struct containing the same fields (e.g. glob()) plus all the registered rules (e.g. cc_library). Since ASTFileLookupFunction used the getEnvironment() method, it relied on the bogus StarlarkNativeModule object being there for static name validation to work.

As of unknown commit, ASTFileLookupFunction now queries PackageFactory for the final environment, so we are free to no longer add this incomplete dummy object. Except for BzlEnv, StarlarkNativeModule is now only used in conjunction with Starlark.addMethods(), not Starlark.addModule(), so the object itself is almost inaccessible. (Its class is still the home of the user documentation for the special native object fields.)

Verified that ApiExporter produces the same result after this change. (In fact, that target doesn't even depend on this CL.)

Cleanup along the way to #11437 and #11954.

RELNOTES: None
PiperOrigin-RevId: 336211639
@brandjon brandjon added team-Starlark-Integration Issues involving Bazel's integration with Starlark, excluding builtin symbols type: feature request and removed team-Starlark type: bug labels Feb 15, 2021
@brandjon
Copy link
Member Author

so different names mean the .bzl file must be compiled differently.

Note that the set of names is currently the same (and there's a test to ensure it remains the same).

Deprioritizing this issue because the benefit is mostly aesthetic, and will become irrelevant as we migrate from the current repository mechanism to bzlmod.

@brandjon brandjon added P4 This is either out of scope or we don't have bandwidth to review a PR. (No assignee) and removed P3 We're not considering working on this, but happy to review a PR. (No assignee) labels Feb 15, 2021
@Wyverald
Copy link
Member

Discussed with @brandjon yesterday:

  • we want binding names to be the same across all three bzl environments (BUILD-loaded, WORKSPACE-loaded, and Bzlmod-loaded)
    • this is so that the same .bzl file can be used by all three environments without erroring outright (this is not necessarily a desired feature, but somewhat nice since the file extensions are all .bzl)
  • we don't necessarily want the same binding name to point to the same object (enforcing this tends to cause an if at the top of the bound function anyway)
  • we don't want to enforce identical fields under the native object for the three bzl environments (so using native.cc_library in a WORKSPACE-loaded bzl file should simply result in a "field not found" error)
    • Right now native.cc_library is defined for a WORKSPACE-loaded bzl file, but errors as soon as it's invoked. This is not better than just having it be absent under native. Changing this is only visible for those who use dir(native), which is ridiculous in the first place.

@fmeum
Copy link
Collaborator

fmeum commented Jul 4, 2022

@brandjon Is it possible that repository_rule is a further difference that is not caught by

public void buildAndWorkspaceBzlEnvsDeclareSameNames() throws Exception {
?

@brandjon
Copy link
Member Author

brandjon commented May 1, 2023

We discussed this again more recently. We confirm that there's conceptually only one .bzl dialect, but that the native object is currently magical because its fields depend on the context. We'd like to simplify by merging all the native variants into a single object, whose fields may error out if called in the wrong context.

This is basically the same as what happens today if you try to glob in WORKSPACE context, or instantiate a repo rule in BUILD context. (Some of those may actually currently be crash bugs, in fact.) The only observable difference will be dir/hasattr behavior, which, aside from change detector tests, will only break users who are trying to determine whether they are in BUILD or WORKSPACE context. We think that usage is rare...

Is it possible that repository_rule is a further difference that is not caught by

No, I don't think so. I believe all non-native objects in the top-level environment are treated the same. Even native should be covered by that test.

Copy link

github-actions bot commented Jul 5, 2024

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 1+ years. It will be closed in the next 90 days unless any other activity occurs. If you think this issue is still relevant and should stay open, please post any comment here and the issue will no longer be marked as stale.

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

github-actions bot commented Oct 3, 2024

This issue has been automatically closed due to inactivity. If you're still interested in pursuing this, please post @bazelbuild/triage in a comment here and we'll take a look. Thanks!

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Oct 3, 2024
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-Integration Issues involving Bazel's integration with Starlark, excluding builtin symbols type: feature request
Projects
None yet
Development

No branches or pull requests

4 participants