Streamline ModuleExtensionId#toString()

Currently `ModuleExtensionId#toString()` uses the default Java record repr, which isn't very user-friendly. There's really no reason not to use the `asTargetString()` method instead. This PR just renames `asTargetString()` to `toString()` and updates all call sites.

Closes #24450.

PiperOrigin-RevId: 700050362
Change-Id: I82238d2134e1642694f0b20235fcfe9307ceaa7d
(cherry picked from commit d23421a)

Fix edge cases in lockfile handling

* Don't run the core logic when `--lockfile_mode` is `off` or `error` but the command doesn't forward options to Skyframe.
* Honor `reproducible` per extension eval factor, not per extension.
* Fix encoding conflict between isolation key and `use_repo_rule`'s fake extension names

This doesn't require a lockfile version bump as `use_repo_rule`'s fake extension (so far) isn't included in the lockfile.

Work towards #24723

Closes #24754.

PiperOrigin-RevId: 712623562
Change-Id: I61fd439539031a01ddec4488276ff2d0484849f2
(cherry picked from commit cfda178)

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

@@ -17,10 +17,14 @@
 import static com.google.devtools.build.lib.bazel.bzlmod.BazelLockFileFunction.LOCKFILE_MODE;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.collect.Maps;
+import com.google.common.collect.Sets;
 import com.google.common.flogger.GoogleLogger;
+import com.google.devtools.build.lib.bazel.repository.RepositoryOptions;
 import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.LockfileMode;
 import com.google.devtools.build.lib.cmdline.LabelConstants;
 import com.google.devtools.build.lib.runtime.BlazeModule;
@@ -36,6 +40,7 @@
 import java.util.HashMap;
 import java.util.Map;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.function.Predicate;
 
 /**
  * Module collecting Bazel module and module extensions resolution results and updating the
@@ -45,13 +50,18 @@ public class BazelLockFileModule extends BlazeModule {
 
   private SkyframeExecutor executor;
   private Path workspaceRoot;
+  private LockfileMode optionsLockfileMode;
 
   private static final GoogleLogger logger = GoogleLogger.forEnclosingClass();
 
+  private static final ImmutableSet<LockfileMode> ENABLED_IN_MODES =
+      Sets.immutableEnumSet(LockfileMode.UPDATE, LockfileMode.REFRESH);
+
   @Override
   public void beforeCommand(CommandEnvironment env) {
     executor = env.getSkyframeExecutor();
     workspaceRoot = env.getWorkspace();
+    optionsLockfileMode = env.getOptions().getOptions(RepositoryOptions.class).lockfileMode;
   }
 
   @Override
@@ -67,11 +77,12 @@ public void afterCommand() {
         // No command run on this server has triggered module resolution yet.
         return;
       }
-      LockfileMode lockfileMode = (LockfileMode) lockfileModeValue.get();
-      // Check the Skyframe value instead of the option since some commands (e.g. shutdown) don't
-      // propagate the options to Skyframe, but we can only operate on Skyframe values that were
-      // generated in UPDATE mode.
-      if (lockfileMode != LockfileMode.UPDATE && lockfileMode != LockfileMode.REFRESH) {
+      // Check the Skyframe value in addition to the option since some commands (e.g. shutdown)
+      // don't propagate the options to Skyframe, but we can only operate on Skyframe values that
+      // were generated in UPDATE mode.
+      LockfileMode skyframeLockfileMode = (LockfileMode) lockfileModeValue.get();
+      if (!(ENABLED_IN_MODES.contains(optionsLockfileMode)
+          && ENABLED_IN_MODES.contains(skyframeLockfileMode))) {
         return;
       }
       moduleResolutionValue =
@@ -111,7 +122,9 @@ public void afterCommand() {
             });
     var combinedExtensionInfos =
         combineModuleExtensions(
-            oldLockfile.getModuleExtensions(), newExtensionInfos, depGraphValue);
+            oldLockfile.getModuleExtensions(),
+            newExtensionInfos,
+            /* hasUsages= */ depGraphValue.getExtensionUsagesTable()::containsRow);
 
     // Create an updated version of the lockfile, keeping only the extension results from the old
     // lockfile that are still up-to-date and adding the newly resolved extension results.
