Let repo rule attributes reference extension apparent names

site/en/external/extension.md

@@ -165,6 +165,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 repo generated by the
+        extension 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/ModuleExtensionEvalStarlarkThreadContext.java

@@ -15,7 +15,7 @@
 
 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;
@@ -29,13 +29,16 @@
 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 javax.annotation.Nullable;
 import net.starlark.java.eval.Dict;
 import net.starlark.java.eval.EvalException;
 import net.starlark.java.eval.Starlark;
+import net.starlark.java.eval.StarlarkList;
+import net.starlark.java.eval.StarlarkSemantics;
 import net.starlark.java.eval.StarlarkThread;
+import net.starlark.java.eval.StarlarkValue;
 import net.starlark.java.syntax.Location;
 
 /**
@@ -56,94 +59,164 @@ public static ModuleExtensionEvalStarlarkThreadContext fromOrNull(StarlarkThread
     return ctx instanceof ModuleExtensionEvalStarlarkThreadContext c ? c : null;
   }
 
-  @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,
       RepositoryMapping mainRepoMapping,
       BlazeDirectories directories,
       ExtendedEventHandler eventHandler) {
     super(() -> mainRepoMapping);
+    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.
+   */
+  public void lazilyCreateRepo(
+      StarlarkThread thread, Dict<String, Object> kwargs, RuleClass ruleClass)
+      throws EvalException {
     Object nameValue = kwargs.getOrDefault("name", Starlark.NONE);
     if (!(nameValue instanceof String name)) {
       throw Starlark.errorf(
           "expected string for attribute 'name', got '%s'", Starlark.type(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>) deepCloneStarlarkObject(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 package containing (only) the repo rule.
+   */
+  public ImmutableMap<String, RepoSpec> createAndGetRepos(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.
+    ImmutableMap.Builder<String, RepositoryName> fullRepoMappingEntries = ImmutableMap.builder();
+    fullRepoMappingEntries.putAll(baseRepoMapping.entries());
+    fullRepoMappingEntries.putAll(
+        Maps.asMap(
+            deferredRepos.keySet(),
+            apparentName -> RepositoryName.createUnvalidated(repoPrefix + apparentName)));
+    RepositoryMapping fullRepoMapping =
+        RepositoryMapping.create(
+            fullRepoMappingEntries.buildKeepingLast(), baseRepoMapping.ownerRepo());
+    // LINT.ThenChange(//src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionRepoMappingEntriesFunction.java)
 
-      generatedRepos.put(name, RepoSpecAndLocation.create(repoSpec, thread.getCallerLocation()));
-    } catch (InvalidRuleException | NoSuchPackageException e) {
-      throw Starlark.errorf("%s", e.getMessage());
+    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));
+
+        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.
-   */
-  public ImmutableMap<String, RepoSpec> getGeneratedRepoSpecs() {
-    return ImmutableMap.copyOf(
-        Maps.transformValues(generatedRepos, RepoSpecAndLocation::getRepoSpec));
+  /** Deep-clones a potentially mutable Starlark object. Immutable (sub-)objects are not cloned. */
+  private static Object deepCloneStarlarkObject(Object x) {
+    if (x instanceof String || x instanceof Boolean) {
+      return x;
+    }
+    StarlarkValue value = (StarlarkValue) Starlark.checkValid(x);
+    if (value.isImmutable()) {
+      return value;
+    }
+
+    // Mutable Starlark values have to be cloned deeply.
+    if (value instanceof Dict<?, ?> dict) {
+      Dict.Builder<Object, Object> newDict = Dict.builder();
+      for (Map.Entry<?, ?> e : dict.entrySet()) {
+        newDict.put(deepCloneStarlarkObject(e.getKey()), deepCloneStarlarkObject(e.getValue()));
+      }
+      return newDict.buildImmutable();
+    }
+    if (value instanceof Iterable<?> iterable) {
+      ImmutableList.Builder<Object> newList = ImmutableList.builder();
+      for (Object item : iterable) {
+        newList.add(deepCloneStarlarkObject(item));
+      }
+      return StarlarkList.immutableCopyOf(newList.build());
+    }
+    throw new IllegalStateException(
+        String.format(
+            "unexpected mutable Starlark value: %s (of type %s)",
+            Starlark.repr(value), 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/ModuleKey.java

@@ -73,6 +73,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/SingleExtensionEvalFunction.java

@@ -569,7 +569,7 @@ RunModuleExtensionResult run(
         SingleExtensionUsagesValue usagesValue,
         StarlarkSemantics starlarkSemantics,
         ModuleExtensionId extensionId,
-        RepositoryMapping repositoryMapping)
+        RepositoryMapping mainRepositoryMapping)
         throws InterruptedException, SingleExtensionEvalFunctionException;
   }
 
@@ -786,7 +786,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 new SingleExtensionEvalFunctionException(
               ExternalDepsException.withCauseAndMessage(
@@ -907,6 +909,7 @@ public RunModuleExtensionResult run(
         throws InterruptedException, SingleExtensionEvalFunctionException {
       ModuleExtensionEvalStarlarkThreadContext threadContext =
           new ModuleExtensionEvalStarlarkThreadContext(
+              extensionId,
               usagesValue.getExtensionUniqueName() + "+",
               extensionId.getBzlFileLabel().getPackageIdentifier(),
               BazelModuleContext.of(bzlLoadValue.getModule()).repoMapping(),
@@ -967,11 +970,17 @@ public RunModuleExtensionResult run(
             moduleContext.getRecordedFileInputs(),
             moduleContext.getRecordedDirentsInputs(),
             moduleContext.getRecordedEnvVarInputs(),
-            threadContext.getGeneratedRepoSpecs(),
+            threadContext.createAndGetRepos(starlarkSemantics),
             moduleExtensionMetadata,
             repoMappingRecorder.recordedEntries());
       } catch (EvalException e) {
-        env.getListener().handle(Event.error(e.getMessageWithStack()));
+        Location innermostLocation =
+            e.getCallStack().reverse().stream()
+                .map(entry -> entry.location)
+                .filter(location -> location != Location.BUILTIN)
+                .findFirst()
+                .orElse(null);
+        env.getListener().handle(Event.error(innermostLocation, e.getMessageWithStack()));
         throw new SingleExtensionEvalFunctionException(
             ExternalDepsException.withMessage(
                 ExternalDeps.Code.BAD_MODULE,

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

@@ -135,7 +135,9 @@ public static StarlarkBazelModule create(
       // (for example, String to Label).
       typeCheckedTags
           .get(tag.getTagName())
-          .add(TypeCheckedTag.create(tagClass, tag, labelConverter));
+          .add(
+              TypeCheckedTag.create(
+                  tagClass, tag, labelConverter, module.getKey().toDisplayString()));
     }
     return new StarlarkBazelModule(
         module.getName(),