Simplify class and var names related to bzl skyframe inlining

Also clean up some documentation and test names. A follow-up will restructure the inlining logic.

This is refactoring work toward adding a new kind of .bzl loading context, for Bazel-internal .bzl files.

Work toward #11437.

BzlLoadFunction:
- delete "self" from inlining terminology
- shorten method names e.g. "createForInlining" with no suffix specifying clients
- "visitedDepsInToplevelLoad" -> "visitedBzls", "visitedNested" -> "loadStack"
- add some explanatory comments
- hide the builder-factory detail in the inlining manager

CachedBzlLoadValueAndDeps:
- rename to "CachedBzlLoadData", with local instances called "cachedData"
- rename its builder factory
- have traverse() process the root node as well (shreyax@ sees no issue with this)

Other
- rename test cases in BzlLoadFunctionTest

RELNOTES: None
PiperOrigin-RevId: 313428699
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
index 7cf5f65..6c53e18 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/BzlLoadFunction.java
@@ -87,7 +87,7 @@
   private final PackageFactory packageFactory;
 
   private final ASTFileLookupValueManager astFileLookupValueManager;
-  @Nullable private final SelfInliningManager selfInliningManager;
+  @Nullable private final InliningManager inliningManager;
 
   private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
 
@@ -95,11 +95,11 @@
       RuleClassProvider ruleClassProvider,
       PackageFactory packageFactory,
       ASTFileLookupValueManager astFileLookupValueManager,
-      @Nullable SelfInliningManager selfInliningManager) {
+      @Nullable InliningManager inliningManager) {
     this.ruleClassProvider = ruleClassProvider;
     this.packageFactory = packageFactory;
     this.astFileLookupValueManager = astFileLookupValueManager;
-    this.selfInliningManager = selfInliningManager;
+    this.inliningManager = inliningManager;
   }
 
   public static BzlLoadFunction create(
@@ -135,10 +135,10 @@
         // waste.
         new InliningAndCachingASTFileLookupValueManager(
             ruleClassProvider, digestHashFunction, astFileLookupValueCache),
-        /*selfInliningManager=*/ null);
+        /*inliningManager=*/ null);
   }
 
-  public static BzlLoadFunction createForInliningSelfForPackageAndWorkspaceNodes(
+  public static BzlLoadFunction createForInlining(
       RuleClassProvider ruleClassProvider,
       PackageFactory packageFactory,
       int bzlLoadValueCacheSize) {
@@ -151,7 +151,7 @@
         // of a BzlLoadValue inlining cache miss). This is important in the situation where a bzl
         // file is loaded by a lot of other bzl files or BUILD files.
         RegularSkyframeASTFileLookupValueManager.INSTANCE,
-        new SelfInliningManager(bzlLoadValueCacheSize));
+        new InliningManager(bzlLoadValueCacheSize));
   }
 
   @Override
@@ -168,39 +168,48 @@
     }
   }
 
+  /**
+   * Entry point for computing "inline", without any direct or indirect Skyframe calls back into
+   * {@link BzlLoadFunction}. (Other Skyframe calls are permitted.)
+   *
+   * <p>{@code visitedBzls} is a cache of values previously computed, and will be added to by this
+   * call.
+   */
   @Nullable
