From 3c71f95ddc6da5a23ca3ce8ff02124dc177e51d5 Mon Sep 17 00:00:00 2001 From: Googler Date: Thu, 6 Oct 2022 12:06:11 -0700 Subject: [PATCH] Disallow calling visibility() via a helper function This CL makes visibility() fail if it is called with a stack frame containing anything other than the entries for top-level module evaluation and for visibility() itself. The motivation is to prohibit fancy framework-style .bzl logic that would frustrate automated refactorings. If static tooling can always locate where the `visibility()` call in a file is, it can always mechanically modify it to `visibility([some_dep] + )`. This is handy for developer workflows that rely on tooling to help locate and update visibilities that need to be broadened, even if the mutated declaration is still not review-ready. It is still possible to alias `visibility` to another symbol, e.g. `f = visibility; f(...)`. There's no real use for this so we're not worried about preempting that. (It'd also probably require elevating visibility to some kind of syntactic builtin, since we can't inspect whether/how a function gets aliased.) Work toward #11261. PiperOrigin-RevId: 479378704 Change-Id: I294bf975d8972c5f91bb5585fa118532bba37435 --- .../starlark/BazelBuildApiGlobals.java | 21 ++++++++++++--- .../StarlarkBuildApiGlobals.java | 9 +++---- .../lib/skyframe/BzlLoadFunctionTest.java | 26 ++++--------------- 3 files changed, 26 insertions(+), 30 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/BazelBuildApiGlobals.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/BazelBuildApiGlobals.java index 93a7b4695af2e8..54791f7327d0d8 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/starlark/BazelBuildApiGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/starlark/BazelBuildApiGlobals.java @@ -41,19 +41,32 @@ public class BazelBuildApiGlobals implements StarlarkBuildApiGlobals { @Override public void visibility(Object value, StarlarkThread thread) throws EvalException { - // Manually check the experimental flag because enableOnlyWithFlag doesn't work for top-level - // builtins. + // Confirm .bzl visibility is enabled. We manually check the experimental flag here because + // StarlarkMethod.enableOnlyWithFlag doesn't work for top-level builtins. if (!thread.getSemantics().getBool(BuildLanguageOptions.EXPERIMENTAL_BZL_VISIBILITY)) { throw Starlark.errorf("Use of `visibility()` requires --experimental_bzl_visibility"); } - // Fail if we're not initializing a .bzl module, or if that .bzl module isn't on the - // experimental allowlist, or if visibility is already set. + // Fail if we're not initializing a .bzl module BzlInitThreadContext context = BzlInitThreadContext.fromOrFailFunction(thread, "visibility"); + // Fail if we're not called from the top level. (We prohibit calling visibility() from within + // helper functions because it's more magical / less readable, and it makes it more difficult + // for static tooling to mechanically find and modify visibility() declarations.) + ImmutableList callStack = thread.getCallStack(); + if (!(callStack.size() == 2 + && callStack.get(0).name.equals("") + && callStack.get(1).name.equals("visibility"))) { + throw Starlark.errorf( + ".bzl visibility may only be set at the top level, not inside a function"); + } + + // Fail if the .bzl module isn't on the experimental allowlist. PackageIdentifier pkgId = context.getBzlFile().getPackageIdentifier(); List allowlist = thread.getSemantics().get(BuildLanguageOptions.EXPERIMENTAL_BZL_VISIBILITY_ALLOWLIST); checkVisibilityAllowlist(pkgId, allowlist); + + // Fail if the module's visibility is already set. if (context.getBzlVisibility() != null) { throw Starlark.errorf(".bzl visibility may not be set more than once"); } diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkBuildApiGlobals.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkBuildApiGlobals.java index b45a4ed96effdd..890d73432b74f1 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkBuildApiGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkBuildApiGlobals.java @@ -40,11 +40,10 @@ public interface StarlarkBuildApiGlobals { + " module, the file doing the loading must live in a package that has been granted" + " visibility to the module. A module can always be loaded within its own package," + " regardless of its visibility." - + "

Generally, visibility() is called at the top of the .bzl file," - + " immediately after its load() statements. (It is poor style to put" - + " this declaration later in the file or in a helper method.) It may not be called" - + " more than once per .bzl, or after the .bzl's top-level code has finished" - + " executing.", + + "

visibility() may only be called once per .bzl file, and only at" + + " the top level, not inside a function. The preferred style is to put this call" + + " immediately below the load() statements and any brief logic needed" + + " to determine the argument.", parameters = { @Param( name = "value", diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java index c5532c8ee65ff1..a58bb9eb788415 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/BzlLoadFunctionTest.java @@ -698,36 +698,20 @@ public void testBzlVisibility_failureInDependency() throws Exception { } @Test - public void testBzlVisibility_setNonlocally() throws Exception { + public void testBzlVisibility_cannotBeSetInFunction() throws Exception { setBuildLanguageOptions( "--experimental_bzl_visibility=true", "--experimental_bzl_visibility_allowlist=everyone"); - // Checks a case where visibility() is called in a different package than the module that is - // actually being initialized. (This is bad style in practice, but it's semantically simpler to - // allow it than to go out of our way to ban it.) scratch.file("a/BUILD"); scratch.file( "a/foo.bzl", // - "load(\"//b:bar.bzl\", \"x\")"); - scratch.file("b/BUILD"); - scratch.file( - "b/bar.bzl", // - "load(\"//c:helper.bzl\", \"helper\")", - "helper()", - "x = 1"); - scratch.file("c/BUILD"); - scratch.file( - "c/helper.bzl", // - // Should have a visibility("public") call here, but let's omit it and rely on the default - // being public. That way we can also test that c need not be in the allowlist just to call - // visibility() on behalf of b. "def helper():", - " visibility(\"private\")"); + " visibility(\"public\")", + "helper()"); reporter.removeHandler(failFastHandler); - checkFailingLookup( - "//a:foo.bzl", "module //a:foo.bzl contains .bzl load-visibility violations"); - assertContainsEvent("Starlark file //b:bar.bzl is not visible for loading from package //a."); + checkFailingLookup("//a:foo.bzl", "initialization of module 'a/foo.bzl' failed"); + assertContainsEvent(".bzl visibility may only be set at the top level"); } @Test