[7.6.0] Bzlmod: nodep deps (#25509)

This PR implements the "nodep" edges from
https://docs.google.com/document/d/1JsfbH9kdMe3dyOY-IR8SUakS541A7OM8pQcKpxTRMRs/edit?tab=t.0,
using the syntax of `bazel_dep(..., repo_name=None)`. The behavior is
that these edges are "unfulfilled" unless the module they refer to
already exist in the dep graph by some other means.

Most of the changes are in the Discovery class -- I reorganized the code
in there to hopefully help with readability, given the new multi-round
discovery logic.

Also changed `ModuleFileValue.Key` to no longer take the applicable
override next to the module key -- `ModuleFileFunction` now gets the
root module from Skyframe itself and looks up the correct override. The
old setup was always weird (what does it mean to request `[email protected]` with
an incorrect override??).

Work towards #25214

RELNOTES: The `repo_name` parameter of `bazel_dep` can now be set to
`None` to mark it a "nodep" dependency -- that is, the `bazel_dep`
specification is only honored if the target module already exists in the
dependency graph by some other means.

PiperOrigin-RevId: 730962587
Change-Id: I1ca7a7a1228da5e4c2e4b5d6983a2ec97b31a4b8

---------

Co-authored-by: Yun Peng <[email protected]>

Author: Wyverald

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModTidyFunction.java

@@ -102,6 +102,6 @@ public SkyValue compute(SkyKey skyKey, Environment env)
     }
 
     return BazelModTidyValue.create(
-        fixups.build(), buildozer.asPath(), rootModuleFileValue.getModuleFilePaths());
+        fixups.build(), buildozer.asPath(), rootModuleFileValue.moduleFilePaths());
   }
 }

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleInspectorFunction.java

@@ -60,7 +60,7 @@ public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedExcept
     if (resolutionValue == null) {
       return null;
     }
-    ImmutableMap<String, ModuleOverride> overrides = root.getOverrides();
+    ImmutableMap<String, ModuleOverride> overrides = root.overrides();
     ImmutableMap<ModuleKey, InterimModule> unprunedDepGraph = resolutionValue.getUnprunedDepGraph();
     ImmutableMap<ModuleKey, Module> resolvedDepGraph = resolutionValue.getResolvedDepGraph();
 

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionFunction.java

@@ -113,7 +113,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
     SequencedMap<String, Optional<Checksum>> registryFileHashes =
         new LinkedHashMap<>(state.discoverAndSelectResult.registryFileHashes);
     ImmutableSet<RepoSpecKey> repoSpecKeys =
-        state.discoverAndSelectResult.selectionResult.getResolvedDepGraph().values().stream()
+        state.discoverAndSelectResult.selectionResult.resolvedDepGraph().values().stream()
             // Modules with a null registry have a non-registry override. We don't need to
             // fetch or store the repo spec in this case.
             .filter(module -> module.getRegistry() != null)
@@ -135,14 +135,14 @@ public SkyValue compute(SkyKey skyKey, Environment env)
         Profiler.instance().profile(ProfilerTask.BZLMOD, "compute final dep graph")) {
       finalDepGraph =
           computeFinalDepGraph(
-              state.discoverAndSelectResult.selectionResult.getResolvedDepGraph(),
-              root.getOverrides(),
+              state.discoverAndSelectResult.selectionResult.resolvedDepGraph(),
+              root.overrides(),
               remoteRepoSpecs.buildOrThrow());
     }
 
     return BazelModuleResolutionValue.create(
         finalDepGraph,
-        state.discoverAndSelectResult.selectionResult.getUnprunedDepGraph(),
+        state.discoverAndSelectResult.selectionResult.unprunedDepGraph(),
         ImmutableMap.copyOf(registryFileHashes),
         state.discoverAndSelectResult.selectedYankedVersions);
   }
@@ -163,15 +163,15 @@ private static Result discoverAndSelect(
       return null;
     }
 