-  BzlLoadValue computeWithSelfInlineCallsForPackageAndWorkspaceNodes(
-      BzlLoadValue.Key key,
-      Environment env,
-      Map<BzlLoadValue.Key, CachedBzlLoadValueAndDeps> visitedDepsInToplevelLoad)
+  BzlLoadValue computeInline(
+      BzlLoadValue.Key key, Environment env, Map<BzlLoadValue.Key, CachedBzlLoadData> visitedBzls)
       throws InconsistentFilesystemException, BzlLoadFailedException, InterruptedException {
-    Preconditions.checkNotNull(selfInliningManager);
-    // See comments in computeWithSelfInlineCallsInternal for an explanation of the visitedNested
-    // and visitedDepsInToplevelLoad vars.
-    CachedBzlLoadValueAndDeps cachedBzlLoadValueAndDeps =
-        computeWithSelfInlineCallsInternal(
+    Preconditions.checkNotNull(inliningManager);
+    // See comments in computeInlineInternal for an explanation of the loadStack and visitedBzls
+    // vars.
+    // TODO(brandjon): centralize the explanation of these vars.
+    CachedBzlLoadData cachedData =
+        computeInlineInternal(
             key,
             env,
-            // visitedNested must use insertion order to display the correct error.
-            /*visitedNested=*/ new LinkedHashSet<>(),
-            /*visitedDepsInToplevelLoad=*/ visitedDepsInToplevelLoad);
-    if (cachedBzlLoadValueAndDeps == null) {
-      return null;
-    }
-    return cachedBzlLoadValueAndDeps.getValue();
+            // loadStack must use insertion order to display the correct error.
+            /*loadStack=*/ new LinkedHashSet<>(),
+            /*visitedBzls=*/ visitedBzls);
+    return cachedData != null ? cachedData.getValue() : null;
   }
 
+  /**
+   * Retrieves or creates the requested CachedBzlLoadData object, entering it into the caches.
+   *
+   * <p>Returns null if the underlying Skyvalue computation is null (error or Skyframe restart).
+   */
   @Nullable