@@ -136,15 +149,14 @@ public void afterCommand() {
    * Combines the old extensions stored in the lockfile -if they are still valid- with the new
    * extensions from the events (if any)
    */
-  private ImmutableMap<
+  @VisibleForTesting
+  static ImmutableMap<
           ModuleExtensionId, ImmutableMap<ModuleExtensionEvalFactors, LockFileModuleExtension>>
       combineModuleExtensions(
-          ImmutableMap<
-                  ModuleExtensionId,
-                  ImmutableMap<ModuleExtensionEvalFactors, LockFileModuleExtension>>
+          Map<ModuleExtensionId, ImmutableMap<ModuleExtensionEvalFactors, LockFileModuleExtension>>
               oldExtensionInfos,
           Map<ModuleExtensionId, LockFileModuleExtension.WithFactors> newExtensionInfos,
-          BazelDepGraphValue depGraphValue) {
+          Predicate<ModuleExtensionId> hasUsages) {
     Map<ModuleExtensionId, ImmutableMap<ModuleExtensionEvalFactors, LockFileModuleExtension>>
         updatedExtensionMap = new HashMap<>();
 
@@ -153,7 +165,7 @@ public void afterCommand() {
     // Other information such as transitive .bzl hash and usages hash are *not* checked here.
     for (var entry : oldExtensionInfos.entrySet()) {
       var moduleExtensionId = entry.getKey();
-      if (!depGraphValue.getExtensionUsagesTable().containsRow(moduleExtensionId)) {
+      if (!hasUsages.test(moduleExtensionId)) {
         // Extensions without any usages are not needed anymore.
         continue;
       }
@@ -163,15 +175,17 @@ public void afterCommand() {
         updatedExtensionMap.put(moduleExtensionId, entry.getValue());
         continue;
       }
-      if (!newExtensionInfo.moduleExtension().shouldLockExtension()) {
-        // Extension has become reproducible and should not be locked anymore.
-        continue;
-      }
       var newFactors = newExtensionInfo.extensionFactors();
+      // Factor results can be individually marked as reproducible and should be dropped if so.
       ImmutableSortedMap<ModuleExtensionEvalFactors, LockFileModuleExtension>
           perFactorResultsToKeep =
               ImmutableSortedMap.copyOf(
-                  Maps.filterKeys(entry.getValue(), newFactors::hasSameDependenciesAs));
+                  Maps.filterKeys(
+                      entry.getValue(),
+                      existingFactors ->
+                          newFactors.hasSameDependenciesAs(existingFactors)
+                              && !(newFactors.equals(existingFactors)
+                                  && newExtensionInfo.moduleExtension().isReproducible())));
       if (perFactorResultsToKeep.isEmpty()) {
         continue;
       }
@@ -181,7 +195,7 @@ public void afterCommand() {
     // Add the new resolved extensions
     for (var extensionIdAndInfo : newExtensionInfos.entrySet()) {
       LockFileModuleExtension extension = extensionIdAndInfo.getValue().moduleExtension();
-      if (!extension.shouldLockExtension()) {
+      if (extension.isReproducible()) {
         continue;
       }
 

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

@@ -18,7 +18,6 @@
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
 
 import com.google.common.base.Preconditions;
-import com.google.common.base.Splitter;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -38,7 +37,6 @@
 import com.google.devtools.build.lib.skyframe.BzlLoadFunction;
 import com.google.devtools.build.lib.skyframe.BzlLoadValue;
 import com.google.devtools.build.skyframe.SkyFunction.Environment;
-import java.util.Iterator;
 import java.util.Map.Entry;
 import java.util.Optional;
 import javax.annotation.Nullable;
@@ -104,18 +102,20 @@ static InnateRunnableExtension load(
     RepositoryMapping repoMapping = usagesValue.getRepoMappings().get(moduleKey);
     Label.RepoContext repoContext = Label.RepoContext.of(repoMapping.ownerRepo(), repoMapping);
 
-    // The name of the extension is of the form "<bzl_file_label>%<rule_name>".
-    Iterator<String> parts = Splitter.on('%').split(extensionId.extensionName()).iterator();
+    // The name of the extension is of the form "<bzl_file_label> <rule_name>". Rule names cannot
+    // contain spaces, so we can split on the last space.
+    int lastSpace = extensionId.extensionName().lastIndexOf(' ');
+    String rawLabel = extensionId.extensionName().substring(0, lastSpace);
+    String ruleName = extensionId.extensionName().substring(lastSpace + 1);
     Location location = tags.getFirst().getLocation();
     Label bzlLabel;
     try {
-      bzlLabel = Label.parseWithRepoContext(parts.next(), repoContext);
+      bzlLabel = Label.parseWithRepoContext(rawLabel, repoContext);
       BzlLoadFunction.checkValidLoadLabel(bzlLabel, starlarkSemantics);
     } catch (LabelSyntaxException e) {
       throw ExternalDepsException.withCauseAndMessage(
           Code.BAD_MODULE, e, "bad repo rule .bzl file label at %s", location);
     }
-    String ruleName = parts.next();
 
     // Load the .bzl file.
     BzlLoadValue loadedBzl;

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

@@ -58,9 +58,8 @@ public static Builder builder() {
   public abstract ImmutableTable<RepositoryName, String, RepositoryName>
       getRecordedRepoMappingEntries();
 
-  public boolean shouldLockExtension() {
-    return getModuleExtensionMetadata().isEmpty()
-        || !getModuleExtensionMetadata().get().getReproducible();
+  public boolean isReproducible() {
+    return getModuleExtensionMetadata().map(ModuleExtensionMetadata::getReproducible).orElse(false);
   }
 
   /** Builder type for {@link LockFileModuleExtension}. */

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

@@ -172,7 +172,7 @@ public ImmutableMap<String, RepoSpec> createRepos(StarlarkSemantics starlarkSema
         var attributesValue = AttributeValues.create(attributes);
         AttributeValues.validateAttrs(
             attributesValue,
-            String.format("in the extension '%s'", extensionId.asTargetString()),
+            String.format("in the extension '%s'", extensionId),
             String.format("%s '%s'", rule.getRuleClass(), name));
         repoSpecs.put(
             name,

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

@@ -24,19 +24,7 @@ public class ModuleExtensionEvaluationProgress implements FetchProgress {
 
   /** Returns the unique identifying string for a module extension evaluation event. */
   public static String moduleExtensionEvaluationContextString(ModuleExtensionId extensionId) {
-    String suffix =
-        extensionId
-            .isolationKey()
-            .map(
-                isolationKey ->
-                    String.format(
-                        " for %s in %s", isolationKey.usageExportedName(), isolationKey.module()))
-            .orElse("");
-    return String.format(
-        "module extension %s in %s%s",
-        extensionId.extensionName(),
-        extensionId.bzlFileLabel().getUnambiguousCanonicalForm(),
-        suffix);
+    return "module extension " + extensionId;
   }
 
   public static ModuleExtensionEvaluationProgress ongoing(

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

@@ -63,7 +63,7 @@ public static IsolationKey create(ModuleKey module, String usageExportedName) {
     }
 
     @Override
-    public final String toString() {
+    public String toString() {
       return module() + "+" + usageExportedName();
     }
 
@@ -79,11 +79,12 @@ public static ModuleExtensionId create(
     return new ModuleExtensionId(bzlFileLabel, extensionName, isolationKey);
   }
 
-  public final boolean isInnate() {
-    return extensionName().contains("%");
+  public boolean isInnate() {
+    return extensionName().contains(" ");
   }
 
-  public String asTargetString() {
+  @Override
+  public String toString() {
     String isolationKeyPart = isolationKey().map(key -> "%" + key).orElse("");
     return String.format(
         "%s%%%s%s",

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

@@ -563,7 +563,7 @@ private static void injectRepos(
         new ModuleThreadContext.ModuleExtensionUsageBuilder(
             context,
             "//:MODULE.bazel",
-            "@bazel_tools//tools/build_defs/repo:local.bzl%local_repository",
+            "@bazel_tools//tools/build_defs/repo:local.bzl local_repository",
             /* isolate= */ false);
     ModuleFileGlobals.ModuleExtensionProxy extensionProxy =
         new ModuleFileGlobals.ModuleExtensionProxy(

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

@@ -785,7 +785,7 @@ public RepoRuleProxy useRepoRule(String bzlFile, String ruleName, StarlarkThread
     ModuleThreadContext context = ModuleThreadContext.fromOrFail(thread, "use_repo_rule()");
     context.setNonModuleCalled();
     // Not a valid Starlark identifier so that it can't collide with a real extension.
-    String extensionName = bzlFile + '%' + ruleName;
+    String extensionName = bzlFile + ' ' + ruleName;
     // Find or create the builder for the singular "innate" extension of this repo rule for this
     // module.
     for (ModuleExtensionUsageBuilder usageBuilder : context.getExtensionUsageBuilders()) {

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

@@ -225,7 +225,7 @@ public RunModuleExtensionResult run(
       try {
         return state.startOrContinueWork(
             env,
-            "module-extension-" + extensionId.asTargetString(),
+            "module-extension-" + extensionId,
             (workerEnv) ->
                 runInternal(
                     workerEnv, usagesValue, starlarkSemantics, extensionId, mainRepositoryMapping));
@@ -281,19 +281,15 @@ private RunModuleExtensionResult runInternal(
       thread.setThreadLocal(Label.RepoMappingRecorder.class, repoMappingRecorder);
       try (SilentCloseable c =
           Profiler.instance()
-              .profile(
-                  ProfilerTask.BZLMOD,
-                  () -> "evaluate module extension: " + extensionId.asTargetString())) {
+              .profile(ProfilerTask.BZLMOD, () -> "evaluate module extension: " + extensionId)) {
         Object returnValue =
             Starlark.fastcall(
                 thread, extension.implementation(), new Object[] {moduleContext}, new Object[0]);
         if (returnValue != Starlark.NONE && !(returnValue instanceof ModuleExtensionMetadata)) {
           throw ExternalDepsException.withMessage(
               ExternalDeps.Code.BAD_MODULE,
-              "expected module extension %s in %s to return None or extension_metadata, got"
-                  + " %s",
-              extensionId.extensionName(),
-              extensionId.bzlFileLabel(),
+              "expected module extension %s to return None or extension_metadata, got %s",
+              extensionId,
               Starlark.type(returnValue));
         }
         if (returnValue instanceof ModuleExtensionMetadata retMetadata) {
@@ -317,10 +313,7 @@ private RunModuleExtensionResult runInternal(
     } catch (EvalException e) {
       env.getListener().handle(Event.error(e.getInnermostLocation(), e.getMessageWithStack()));
       throw ExternalDepsException.withMessage(
-          ExternalDeps.Code.BAD_MODULE,
-          "error evaluating module extension %s in %s",
-          extensionId.extensionName(),
-          extensionId.bzlFileLabel());
+          ExternalDeps.Code.BAD_MODULE, "error evaluating module extension %s", extensionId);
     } catch (IOException e) {
       throw ExternalDepsException.withCauseAndMessage(
           ExternalDeps.Code.EXTERNAL_DEPS_UNKNOWN,

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

@@ -199,7 +199,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
                     String.format(
                         "The module extension %s produced an invalid lockfile entry because it"
                             + " referenced %s. Please report this issue to its maintainers.",
-                        extensionId.asTargetString(), nonVisibleRepoNames)));
+                        extensionId, nonVisibleRepoNames)));
       }
     }
     if (lockfileMode.equals(LockfileMode.ERROR)) {

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

@@ -61,10 +61,9 @@ public SkyValue compute(SkyKey skyKey, Environment env)
             throw new SingleExtensionFunctionException(
                 ExternalDepsException.withMessage(
                     Code.INVALID_EXTENSION_IMPORT,
-                    "module extension \"%s\" from \"%s\" does not generate repository \"%s\", yet"
+                    "module extension %s does not generate repository \"%s\", yet"
                         + " it is imported as \"%s\" in the usage at %s%s",
-                    extensionId.extensionName(),
-                    extensionId.bzlFileLabel(),
+                    extensionId,
                     repoImport.getValue(),
                     repoImport.getKey(),
                     proxy.getLocation(),
@@ -84,23 +83,21 @@ public SkyValue compute(SkyKey skyKey, Environment env)
           throw new SingleExtensionFunctionException(
               ExternalDepsException.withMessage(
                   Code.INVALID_EXTENSION_IMPORT,
-                  "module extension \"%s\" from \"%s\" generates repository \"%s\", yet"
+                  "module extension %s generates repository \"%s\", yet"
                       + " it is injected via inject_repo() at %s. Use override_repo() instead to"
                       + " override an existing repository.",
-                  extensionId.extensionName(),
-                  extensionId.bzlFileLabel(),
+                  extensionId,
                   override.getKey(),
                   override.getValue().location()),
               Transience.PERSISTENT);
         } else if (!repoExists && override.getValue().mustExist()) {
           throw new SingleExtensionFunctionException(
               ExternalDepsException.withMessage(
                   Code.INVALID_EXTENSION_IMPORT,
-                  "module extension \"%s\" from \"%s\" does not generate repository \"%s\", yet"
+                  "module extension %s does not generate repository \"%s\", yet"
                       + " it is overridden via override_repo() at %s. Use inject_repo() instead to"
                       + " inject a new repository.",
-                  extensionId.extensionName(),
-                  extensionId.bzlFileLabel(),
+                  extensionId,
                   override.getKey(),
                   override.getValue().location()),
               Transience.PERSISTENT);

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

@@ -120,7 +120,7 @@ private String toId(ModuleKey key) {
   }
 
   private String toId(ModuleExtensionId id) {
-    return id.asTargetString();
+    return id.toString();
   }
 
   private String toId(ModuleExtensionId id, String repo) {

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

@@ -60,7 +60,7 @@ public String printKey(ModuleKey key) {
   private JsonObject printExtension(
       ModuleKey key, ModuleExtensionId extensionId, boolean unexpanded) {
     JsonObject json = new JsonObject();
-    json.addProperty("key", extensionId.asTargetString());
+    json.addProperty("key", extensionId.toString());
     json.addProperty("unexpanded", unexpanded);
     if (options.extensionInfo == ExtensionShow.USAGES) {
       return json;

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

@@ -467,13 +467,12 @@ private boolean filterBuiltin(ModuleKey key) {
   /** Helper to display show_extension info. */
   private void displayExtension(ModuleExtensionId extension, ImmutableSet<ModuleKey> fromUsages)
       throws InvalidArgumentException {
-    printer.printf("## %s:\n", extension.asTargetString());
+    printer.printf("## %s:\n", extension.toString());
     printer.println();
     printer.println("Fetched repositories:");
     if (!extensionRepoImports.containsKey(extension)) {
       throw new InvalidArgumentException(
-          String.format(
-              "No extension %s exists in the dependency graph", extension.asTargetString()));
+          String.format("No extension %s exists in the dependency graph", extension));
     }
     ImmutableSortedSet<String> usedRepos =
         ImmutableSortedSet.copyOf(extensionRepoImports.get(extension).keySet());

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

@@ -96,7 +96,7 @@ private void printExtension(
       ModuleKey key, ModuleExtensionId extensionId, boolean unexpanded, int depth) {
     printTreeDrawing(IsIndirect.FALSE, depth);
     str.append('$');
-    str.append(extensionId.asTargetString());
+    str.append(extensionId);
     str.append(' ');
     if (unexpanded && options.extensionInfo == ExtensionShow.ALL) {
       str.append("... ");