[7.4.0] Let repo rule attributes reference extension apparent names (b…

…azelbuild#23585)

Fixes bazelbuild#19055

RELNOTES: Repository rules instantiated in the same module extensions
can now refer to each other by their extension-specified names in label
attributes.

Closes bazelbuild#23369.

PiperOrigin-RevId: 666893202
Change-Id: Ib2eaa55fcb563adc86e16dc4a357ac808228ebda

---------

Co-authored-by: Fabian Meumertzheim <[email protected]>

site/en/external/extension.md

@@ -167,6 +167,14 @@ several repo visibility rules:
         the apparent name `foo`, and the extension generates a repo with the
         specified name `foo`, then for all repos generated by that extension
         `foo` refers to the former.
+*   Similarly, in a module extension's implementation function, repos created
+    by the extension can refer to each other by their apparent names in
+    attributes, regardless of the order in which they are created.
+    *   In case of a conflict with a repository visible to the module, labels
+        passed to repository rule attributes can be wrapped in a call to
+        [`Label`](/rules/lib/toplevel/attr#label) to ensure that they refer to
+        the repo visible to the module instead of the extension-generated repo
+        of the same name.
 
 ## Best practices
 

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

@@ -45,32 +45,25 @@ public static AttributeValues create(Map<String, Object> attribs) {
 
   public abstract Dict<String, Object> attributes();
 
-  public static void validateAttrs(AttributeValues attributes, String what) throws EvalException {
+  public static void validateAttrs(AttributeValues attributes, String where, String what)
+      throws EvalException {
     for (var entry : attributes.attributes().entrySet()) {
-      validateSingleAttr(entry.getKey(), entry.getValue(), what);
+      validateSingleAttr(entry.getKey(), entry.getValue(), where, what);
     }
   }
 
-  public static void validateSingleAttr(String attrName, Object attrValue, String what)
-      throws EvalException {
+  public static void validateSingleAttr(
+      String attrName, Object attrValue, String where, String what) throws EvalException {
     var maybeNonVisibleLabel = getFirstNonVisibleLabel(attrValue);
     if (maybeNonVisibleLabel.isEmpty()) {
       return;
     }
     Label label = maybeNonVisibleLabel.get();
     String repoName = label.getRepository().getName();
     throw Starlark.errorf(
-        "no repository visible as '@%s' to the %s, but referenced by label '@%s//%s:%s' in"
-            + " attribute '%s' of %s. Is the %s missing a bazel_dep or use_repo(..., \"%s\")?",
-        repoName,
-        label.getRepository().getOwnerRepoDisplayString(),
-        repoName,
-        label.getPackageName(),
-        label.getName(),
-        attrName,
-        what,
-        label.getRepository().getOwnerModuleDisplayString(),
-        repoName);
+        "no repository visible as '@%s' %s, but referenced by label '@%s//%s:%s' in"
+            + " attribute '%s' of %s.",
+        repoName, where, repoName, label.getPackageName(), label.getName(), attrName, what);
   }
 
   private static Optional<Label> getFirstNonVisibleLabel(Object nativeAttrValue) {

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

@@ -239,7 +239,9 @@ public RunModuleExtensionResult run(
                     Maps.transformEntries(kwargs, (k, v) -> ruleInstance.getAttr(k)),
                     k -> !k.equals("name")));
         AttributeValues.validateAttrs(
-            attributesValue, String.format("%s '%s'", ruleInstance.getRuleClass(), name));
+            attributesValue,
+            String.format("to the %s", moduleKey.toDisplayString()),
+            String.format("%s '%s'", ruleInstance.getRuleClass(), name));
       } catch (InvalidRuleException | NoSuchPackageException | EvalException e) {
         throw ExternalDepsException.withCauseAndMessage(
             Code.BAD_MODULE,

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

@@ -15,10 +15,11 @@
 
 package com.google.devtools.build.lib.bazel.bzlmod;
 
-import com.google.auto.value.AutoValue;
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.analysis.BlazeDirectories;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
@@ -28,11 +29,15 @@
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.packages.RuleFactory.InvalidRuleException;
 import com.google.devtools.build.lib.packages.StarlarkNativeModule.ExistingRulesShouldBeNoOp;
-import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.Map;
 import net.starlark.java.eval.Dict;
 import net.starlark.java.eval.EvalException;
+import net.starlark.java.eval.NoneType;
 import net.starlark.java.eval.Starlark;
+import net.starlark.java.eval.StarlarkInt;
+import net.starlark.java.eval.StarlarkList;
+import net.starlark.java.eval.StarlarkSemantics;
 import net.starlark.java.eval.StarlarkThread;
 import net.starlark.java.syntax.Location;
 
@@ -51,93 +56,162 @@ public static ModuleExtensionEvalStarlarkThreadContext from(StarlarkThread threa
     return thread.getThreadLocal(ModuleExtensionEvalStarlarkThreadContext.class);
   }
 
-  @AutoValue
-  abstract static class RepoSpecAndLocation {
-    abstract RepoSpec getRepoSpec();
-
-    abstract Location getLocation();
-
-    static RepoSpecAndLocation create(RepoSpec repoSpec, Location location) {
-      return new AutoValue_ModuleExtensionEvalStarlarkThreadContext_RepoSpecAndLocation(
-          repoSpec, location);
-    }
-  }
+  record RepoRuleCall(
+      RuleClass ruleClass,
+      Dict<String, Object> kwargs,
+      Location location,
+      ImmutableList<StarlarkThread.CallStackEntry> callStack) {}
 
+  private final ModuleExtensionId extensionId;
   private final String repoPrefix;
   private final PackageIdentifier basePackageId;
-  private final RepositoryMapping repoMapping;
+  private final RepositoryMapping baseRepoMapping;
   private final BlazeDirectories directories;
   private final ExtendedEventHandler eventHandler;
-  private final Map<String, RepoSpecAndLocation> generatedRepos = new HashMap<>();
+  private final Map<String, RepoRuleCall> deferredRepos = new LinkedHashMap<>();
 
   public ModuleExtensionEvalStarlarkThreadContext(
+      ModuleExtensionId extensionId,
       String repoPrefix,
       PackageIdentifier basePackageId,
-      RepositoryMapping repoMapping,
+      RepositoryMapping baseRepoMapping,
       BlazeDirectories directories,
       ExtendedEventHandler eventHandler) {
+    this.extensionId = extensionId;
     this.repoPrefix = repoPrefix;
     this.basePackageId = basePackageId;
-    this.repoMapping = repoMapping;
+    this.baseRepoMapping = baseRepoMapping;
     this.directories = directories;
     this.eventHandler = eventHandler;
   }
 
-  public void createRepo(StarlarkThread thread, Dict<String, Object> kwargs, RuleClass ruleClass)
-      throws InterruptedException, EvalException {
+  /**
+   * Records a call to a repo rule that should be created at the end of the module extension
+   * evaluation.
+   */
+  @SuppressWarnings("unchecked")
+  public void lazilyCreateRepo(
+      StarlarkThread thread, Dict<String, Object> kwargs, RuleClass ruleClass)
+      throws EvalException {
     Object nameValue = kwargs.getOrDefault("name", Starlark.NONE);
     if (!(nameValue instanceof String)) {
       throw Starlark.errorf(
           "expected string for attribute 'name', got '%s'", Starlark.type(nameValue));
     }
     String name = (String) nameValue;
     RepositoryName.validateUserProvidedRepoName(name);
-    RepoSpecAndLocation conflict = generatedRepos.get(name);
+    RepoRuleCall conflict = deferredRepos.get(name);
     if (conflict != null) {
       throw Starlark.errorf(
           "A repo named %s is already generated by this module extension at %s",
-          name, conflict.getLocation());
+          name, conflict.location());
     }
-    String prefixedName = repoPrefix + name;
-    try {
-      Rule rule =
-          BzlmodRepoRuleCreator.createRule(
-              basePackageId,
-              repoMapping,
-              directories,
-              thread.getSemantics(),
-              eventHandler,
-              thread.getCallStack(),
-              ruleClass,
-              Maps.transformEntries(kwargs, (k, v) -> k.equals("name") ? prefixedName : v));
+    deferredRepos.put(
+        name,
+        new RepoRuleCall(
+            ruleClass,
+            // The extension may mutate the values of the kwargs after this function returns.
+            (Dict<String, Object>) deepCloneAttrValue(kwargs),
+            thread.getCallerLocation(),
+            thread.getCallStack()));
+  }
 
-      Map<String, Object> attributes =
-          Maps.filterKeys(
-              Maps.transformEntries(kwargs, (k, v) -> rule.getAttr(k)), k -> !k.equals("name"));
-      String bzlFile = ruleClass.getRuleDefinitionEnvironmentLabel().getUnambiguousCanonicalForm();
-      var attributesValue = AttributeValues.create(attributes);
-      AttributeValues.validateAttrs(
-          attributesValue, String.format("%s '%s'", rule.getRuleClass(), name));
-      RepoSpec repoSpec =
-          RepoSpec.builder()
-              .setBzlFile(bzlFile)
-              .setRuleClassName(ruleClass.getName())
-              .setAttributes(attributesValue)
-              .build();
+  /**
+   * Evaluates the repo rule calls recorded by {@link #lazilyCreateRepo} and returns all repos
+   * generated by the extension. The key is the "internal name" (as specified by the extension) of
+   * the repo, and the value is the {@link RepoSpec}.
+   */
+  public ImmutableMap<String, RepoSpec> createRepos(StarlarkSemantics starlarkSemantics)
+      throws EvalException, InterruptedException {
+    // LINT.IfChange
+    // Make it possible to refer to extension repos in the label attributes of another extension
+    // repo. Wrapping a label in Label(...) ensures that it is evaluated with respect to the
+    // containing module's repo mapping instead.
+    var extensionRepos =
+        Maps.asMap(
+            deferredRepos.keySet(),
+            apparentName -> RepositoryName.createUnvalidated(repoPrefix + apparentName));
+    RepositoryMapping fullRepoMapping =
+        RepositoryMapping.create(extensionRepos, baseRepoMapping.ownerRepo())
+            .withAdditionalMappings(baseRepoMapping);
+    // LINT.ThenChange(//src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionRepoMappingEntriesFunction.java)
+
+    ImmutableMap.Builder<String, RepoSpec> repoSpecs = ImmutableMap.builder();
+    for (var entry : deferredRepos.entrySet()) {
+      String name = entry.getKey();
+      RepoRuleCall repoRuleCall = entry.getValue();
+      try {
+        String prefixedName = repoPrefix + name;
+        Rule rule =
+            BzlmodRepoRuleCreator.createRule(
+                basePackageId,
+                fullRepoMapping,
+                directories,
+                starlarkSemantics,
+                eventHandler,
+                repoRuleCall.callStack,
+                repoRuleCall.ruleClass,
+                Maps.transformEntries(
+                    repoRuleCall.kwargs, (k, v) -> k.equals("name") ? prefixedName : v));
 
-      generatedRepos.put(name, RepoSpecAndLocation.create(repoSpec, thread.getCallerLocation()));
-    } catch (InvalidRuleException | NoSuchPackageException e) {
-      throw Starlark.errorf("%s", e.getMessage());
+        Map<String, Object> attributes =
+            Maps.filterKeys(
+                Maps.transformEntries(repoRuleCall.kwargs, (k, v) -> rule.getAttr(k)),
+                k -> !k.equals("name"));
+        String bzlFile =
+            repoRuleCall
+                .ruleClass
+                .getRuleDefinitionEnvironmentLabel()
+                .getUnambiguousCanonicalForm();
+        var attributesValue = AttributeValues.create(attributes);
+        AttributeValues.validateAttrs(
+            attributesValue,
+            String.format("in the extension '%s'", extensionId.asTargetString()),
+            String.format("%s '%s'", rule.getRuleClass(), name));
+        RepoSpec repoSpec =
+            RepoSpec.builder()
+                .setBzlFile(bzlFile)
+                .setRuleClassName(repoRuleCall.ruleClass.getName())
+                .setAttributes(attributesValue)
+                .build();
+        repoSpecs.put(name, repoSpec);
+      } catch (EvalException e) {
+        throw e.withCallStack(repoRuleCall.callStack);
+      } catch (InvalidRuleException | NoSuchPackageException e) {
+        throw new EvalException(e).withCallStack(repoRuleCall.callStack);
+      }
     }
+    return repoSpecs.buildOrThrow();
   }
 
   /**
-   * Returns the repos generated by the extension so far. The key is the "internal name" (as
-   * specified by the extension) of the repo, and the value is the package containing (only) the
-   * repo rule.
+   * Deep-clones a potentially mutable Starlark object that is a valid repo rule attribute.
+   * Immutable (sub-)objects are not cloned.
    */
-  public ImmutableMap<String, RepoSpec> getGeneratedRepoSpecs() {
-    return ImmutableMap.copyOf(
-        Maps.transformValues(generatedRepos, RepoSpecAndLocation::getRepoSpec));
+  private static Object deepCloneAttrValue(Object x) throws EvalException {
+    if (x instanceof NoneType
+        || x instanceof Boolean
+        || x instanceof StarlarkInt
+        || x instanceof String
+        || x instanceof Label) {
+      return x;
+    }
+    // Mutable Starlark values have to be cloned deeply.
+    if (x instanceof Dict<?, ?> dict) {
+      Dict.Builder<Object, Object> newDict = Dict.builder();
+      for (Map.Entry<?, ?> e : dict.entrySet()) {
+        newDict.put(deepCloneAttrValue(e.getKey()), deepCloneAttrValue(e.getValue()));
+      }
+      return newDict.buildImmutable();
+    }
+    if (x instanceof Iterable<?> iterable) {
+      ImmutableList.Builder<Object> newList = ImmutableList.builder();
+      for (Object item : iterable) {
+        newList.add(deepCloneAttrValue(item));
+      }
+      return StarlarkList.immutableCopyOf(newList.build());
+    }
+    throw Starlark.errorf(
+        "unexpected Starlark value: %s (of type %s)", Starlark.repr(x), Starlark.type(x));
   }
 }

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

@@ -70,9 +70,11 @@ private ModuleExtensionRepoMappingEntriesValue computeRepoMappingEntries(
     // extension generates an internal repo name "bar", then within a repo generated by the
     // extension, "bar" will refer to the latter. We should explore a way to differentiate between
     // the two to avoid any surprises.
+    // LINT.IfChange
     ImmutableMap.Builder<String, RepositoryName> entries = ImmutableMap.builder();
     entries.putAll(bazelDepGraphValue.getFullRepoMapping(moduleKey).entries());
     entries.putAll(extensionEvalValue.getCanonicalRepoNameToInternalNames().inverse());
     return ModuleExtensionRepoMappingEntriesValue.create(entries.buildKeepingLast(), moduleKey);
+    // LINT.ThenChange(//src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionEvalStarlarkThreadContext.java)
   }
 }

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

@@ -201,7 +201,7 @@ public SkyValue compute(SkyKey skyKey, Environment env)
     try {
       module = moduleThreadContext.buildModule(getModuleFileResult.registry);
     } catch (EvalException e) {
-      env.getListener().handle(Event.error(e.getMessageWithStack()));
+      env.getListener().handle(Event.error(e.getInnermostLocation(), e.getMessageWithStack()));
       throw errorf(Code.BAD_MODULE, "error executing MODULE.bazel file for %s", moduleKey);
     }
     if (!module.getName().equals(moduleKey.getName())) {
@@ -478,10 +478,7 @@ public static RootModuleFileValue evaluateRootModuleFile(
                     .map(label -> Label.parseCanonicalUnchecked(label).toPathFragment()))
             .collect(toImmutableSet());
     return RootModuleFileValue.create(
-        module,
-        overrides,
-        nonRegistryOverrideCanonicalRepoNameLookup,
-        moduleFilePaths);
+        module, overrides, nonRegistryOverrideCanonicalRepoNameLookup, moduleFilePaths);
   }
 
   private static ModuleThreadContext execModuleFile(

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

@@ -76,6 +76,14 @@ public final String toString() {
     return getName() + "@" + (getVersion().isEmpty() ? "_" : getVersion().toString());
   }
 
+  /** Returns a string such as "root module" or "module [email protected]" for display purposes. */
+  public final String toDisplayString() {
+    if (this.equals(ROOT)) {
+      return "root module";
+    }
+    return String.format("module '%s'", this);
+  }
+
   /**
    * Returns the canonical name of the repo backing this module, including its version. This name is
    * always guaranteed to be unique.

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

@@ -259,6 +259,7 @@ private RunModuleExtensionResult runInternal(
             : '~';
     ModuleExtensionEvalStarlarkThreadContext threadContext =
         new ModuleExtensionEvalStarlarkThreadContext(
+            extensionId,
             usagesValue.getExtensionUniqueName() + separator,
             extensionId.getBzlFileLabel().getPackageIdentifier(),
             BazelModuleContext.of(bzlLoadValue.getModule()).repoMapping(),
@@ -315,11 +316,11 @@ private RunModuleExtensionResult runInternal(
           moduleContext.getRecordedFileInputs(),
           moduleContext.getRecordedDirentsInputs(),
           moduleContext.getRecordedEnvVarInputs(),
-          threadContext.getGeneratedRepoSpecs(),
+          threadContext.createRepos(starlarkSemantics),
           moduleExtensionMetadata,
           repoMappingRecorder.recordedEntries());
     } catch (EvalException e) {
-      env.getListener().handle(Event.error(e.getMessageWithStack()));
+      env.getListener().handle(Event.error(e.getInnermostLocation(), e.getMessageWithStack()));
       throw ExternalDepsException.withMessage(
           ExternalDeps.Code.BAD_MODULE,
           "error evaluating module extension %s in %s",

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

@@ -473,7 +473,7 @@ private SingleExtensionValue createSingleExtensionValue(
                     generatedRepoSpecs.keySet(),
                     env.getListener());
       } catch (EvalException e) {
-        env.getListener().handle(Event.error(e.getMessageWithStack()));
+        env.getListener().handle(Event.error(e.getInnermostLocation(), e.getMessageWithStack()));
         throw new SingleExtensionEvalFunctionException(
             ExternalDepsException.withMessage(
                 Code.BAD_MODULE,