From c630a7c5303d1472ef9605d84cef33debeca00a5 Mon Sep 17 00:00:00 2001
From: salma-samy <salmasamy@google.com>
Date: Tue, 16 May 2023 13:32:46 -0700
Subject: [PATCH] Stop checking .bzl files visibility in RepoRuleFunction Fixes
 https://github.com/bazelbuild/bazel/issues/18346

At this point we are creating a rule from an extension repospec and that is always public

PiperOrigin-RevId: 532558852
Change-Id: I245de0a23f92ba6283be4bf3be6af4863b7a8c68
---
 .../lib/skyframe/BzlmodRepoRuleFunction.java  |  6 ++---
 .../build/lib/skyframe/PackageFunction.java   | 26 ++++++++++++-------
 .../lib/skyframe/WorkspaceFileFunction.java   |  3 ++-
 3 files changed, 21 insertions(+), 14 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java
index 7d51708fca58e6..dfaf1c83d19a33 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlmodRepoRuleFunction.java
@@ -239,8 +239,7 @@ private ImmutableMap<String, Module> loadBzlModules(
 
     // Load the .bzl module.
     try {
-      // TODO(b/22193153, wyv): Determine whether .bzl load visibility should apply at all to this
-      // type of .bzl load. As it stands, this call checks that bzlFile is visible to package @//.
+      // No need to check visibility for an extension repospec that is always public
       return PackageFunction.loadBzlModules(
           env,
           PackageIdentifier.EMPTY_PACKAGE_ID,
@@ -248,7 +247,8 @@ private ImmutableMap<String, Module> loadBzlModules(
           programLoads,
           keys,
           starlarkSemantics,
-          null);
+          null,
+          /* checkVisibility= */ false);
     } catch (NoSuchPackageException e) {
       throw new BzlmodRepoRuleFunctionException(e, Transience.PERSISTENT);
     }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
index 4e8f3732eb466a..3977631986e5fb 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PackageFunction.java
@@ -636,6 +636,7 @@ private static FileValue getBuildFileValue(Environment env, RootedPath buildFile
    * construction to the caller, so that loadPrelude can become just a call to the factored-out
    * code.
    */
+  // TODO(18422): Cleanup/refactor this method's signature.
   @Nullable
   static ImmutableMap<String, Module> loadBzlModules(
       Environment env,
@@ -644,7 +645,8 @@ static ImmutableMap<String, Module> loadBzlModules(
       List<Pair<String, Location>> programLoads,
       List<BzlLoadValue.Key> keys,
       StarlarkSemantics semantics,
-      @Nullable BzlLoadFunction bzlLoadFunctionForInlining)
+      @Nullable BzlLoadFunction bzlLoadFunctionForInlining,
+      boolean checkVisibility)
       throws NoSuchPackageException, InterruptedException {
     List<BzlLoadValue> bzlLoads;
     try {
@@ -657,14 +659,17 @@ static ImmutableMap<String, Module> loadBzlModules(
       }
       // Validate that the current BUILD/WORKSPACE file satisfies each loaded dependency's
       // load visibility.
-      BzlLoadFunction.checkLoadVisibilities(
-          packageId,
-          requestingFileDescription,
-          bzlLoads,
-          keys,
-          programLoads,
-          /*demoteErrorsToWarnings=*/ !semantics.getBool(BuildLanguageOptions.CHECK_BZL_VISIBILITY),
-          env.getListener());
+      if (checkVisibility) {
+        BzlLoadFunction.checkLoadVisibilities(
+            packageId,
+            requestingFileDescription,
+            bzlLoads,
+            keys,
+            programLoads,
+            /* demoteErrorsToWarnings= */ !semantics.getBool(
+                BuildLanguageOptions.CHECK_BZL_VISIBILITY),
+            env.getListener());
+      }
     } catch (BzlLoadFailedException e) {
       Throwable rootCause = Throwables.getRootCause(e);
       throw PackageFunctionException.builder()
@@ -1325,7 +1330,8 @@ private LoadedPackage loadPackage(
                   programLoads,
                   keys.build(),
                   starlarkBuiltinsValue.starlarkSemantics,
-                  bzlLoadFunctionForInlining);
+                  bzlLoadFunctionForInlining,
+                  /* checkVisibility= */ true);
         } catch (NoSuchPackageException e) {
           throw new PackageFunctionException(e, Transience.PERSISTENT);
         }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
index 768586cb067543..83b7c0b994fe2f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunction.java
@@ -333,7 +333,8 @@ public SkyValue compute(SkyKey skyKey, Environment env)
               programLoads,
               keys.build(),
               starlarkSemantics,
-              bzlLoadFunctionForInlining);
+              bzlLoadFunctionForInlining,
+              /* checkVisibility= */ true);
     } catch (NoSuchPackageException e) {
       throw new WorkspaceFileFunctionException(e, Transience.PERSISTENT);
     }