-    verifyAllOverridesAreOnExistentModules(discoveryResult.depGraph(), root.getOverrides());
+    verifyAllOverridesAreOnExistentModules(discoveryResult.depGraph(), root.overrides());
 
     Selection.Result selectionResult;
     try (SilentCloseable c = Profiler.instance().profile(ProfilerTask.BZLMOD, "selection")) {
-      selectionResult = Selection.run(discoveryResult.depGraph(), root.getOverrides());
+      selectionResult = Selection.run(discoveryResult.depGraph(), root.overrides());
     } catch (ExternalDepsException e) {
       throw new BazelModuleResolutionFunctionException(e, Transience.PERSISTENT);
     }
-    ImmutableMap<ModuleKey, InterimModule> resolvedDepGraph = selectionResult.getResolvedDepGraph();
+    ImmutableMap<ModuleKey, InterimModule> resolvedDepGraph = selectionResult.resolvedDepGraph();
 
     ImmutableMap<ModuleKey, YankedVersionsValue> yankedVersionsValues;
     try (SilentCloseable c =

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Discovery.java

@@ -15,26 +15,25 @@
 
 package com.google.devtools.build.lib.bazel.bzlmod;
 
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
 import static java.util.stream.Collectors.joining;
 
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.devtools.build.lib.bazel.bzlmod.InterimModule.DepSpec;
 import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileValue.RootModuleFileValue;
 import com.google.devtools.build.lib.bazel.repository.downloader.Checksum;
 import com.google.devtools.build.lib.server.FailureDetails;
 import com.google.devtools.build.skyframe.SkyFunction.Environment;
-import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyframeLookupResult;
-import java.util.ArrayDeque;
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.LinkedHashMap;
-import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
-import java.util.Queue;
 import java.util.SequencedMap;
 import java.util.Set;
 import javax.annotation.Nullable;
@@ -58,82 +57,179 @@ public record Result(
   @Nullable
   public static Result run(Environment env, RootModuleFileValue root)
       throws InterruptedException, ExternalDepsException {
-    String rootModuleName = root.getModule().getName();
-    ImmutableMap<String, ModuleOverride> overrides = root.getOverrides();
-    Map<ModuleKey, InterimModule> depGraph = new HashMap<>();
-    depGraph.put(
-        ModuleKey.ROOT,
-        root.getModule()
-            .withDepSpecsTransformed(InterimModule.applyOverrides(overrides, rootModuleName)));
-    Queue<ModuleKey> unexpanded = new ArrayDeque<>();
-    Map<ModuleKey, ModuleKey> predecessors = new HashMap<>();
-    SequencedMap<String, Optional<Checksum>> registryFileHashes =
-        new LinkedHashMap<>(root.getRegistryFileHashes());
-    unexpanded.add(ModuleKey.ROOT);
-    while (!unexpanded.isEmpty()) {
-      Set<SkyKey> unexpandedSkyKeys = new LinkedHashSet<>();
-      while (!unexpanded.isEmpty()) {
-        InterimModule module = depGraph.get(unexpanded.remove());
+    // Because of the possible existence of nodep edges, we do multiple rounds of discovery.
+    // In each round, we keep track of unfulfilled nodep edges, and at the end of the round, if any
+    // unfulfilled nodep edge can now be fulfilled, we run another round.
+    ImmutableSet<String> prevRoundModuleNames = ImmutableSet.of(root.module().getName());
+    while (true) {
+      DiscoveryRound discoveryRound = new DiscoveryRound(env, root, prevRoundModuleNames);
+      Result result = discoveryRound.run();
+      if (result == null) {
+        return null;
+      }
+      prevRoundModuleNames =
+          result.depGraph().values().stream().map(InterimModule::getName).collect(toImmutableSet());
+      if (discoveryRound.unfulfilledNodepEdgeModuleNames.stream()
+          .noneMatch(prevRoundModuleNames::contains)) {
+        return result;
+      }
+    }
+  }
+
+  private static class DiscoveryRound {
+    private final Environment env;
+    private final RootModuleFileValue root;
+    private final ImmutableSet<String> prevRoundModuleNames;
+    private final Map<ModuleKey, InterimModule> depGraph = new HashMap<>();
+
+    /**
+     * Stores a mapping from a module to its "predecessor" -- that is, its first dependent in BFS
+     * order. This is used to report a dependency chain in errors (see {@link
+     * #maybeReportDependencyChain}.
+     */
+    private final Map<ModuleKey, ModuleKey> predecessors = new HashMap<>();
+
+    /**
+     * For all unfulfilled nodep edges seen during this round, this set stores the module names of
+     * those nodep edges. Remember that whether a nodep edge can be fulfilled depends on whether the
+     * module it names already exists in the dep graph.
+     */
+    private final Set<String> unfulfilledNodepEdgeModuleNames = new HashSet<>();
+
+    DiscoveryRound(
+        Environment env, RootModuleFileValue root, ImmutableSet<String> prevRoundModuleNames) {
+      this.env = env;
+      this.root = root;
+      this.prevRoundModuleNames = prevRoundModuleNames;
+    }
+
+    /**
+     * Runs one round of discovery. At its core, this is a simple breadth-first search: we start
+     * from the "horizon" of just the root module, and advance the horizon by discovering the
+     * dependencies of modules in the current horizon. Keep doing this until the horizon is empty.
+     */
+    @Nullable
+    Result run() throws InterruptedException, ExternalDepsException {
+      SequencedMap<String, Optional<Checksum>> registryFileHashes = new LinkedHashMap<>();
+      depGraph.put(ModuleKey.ROOT, root.module().withDepSpecsTransformed(this::applyOverrides));
+      ImmutableSet<ModuleKey> horizon = ImmutableSet.of(ModuleKey.ROOT);
+      while (!horizon.isEmpty()) {
+        ImmutableSet<ModuleFileValue.Key> nextHorizonSkyKeys = advanceHorizon(horizon);
+        SkyframeLookupResult result = env.getValuesAndExceptions(nextHorizonSkyKeys);
+        var nextHorizon = ImmutableSet.<ModuleKey>builder();
+        for (ModuleFileValue.Key skyKey : nextHorizonSkyKeys) {
+          ModuleKey depKey = skyKey.moduleKey();
+          ModuleFileValue moduleFileValue;
+          try {
+            moduleFileValue =
+                (ModuleFileValue) result.getOrThrow(skyKey, ExternalDepsException.class);
+          } catch (ExternalDepsException e) {
+            throw maybeReportDependencyChain(e, depKey);
+          }
+          if (moduleFileValue == null) {
+            // Don't return yet. Try to expand any other unexpanded nodes before returning.
+            depGraph.put(depKey, null);
+          } else {
+            depGraph.put(
+                depKey, moduleFileValue.module().withDepSpecsTransformed(this::applyOverrides));
+            registryFileHashes.putAll(moduleFileValue.registryFileHashes());
+            nextHorizon.add(depKey);
+          }
+        }
+        horizon = nextHorizon.build();
+      }
+      if (env.valuesMissing()) {
+        return null;
+      }
+      return new Result(ImmutableMap.copyOf(depGraph), ImmutableMap.copyOf(registryFileHashes));
+    }
+
+    /**
+     * Returns a new {@link DepSpec} that is transformed according to any existing overrides on the
+     * dependency module.
+     */
+    DepSpec applyOverrides(DepSpec depSpec) {
+      if (root.module().getName().equals(depSpec.getName())) {
+        return DepSpec.fromModuleKey(ModuleKey.ROOT);
+      }
+      Version newVersion =
+          switch (root.overrides().get(depSpec.getName())) {
+            case NonRegistryOverride nro -> Version.EMPTY;
+            case SingleVersionOverride svo when !svo.getVersion().isEmpty() -> svo.getVersion();
+            case null, default -> depSpec.getVersion();
+          };
+      return DepSpec.create(depSpec.getName(), newVersion, depSpec.getMaxCompatibilityLevel());
+    }
+
+    /**
+     * Given a set of module keys to discover (the current "horizon"), return the next horizon
+     * consisting of newly discovered module keys from the current set (mostly, their dependencies).
+     *
+     * <p>The current horizon contains keys to modules that are already in the {@code depGraph}.
+     * Note also that this method mutates {@code predecessors} and {@code
+     * unfulfilledNodepEdgeModuleNames}.
+     */
+    ImmutableSet<ModuleFileValue.Key> advanceHorizon(ImmutableSet<ModuleKey> horizon) {
+      var nextHorizon = ImmutableSet.<ModuleFileValue.Key>builder();
+      for (ModuleKey moduleKey : horizon) {
+        InterimModule module = depGraph.get(moduleKey);
+        // The main group of module keys to discover are the current horizon's normal deps.
         for (DepSpec depSpec : module.getDeps().values()) {
-          if (depGraph.containsKey(depSpec.toModuleKey())) {
+          ModuleKey depKey = depSpec.toModuleKey();
+          if (depGraph.containsKey(depKey)) {
             continue;
           }
-          predecessors.putIfAbsent(depSpec.toModuleKey(), module.getKey());
-          unexpandedSkyKeys.add(
-              ModuleFileValue.key(depSpec.toModuleKey(), overrides.get(depSpec.getName())));
+          predecessors.putIfAbsent(depKey, module.getKey());
+          nextHorizon.add(ModuleFileValue.key(depKey));
         }
-      }
-      SkyframeLookupResult result = env.getValuesAndExceptions(unexpandedSkyKeys);
-      for (SkyKey skyKey : unexpandedSkyKeys) {
-        ModuleKey depKey = ((ModuleFileValue.Key) skyKey).getModuleKey();
-        ModuleFileValue moduleFileValue;
-        try {
-          moduleFileValue =
-              (ModuleFileValue) result.getOrThrow(skyKey, ExternalDepsException.class);
-        } catch (ExternalDepsException e) {
-          if (e.getDetailedExitCode().getFailureDetail() == null
-              || e.getDetailedExitCode().getFailureDetail().getExternalDeps().getCode()
-                  != FailureDetails.ExternalDeps.Code.BAD_MODULE) {
-            // This is not due to a bad module, so don't print a dependency chain. This covers cases
-            // such as a parse error in the lockfile or an I/O exception during registry access,
-            // which aren't related to any particular module dep.
-            throw e;
+        // Any of the current horizon's nodep deps should also be discovered ("fulfilled"), iff the
+        // module they refer to already exists in the dep graph. Otherwise, record these unfulfilled
+        // nodep edges, so that we can later decide whether to run another round of discovery.
+        for (DepSpec depSpec : module.getNodepDeps()) {
+          ModuleKey depKey = depSpec.toModuleKey();
+          if (depGraph.containsKey(depKey)) {
+            continue;
           }
-          // Trace back a dependency chain to the root module. There can be multiple paths to the
-          // failing module, but any of those is useful for debugging.
-          List<ModuleKey> depChain = new ArrayList<>();
-          depChain.add(depKey);
-          ModuleKey predecessor = depKey;
-          while ((predecessor = predecessors.get(predecessor)) != null) {
-            depChain.add(predecessor);
+          if (!prevRoundModuleNames.contains(depSpec.getName())) {
+            unfulfilledNodepEdgeModuleNames.add(depSpec.getName());
+            continue;
           }
-          Collections.reverse(depChain);
-          String depChainString =
-              depChain.stream().map(ModuleKey::toString).collect(joining(" -> "));
-          throw ExternalDepsException.withCauseAndMessage(
-              FailureDetails.ExternalDeps.Code.BAD_MODULE,
-              e,
-              "in module dependency chain %s",
-              depChainString);
-        }
-        if (moduleFileValue == null) {
-          // Don't return yet. Try to expand any other unexpanded nodes before returning.
-          depGraph.put(depKey, null);
-        } else {
-          depGraph.put(
-              depKey,
-              moduleFileValue
-                  .getModule()
-                  .withDepSpecsTransformed(
-                      InterimModule.applyOverrides(overrides, rootModuleName)));
-          registryFileHashes.putAll(moduleFileValue.getRegistryFileHashes());
-          unexpanded.add(depKey);
+          predecessors.putIfAbsent(depKey, module.getKey());
+          nextHorizon.add(ModuleFileValue.key(depKey));
         }
       }
+      return nextHorizon.build();
     }
-    if (env.valuesMissing()) {
-      return null;
+
+    /**
+     * When an exception occurs while discovering a new dep, try to add information about the
+     * dependency chain that led to that dep.
+     */
+    private ExternalDepsException maybeReportDependencyChain(
+        ExternalDepsException e, ModuleKey depKey) {
+      if (e.getDetailedExitCode().getFailureDetail() == null
+          || e.getDetailedExitCode().getFailureDetail().getExternalDeps().getCode()
+              != FailureDetails.ExternalDeps.Code.BAD_MODULE) {
+        // This is not due to a bad module, so don't print a dependency chain. This covers cases
+        // such as a parse error in the lockfile or an I/O exception during registry access,
+        // which aren't related to any particular module dep.
+        return e;
+      }
+      // Trace back a dependency chain to the root module. There can be multiple paths to the
+      // failing module, but any of those is useful for debugging.
+      List<ModuleKey> depChain = new ArrayList<>();
+      depChain.add(depKey);
+      ModuleKey predecessor = depKey;
+      while ((predecessor = predecessors.get(predecessor)) != null) {
+        depChain.add(predecessor);
+      }
+      Collections.reverse(depChain);
+      String depChainString = depChain.stream().map(ModuleKey::toString).collect(joining(" -> "));
+      return ExternalDepsException.withCauseAndMessage(
+          FailureDetails.ExternalDeps.Code.BAD_MODULE,
+          e,
+          "in module dependency chain %s",
+          depChainString);
     }
-    return new Result(ImmutableMap.copyOf(depGraph), ImmutableMap.copyOf(registryFileHashes));
   }
 }

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/InterimModule.java

@@ -84,6 +84,13 @@ public final ModuleKey toModuleKey() {
    */
   public abstract ImmutableMap<String, DepSpec> getOriginalDeps();
 
+  /**
+   * The "nodep" dependencies of this module: these don't actually add a dependency on the specified
+   * module, but if specified module is somehow in the dependency graph, it'll be at least at this
+   * version.
+   */
+  public abstract ImmutableList<DepSpec> getNodepDeps();
+
   /**
    * The registry where this module came from. Must be null iff the module has a {@link
    * NonRegistryOverride}.
@@ -159,6 +166,16 @@ public final Builder addToolchainsToRegister(Iterable<String> values) {
 
     public abstract Builder setDeps(ImmutableMap<String, DepSpec> value);
 
+    abstract ImmutableList.Builder<DepSpec> nodepDepsBuilder();
+
+    @CanIgnoreReturnValue
+    public final Builder addNodepDep(DepSpec value) {
+      nodepDepsBuilder().add(value);
+      return this;
+    }
+
+    public abstract Builder setNodepDeps(ImmutableList<DepSpec> value);
+
     public abstract Builder setRegistry(Registry value);
 
     public abstract Builder setExtensionUsages(ImmutableList<ModuleExtensionUsage> value);
@@ -229,26 +246,4 @@ private static RepoSpec maybeAppendAdditionalPatches(
         .setAttributes(AttributeValues.create(attrBuilder.buildOrThrow()))
         .build();
   }
-
-  static UnaryOperator<DepSpec> applyOverrides(
-      ImmutableMap<String, ModuleOverride> overrides, String rootModuleName) {
-    return depSpec -> {
-      if (rootModuleName.equals(depSpec.getName())) {
-        return DepSpec.fromModuleKey(ModuleKey.ROOT);
-      }
-
-      Version newVersion = depSpec.getVersion();
-      @Nullable ModuleOverride override = overrides.get(depSpec.getName());
-      if (override instanceof NonRegistryOverride) {
-        newVersion = Version.EMPTY;
-      } else if (override instanceof SingleVersionOverride) {
-        Version overrideVersion = ((SingleVersionOverride) override).getVersion();
-        if (!overrideVersion.isEmpty()) {
-          newVersion = overrideVersion;
-        }
-      }
-
-      return DepSpec.create(depSpec.getName(), newVersion, depSpec.getMaxCompatibilityLevel());
-    };
-  }
 }