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(<foo>)` call in a file is, it can always mechanically modify it to `visibility([some_dep] + <foo>)`. 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

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<StarlarkThread.CallStackEntry> callStack = thread.getCallStack();
+    if (!(callStack.size() == 2
+        && callStack.get(0).name.equals("<toplevel>")
+        && 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<String> 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");
     }

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."
-              + "<p>Generally, <code>visibility()</code> is called at the top of the .bzl file,"
-              + " immediately after its <code>load()</code> 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.",
+              + "<p><code>visibility()</code> 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 <code>load()</code> statements and any brief logic needed"
+              + " to determine the argument.",
       parameters = {
         @Param(
             name = "value",

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