-  private CachedBzlLoadValueAndDeps computeWithSelfInlineCallsInternal(
+  private CachedBzlLoadData computeInlineInternal(
       BzlLoadValue.Key key,
       Environment env,
-      Set<BzlLoadValue.Key> visitedNested,
-      Map<BzlLoadValue.Key, CachedBzlLoadValueAndDeps> visitedDepsInToplevelLoad)
+      Set<BzlLoadValue.Key> loadStack,
+      Map<BzlLoadValue.Key, CachedBzlLoadData> visitedBzls)
       throws InconsistentFilesystemException, BzlLoadFailedException, InterruptedException {
-    // Under BzlLoadFunction inlining, BUILD and WORKSPACE files are evaluated in separate Skyframe
-    // threads, but all the .bzls transitively loaded by a single package occur in one thread. All
-    // these threads share a global cache in selfInliningManager, so that once any thread completes
-    // evaluation of a .bzl, it needn't be evaluated again (unless it's evicted).
+    // Under BzlLoadFunction inlining, BUILD and WORKSPACE files (and the singular
+    // StarlarkBuiltinsFunction) are evaluated in separate Skyframe threads, but all the .bzls
+    // transitively loaded by a single package occur in one thread. All these threads share a global
+    // cache in inliningManager, so that once any thread completes evaluation of a .bzl, it needn't
+    // be evaluated again (unless it's evicted).
     //
     // If two threads race to evaluate the same .bzl, each one will see a different copy of it, and
     // only one will end up in the global cache. This presents a hazard if the same BUILD or
@@ -213,40 +222,41 @@
     // two threads could deadlock while trying to evaluate an illegal load() cycle from opposite
     // ends.)
     //
-    // To solve this, we keep a second cache in visitedDepsInToplevelLoad, of just the .bzls
-    // transitively loaded in the current package. The entry for foo.bzl may be a different copy
-    // than the one in the global cache, but the BUILD or WORKSPACE file won't know the difference.
-    // (We don't need to worry about Starlark values from different packages interacting since
-    // inlining is only used for the loading phase.)
+    // To solve this, we keep a second cache in visitedBzls, of just the .bzls transitively loaded
+    // in the current package. The entry for foo.bzl may be a different copy than the one in the
+    // global cache, but the BUILD or WORKSPACE file won't know the difference. (We don't need to
+    // worry about Starlark values from different packages interacting since inlining is only used
+    // for the loading phase.)
     //
-    CachedBzlLoadValueAndDeps cachedBzlLoadValueAndDeps = visitedDepsInToplevelLoad.get(key);
-    if (cachedBzlLoadValueAndDeps == null) {
-      cachedBzlLoadValueAndDeps = selfInliningManager.bzlLoadValueCache.getIfPresent(key);
-      if (cachedBzlLoadValueAndDeps != null) {
-        cachedBzlLoadValueAndDeps.traverse(env::registerDependencies, visitedDepsInToplevelLoad);
+    CachedBzlLoadData cachedData = visitedBzls.get(key);
+    if (cachedData == null) {
+      cachedData = inliningManager.bzlLoadValueCache.getIfPresent(key);
+      if (cachedData != null) {
+        // Found a cache hit from another thread's computation; register the recorded deps as if our
+        // thread required them for the current key. Incorporate into visitedBzls any transitive
+        // cache hits it does not already contain.
+        cachedData.traverse(env::registerDependencies, visitedBzls);
       }
     }
-    if (cachedBzlLoadValueAndDeps != null) {
-      return cachedBzlLoadValueAndDeps;
+    if (cachedData != null) {
+      return cachedData;
     }
 
-    // visitedNested is keyed on the SkyKey, not the label, because it's possible for distinct keys
-    // to share the same label. Examples include the "@builtins" pseudo-repo vs a real repository
-    // that happens to be named "@builtins", or keys for the same .bzl with different workspace
-    // chunking information. It's unclear whether these particular cycles can arise in practice, but
-    // it doesn't hurt to be robust to future changes that may make that possible.
-    if (!visitedNested.add(key)) {
+    // loadStack is keyed on the SkyKey, not the label, because it's possible for distinct keys to
+    // share the same label. Examples include the "@builtins" pseudo-repo vs a real repository that
+    // happens to be named "@builtins", or keys for the same .bzl with different workspace chunking
+    // information. It's unclear whether these particular cycles can arise in practice, but it
+    // doesn't hurt to be robust to future changes that may make that possible.
+    if (!loadStack.add(key)) {
       ImmutableList<BzlLoadValue.Key> cycle =
-          CycleUtils.splitIntoPathAndChain(Predicates.equalTo(key), visitedNested).second;
+          CycleUtils.splitIntoPathAndChain(Predicates.equalTo(key), loadStack).second;
       throw new BzlLoadFailedException("Starlark load cycle: " + cycle);
     }
 
-    CachedBzlLoadValueAndDeps.Builder inlineCachedValueBuilder =
-        selfInliningManager.cachedBzlLoadValueAndDepsBuilderFactory
-            .newCachedBzlLoadValueAndDepsBuilder();
-    // Use an instrumented Skyframe env to capture Skyframe deps in the CachedBzlLoadValueAndDeps.
-    // This is transitive but doesn't include deps underneath recursively loaded .bzls (the
-    // recursion uses the unwrapped original env).
+    CachedBzlLoadData.Builder cachedDataBuilder = inliningManager.cachedDataBuilder();
+    // Use an instrumented Skyframe env to capture Skyframe deps in the CachedBzlLoadData. This is
+    // transitive but doesn't include deps underneath recursively loaded .bzls (the recursion uses
+    // the unwrapped original env).
     Preconditions.checkState(
         !(env instanceof RecordingSkyFunctionEnvironment),
         "Found nested RecordingSkyFunctionEnvironment but it should have been stripped: %s",
@@ -254,29 +264,27 @@
     RecordingSkyFunctionEnvironment recordingEnv =
         new RecordingSkyFunctionEnvironment(
             env,
-            inlineCachedValueBuilder::addDep,
-            inlineCachedValueBuilder::addDeps,
-            inlineCachedValueBuilder::noteException);
+            cachedDataBuilder::addDep,
+            cachedDataBuilder::addDeps,
+            cachedDataBuilder::noteException);
     BzlLoadValue value =
         computeInternal(
-            key,
-            recordingEnv,
-            new InliningState(visitedNested, inlineCachedValueBuilder, visitedDepsInToplevelLoad));
+            key, recordingEnv, new InliningState(loadStack, cachedDataBuilder, visitedBzls));
     // All loads traversed, this key can no longer be part of a cycle.
-    Preconditions.checkState(visitedNested.remove(key), key);
+    Preconditions.checkState(loadStack.remove(key), key);
 
     if (value != null) {
-      inlineCachedValueBuilder.setValue(value);
-      inlineCachedValueBuilder.setKey(key);
-      cachedBzlLoadValueAndDeps = inlineCachedValueBuilder.build();
-      visitedDepsInToplevelLoad.put(key, cachedBzlLoadValueAndDeps);
-      selfInliningManager.bzlLoadValueCache.put(key, cachedBzlLoadValueAndDeps);
+      cachedDataBuilder.setValue(value);
+      cachedDataBuilder.setKey(key);
+      cachedData = cachedDataBuilder.build();
+      visitedBzls.put(key, cachedData);
+      inliningManager.bzlLoadValueCache.put(key, cachedData);
     }
-    return cachedBzlLoadValueAndDeps;
+    return cachedData;
   }
 
-  public void resetSelfInliningCache() {
-    selfInliningManager.reset();
+  public void resetInliningCache() {
+    inliningManager.reset();
   }
 
   private static ContainingPackageLookupValue getContainingPackageLookupValue(
@@ -313,18 +321,22 @@
     return containingPackageLookupValue;
   }
 
+  /**
+   * Value class bundling parameters that are only used in the code path where BzlLoadFunction calls
+   * are inlined.
+   */
   private static class InliningState {
-    private final Set<BzlLoadValue.Key> visitedNested;
-    private final CachedBzlLoadValueAndDeps.Builder inlineCachedValueBuilder;
-    private final Map<BzlLoadValue.Key, CachedBzlLoadValueAndDeps> visitedDepsInToplevelLoad;
+    private final Set<BzlLoadValue.Key> loadStack;
+    private final CachedBzlLoadData.Builder cachedDataBuilder;
+    private final Map<BzlLoadValue.Key, CachedBzlLoadData> visitedBzls;
 
     private InliningState(
-        Set<BzlLoadValue.Key> visitedNested,
-        CachedBzlLoadValueAndDeps.Builder inlineCachedValueBuilder,
-        Map<BzlLoadValue.Key, CachedBzlLoadValueAndDeps> visitedDepsInToplevelLoad) {
-      this.visitedNested = visitedNested;
-      this.inlineCachedValueBuilder = inlineCachedValueBuilder;
-      this.visitedDepsInToplevelLoad = visitedDepsInToplevelLoad;
+        Set<BzlLoadValue.Key> loadStack,
+        CachedBzlLoadData.Builder cachedDataBuilder,
+        Map<BzlLoadValue.Key, CachedBzlLoadData> visitedBzls) {
+      this.loadStack = loadStack;
+      this.cachedDataBuilder = cachedDataBuilder;
+      this.visitedBzls = visitedBzls;
     }
   }
 
@@ -417,7 +429,7 @@
     List<BzlLoadValue> bzlLoads =
         inliningState == null
             ? computeBzlLoadsNoInlining(env, loadKeys, file.getStartLocation())
-            : computeBzlLoadsWithSelfInlining(env, loadKeys, label, inliningState);
+            : computeBzlLoadsWithInlining(env, loadKeys, label, inliningState);
     if (bzlLoads == null) {
       return null; // Skyframe deps unavailable
     }
@@ -555,8 +567,8 @@
   }
 
   /**
-   * Compute the BzlLoadValue for all given keys using vanilla Skyframe evaluation, returning {@code
-   * null} if Skyframe deps were missing and have been requested.
+   * Computes the BzlLoadValue for all given keys using vanilla Skyframe evaluation, returning
+   * {@code null} if Skyframe deps were missing and have been requested.
    */
   @Nullable
   private static List<BzlLoadValue> computeBzlLoadsNoInlining(
@@ -578,12 +590,12 @@
   }
 
   /**
-   * Compute the BzlLoadValue for all given keys by reusing this instance of the BzlLoadFunction,
+   * Computes the BzlLoadValue for all given keys by reusing this instance of the BzlLoadFunction,
    * bypassing traditional Skyframe evaluation, returning {@code null} if Skyframe deps were missing
    * and have been requested.
    */
   @Nullable
-  private List<BzlLoadValue> computeBzlLoadsWithSelfInlining(
+  private List<BzlLoadValue> computeBzlLoadsWithInlining(
       Environment env, List<BzlLoadValue.Key> keys, Label fileLabel, InliningState inliningState)
       throws InterruptedException, BzlLoadFailedException, InconsistentFilesystemException {
     Preconditions.checkState(
@@ -596,29 +608,26 @@
     boolean valuesMissing = false;
     // NOTE: Iterating over loads in the order listed in the file.
     for (BzlLoadValue.Key key : keys) {
-      CachedBzlLoadValueAndDeps cachedValue;
+      CachedBzlLoadData cachedData;
       try {
-        cachedValue =
-            computeWithSelfInlineCallsInternal(
-                key,
-                strippedEnv,
-                inliningState.visitedNested,
-                inliningState.visitedDepsInToplevelLoad);
+        cachedData =
+            computeInlineInternal(
+                key, strippedEnv, inliningState.loadStack, inliningState.visitedBzls);
       } catch (BzlLoadFailedException | InconsistentFilesystemException e) {
         // For determinism's sake while inlining, preserve the first exception and continue to run
         // subsequently listed loads to completion/exception, loading all transitive deps anyway.
         deferredException = MoreObjects.firstNonNull(deferredException, e);
         continue;
       }
-      if (cachedValue == null) {
+      if (cachedData == null) {
         Preconditions.checkState(env.valuesMissing(), "no starlark load value for %s", key);
         // We continue making inline calls even if some requested values are missing, to maximize
         // the number of dependent (non-inlined) SkyFunctions that are requested, thus avoiding a
         // quadratic number of restarts.
         valuesMissing = true;
       } else {
-        bzlLoads.add(cachedValue.getValue());
-        inliningState.inlineCachedValueBuilder.addTransitiveDeps(cachedValue);
+        bzlLoads.add(cachedData.getValue());
+        inliningState.cachedDataBuilder.addTransitiveDeps(cachedData);
       }
     }
     if (deferredException != null) {
@@ -830,22 +839,27 @@
     }
   }
 
-  private static class SelfInliningManager {
+  /** Per-instance manager for when {@code BzlLoadFunction} calls are inlined. */
+  private static class InliningManager {
     private final int bzlLoadValueCacheSize;
-    private Cache<BzlLoadValue.Key, CachedBzlLoadValueAndDeps> bzlLoadValueCache;
-    private CachedBzlLoadValueAndDepsBuilderFactory cachedBzlLoadValueAndDepsBuilderFactory =
-        new CachedBzlLoadValueAndDepsBuilderFactory();
+    private Cache<BzlLoadValue.Key, CachedBzlLoadData> bzlLoadValueCache;
+    private CachedBzlLoadDataBuilderFactory cachedBzlLoadDataBuilderFactory =
+        new CachedBzlLoadDataBuilderFactory();
 
-    private SelfInliningManager(int bzlLoadValueCacheSize) {
+    private InliningManager(int bzlLoadValueCacheSize) {
       this.bzlLoadValueCacheSize = bzlLoadValueCacheSize;
     }
 
+    private CachedBzlLoadData.Builder cachedDataBuilder() {
+      return cachedBzlLoadDataBuilderFactory.newCachedBzlLoadDataBuilder();
+    }
+
     private void reset() {
       if (bzlLoadValueCache != null) {
         logger.atInfo().log(
             "Starlark inlining cache stats from earlier build: " + bzlLoadValueCache.stats());
       }
-      cachedBzlLoadValueAndDepsBuilderFactory = new CachedBzlLoadValueAndDepsBuilderFactory();
+      cachedBzlLoadDataBuilderFactory = new CachedBzlLoadDataBuilderFactory();
       Preconditions.checkState(
           bzlLoadValueCacheSize >= 0,
           "Expected positive Starlark cache size if caching. %s",