Track dev/non-dev use_extension calls

By always tracking whether a given extension usage by a module has
`use_extension` calls with and/or without `dev_dependency = True`
instead of just for isolated extension usages, we obtain the following
advantages:

* Module extensions can use `module_ctx.is_dev_dependency` to learn
  whether the root module contains only `use_extension` calls with
  `dev_dependency = True` for them. This is necessary to decide
  whether repositories that do not directly correspond to tags (e.g. hub
  repos) should be marked as dev or non-dev dependencies in
  `module_ctx.extension_metadata`.
* `ModuleExtensionMetadata` consistency checks of the type "no
  dev/non-dev imports without dev/non-dev usage" are generalized from
  isolated to all extensions.
* Prepares for the removal of `isDevUsage` from `IsolationKey` in a
  follow-up change which will instead use the exported name of the
  (only) usage proxy of an isolated usage as the key.

Author: fmeum

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

@@ -96,31 +96,44 @@ protected ImmutableMap<String, String> getRemoteExecProperties() throws EvalExce
       name = "modules",
       structField = true,
       doc =
-          "A list of all the Bazel modules in the external dependency graph, each of which is a <a"
-              + " href=\"../builtins/bazel_module.html\">bazel_module</a> object that exposes all"
-              + " the tags it specified for this module extension. The iteration order of this"
-              + " dictionary is guaranteed to be the same as breadth-first search starting from the"
-              + " root module.")
+          "A list of all the Bazel modules in the external dependency graph that use this module "
+              + "extension, each of which is a <a href=\"../builtins/bazel_module.html\">"
+              + "bazel_module</a> object that exposes all the tags it specified for this extension."
+              + " The iteration order of this dictionary is guaranteed to be the same as"
+              + " breadth-first search starting from the root module.")
   public StarlarkList<StarlarkBazelModule> getModules() {
     return modules;
   }
 
   @StarlarkMethod(
       name = "is_dev_dependency",
       doc =
-          "Returns whether the given tag was specified on the result of a <a "
+          "When given a tag, returns whether the given tag was specified on the result of a <a "
               + "href=\"../globals/module.html#use_extension\">use_extension</a> call with "
-              + "<code>devDependency = True</code>.",
+              + "<code>devDependency = True</code>.<p>When given a module, returns "
+              + "<code>True</code> if and only if all <a "
+              + "href=\"../globals/module.html#use_extension\">use_extension</a> calls made by that "
+              + "module for the current extension specify <code>devDependency = True</code>. In "
+              + "particular, it returns <code>False</code> for modules other than the root module.",
       parameters = {
         @Param(
-            name = "tag",
+            name = "tag_or_module",
             doc =
-                "A tag obtained from <a"
-                    + " href=\"../builtins/bazel_module.html#tags\">bazel_module.tags</a>.",
-            allowedTypes = {@ParamType(type = TypeCheckedTag.class)})
+                "A tag obtained from the <a"
+                    + " href=\"../builtins/bazel_module.html#tags\">tags</a> property of a "
+                    + "<a href=\"../builtins/bazel_module.html\">bazel_module</a> or a "
+                    + "<a href=\"../builtins/bazel_module.html\">bazel_module</a>.",
+            allowedTypes = {
+              @ParamType(type = TypeCheckedTag.class),
+              @ParamType(type = StarlarkBazelModule.class)
+            })
       })
-  public boolean isDevDependency(TypeCheckedTag tag) {
-    return tag.isDevDependency();
+  public boolean isDevDependency(Object tagOrModule) {
+    if (tagOrModule instanceof TypeCheckedTag) {
+      return ((TypeCheckedTag) tagOrModule).isDevDependency();
+    } else {
+      return ((StarlarkBazelModule) tagOrModule).hasDevUseExtensionOnly();
+    }
   }
 
   @StarlarkMethod(
@@ -154,7 +167,7 @@ public boolean isIsolated() {
                     + " disjoint.<p>Exactly one of <code>root_module_direct_deps</code> and"
                     + " <code>root_module_direct_dev_deps</code> can be set to the special value"
                     + " <code>\"all\"</code>, which is treated as if a list with the names of"
-                    + " allrepositories generated by the extension was specified as the value.",
+                    + " all repositories generated by the extension was specified as the value.",
             positional = false,
             named = true,
             defaultValue = "None",
@@ -180,7 +193,7 @@ public boolean isIsolated() {
                     + " disjoint.<p>Exactly one of <code>root_module_direct_deps</code> and"
                     + " <code>root_module_direct_dev_deps</code> can be set to the special value"
                     + " <code>\"all\"</code>, which is treated as if a list with the names of"
-                    + " allrepositories generated by the extension was specified as the value.",
+                    + " all repositories generated by the extension was specified as the value.",
             positional = false,
             named = true,
             defaultValue = "None",

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

@@ -20,14 +20,14 @@
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableSortedSet;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Sets;
 import com.google.devtools.build.docgen.annot.DocCategory;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
 import com.google.devtools.build.lib.events.Event;
 import com.google.devtools.build.lib.events.EventHandler;
 import java.util.Collection;
 import java.util.LinkedHashSet;
-import java.util.List;
 import java.util.Optional;
 import java.util.Set;
 import java.util.stream.Collectors;
@@ -82,23 +82,11 @@ static ModuleExtensionMetadata create(
     // root_module_direct_dev_deps = [], but not root_module_direct_dev_deps = ["some_repo"].
     if (rootModuleDirectDepsUnchecked.equals("all")
         && rootModuleDirectDevDepsUnchecked.equals(StarlarkList.immutableOf())) {
-      if (extensionId.getIsolationKey().isPresent()
-          && extensionId.getIsolationKey().get().isDevUsage()) {
-        throw Starlark.errorf(
-            "root_module_direct_deps must be empty for an isolated extension usage with "
-                + "dev_dependency = True");
-      }
       return new ModuleExtensionMetadata(null, null, UseAllRepos.REGULAR);
     }
 
     if (rootModuleDirectDevDepsUnchecked.equals("all")
         && rootModuleDirectDepsUnchecked.equals(StarlarkList.immutableOf())) {
-      if (extensionId.getIsolationKey().isPresent()
-          && !extensionId.getIsolationKey().get().isDevUsage()) {
-        throw Starlark.errorf(
-            "root_module_direct_dev_deps must be empty for an isolated extension usage with "
-                + "dev_dependency = False");
-      }
       return new ModuleExtensionMetadata(null, null, UseAllRepos.DEV);
     }
 
@@ -128,20 +116,6 @@ static ModuleExtensionMetadata create(
         Sequence.cast(
             rootModuleDirectDevDepsUnchecked, String.class, "root_module_direct_dev_deps");
 
-    if (extensionId.getIsolationKey().isPresent()) {
-      ModuleExtensionId.IsolationKey isolationKey = extensionId.getIsolationKey().get();
-      if (isolationKey.isDevUsage() && !rootModuleDirectDeps.isEmpty()) {
-        throw Starlark.errorf(
-            "root_module_direct_deps must be empty for an isolated extension usage with "
-                + "dev_dependency = True");
-      }
-      if (!isolationKey.isDevUsage() && !rootModuleDirectDevDeps.isEmpty()) {
-        throw Starlark.errorf(
-            "root_module_direct_dev_deps must be empty for an isolated extension usage with "
-                + "dev_dependency = False");
-      }
-    }
-
     Set<String> explicitRootModuleDirectDeps = new LinkedHashSet<>();
     for (String dep : rootModuleDirectDeps) {
       try {
@@ -192,40 +166,46 @@ Optional<Event> generateFixupMessage(
       // expected to modify any other module's MODULE.bazel file.
       return Optional.empty();
     }
+    // Every module only has at most a single usage of a given extension.
+    ModuleExtensionUsage rootUsage = Iterables.getOnlyElement(rootUsages);
 
     var rootModuleDirectDevDeps = getRootModuleDirectDevDeps(allRepos);
     var rootModuleDirectDeps = getRootModuleDirectDeps(allRepos);
     if (rootModuleDirectDevDeps.isEmpty() && rootModuleDirectDeps.isEmpty()) {
       return Optional.empty();
     }
-
     Preconditions.checkState(
         rootModuleDirectDevDeps.isPresent() && rootModuleDirectDeps.isPresent());
+
+    if (!rootUsage.getHasNonDevUseExtension() && !rootModuleDirectDeps.get().isEmpty()) {
+      throw Starlark.errorf(
+          "root_module_direct_deps must be empty if the root module contains no "
+              + "usages with dev_dependency = False");
+    }
+    if (!rootUsage.getHasDevUseExtension() && !rootModuleDirectDevDeps.get().isEmpty()) {
+      throw Starlark.errorf(
+          "root_module_direct_dev_deps must be empty if the root module contains no "
+              + "usages with dev_dependency = True");
+    }
+
     return generateFixupMessage(
-        rootUsages, allRepos, rootModuleDirectDeps.get(), rootModuleDirectDevDeps.get());
+        rootUsage, allRepos, rootModuleDirectDeps.get(), rootModuleDirectDevDeps.get());
   }
 
   private static Optional<Event> generateFixupMessage(
-      List<ModuleExtensionUsage> rootUsages,
+      ModuleExtensionUsage rootUsage,
       Set<String> allRepos,
       Set<String> expectedImports,
       Set<String> expectedDevImports) {
-    var actualDevImports =
-        rootUsages.stream()
-            .flatMap(usage -> usage.getDevImports().stream())
-            .collect(toImmutableSet());
+    var actualDevImports = ImmutableSet.copyOf(rootUsage.getDevImports());
     var actualImports =
-        rootUsages.stream()
-            .flatMap(usage -> usage.getImports().values().stream())
+        rootUsage.getImports().values().stream()
             .filter(repo -> !actualDevImports.contains(repo))
             .collect(toImmutableSet());
 
-    // All label strings that map to the same Label are equivalent for buildozer as it implements
-    // the same normalization of label strings with no or empty repo name.
-    ModuleExtensionUsage firstUsage = rootUsages.get(0);
-    String extensionBzlFile = firstUsage.getExtensionBzlFile();
-    String extensionName = firstUsage.getExtensionName();
-    Location location = firstUsage.getLocation();
+    String extensionBzlFile = rootUsage.getExtensionBzlFile();
+    String extensionName = rootUsage.getExtensionName();
+    Location location = rootUsage.getLocation();
 
     var importsToAdd = ImmutableSortedSet.copyOf(Sets.difference(expectedImports, actualImports));
     var importsToRemove =
@@ -290,28 +270,28 @@ private static Optional<Event> generateFixupMessage(
                     importsToAdd,
                     extensionBzlFile,
                     extensionName,
-                    firstUsage.getIsolationKey()),
+                    rootUsage.getIsolationKey()),
                 makeUseRepoCommand(
                     "use_repo_remove",
                     false,
                     importsToRemove,
                     extensionBzlFile,
                     extensionName,
-                    firstUsage.getIsolationKey()),
+                    rootUsage.getIsolationKey()),
                 makeUseRepoCommand(
                     "use_repo_add",
                     true,
                     devImportsToAdd,
                     extensionBzlFile,
                     extensionName,
-                    firstUsage.getIsolationKey()),
+                    rootUsage.getIsolationKey()),
                 makeUseRepoCommand(
                     "use_repo_remove",
                     true,
                     devImportsToRemove,
                     extensionBzlFile,
                     extensionName,
-                    firstUsage.getIsolationKey()))
+                    rootUsage.getIsolationKey()))
             .flatMap(Optional::stream);
 
     return Optional.of(

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

@@ -68,6 +68,14 @@ public abstract class ModuleExtensionUsage {
   /** All the tags specified by this module for this extension. */
   public abstract ImmutableList<Tag> getTags();
 
+  /** Whether any <code>use_extension</code> calls for this usage had <code>dev_dependency = True
+   * </code> set.**/
+  public abstract boolean getHasDevUseExtension();
+
+  /** Whether any <code>use_extension</code> calls for this usage had <code>dev_dependency = False
+   * </code> set.**/
+  public abstract boolean getHasNonDevUseExtension();
+
   public static Builder builder() {
     return new AutoValue_ModuleExtensionUsage.Builder();
   }
@@ -100,6 +108,10 @@ public ModuleExtensionUsage.Builder addTag(Tag value) {
       return this;
     }
 
+    public abstract Builder setHasDevUseExtension(boolean hasDevUseExtension);
+
+    public abstract Builder setHasNonDevUseExtension(boolean hasNonDevUseExtension);
+
     public abstract ModuleExtensionUsage build();
   }
 }

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

@@ -521,6 +521,9 @@ class ModuleExtensionUsageBuilder {
     private final ImmutableSet.Builder<String> devImports;
     private final ImmutableList.Builder<Tag> tags;
 
+    private boolean hasNonDevUseExtension;
+    private boolean hasDevUseExtension;
+
     ModuleExtensionUsageBuilder(
         String extensionBzlFile,
         String extensionName,
@@ -545,6 +548,8 @@ ModuleExtensionUsage buildUsage() {
               .setLocation(location)
               .setImports(ImmutableBiMap.copyOf(imports))
               .setDevImports(devImports.build())
+              .setHasDevUseExtension(hasDevUseExtension)
+              .setHasNonDevUseExtension(hasNonDevUseExtension)
               .setTags(tags.build());
       return builder.build();
     }
@@ -554,6 +559,11 @@ ModuleExtensionUsage buildUsage() {
      * tags with all other such proxies, thus preserving their order across dev/non-dev deps.
      */
     ModuleExtensionProxy getProxy(boolean devDependency) {
+      if (devDependency) {
+        hasDevUseExtension = true;
+      } else {
+        hasNonDevUseExtension = true;
+      }
       return new ModuleExtensionProxy(devDependency);
     }
 

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

@@ -46,6 +46,7 @@ public class StarlarkBazelModule implements StarlarkValue {
   private final String version;
   private final Tags tags;
   private final boolean isRootModule;
+  private final boolean hasDevUseExtensionOnly;
 
   @StarlarkBuiltin(
       name = "bazel_module_tags",
@@ -85,11 +86,17 @@ public String getErrorMessageForUnknownField(String field) {
     }
   }
 
-  private StarlarkBazelModule(String name, String version, Tags tags, boolean isRootModule) {
+  private StarlarkBazelModule(
+      String name,
+      String version,
+      Tags tags,
+      boolean isRootModule,
+      boolean hasDevUseExtensionOnly) {
     this.name = name;
     this.version = version;
     this.tags = tags;
     this.isRootModule = isRootModule;
+    this.hasDevUseExtensionOnly = hasDevUseExtensionOnly;
   }
 
   /**
@@ -132,11 +139,14 @@ public static StarlarkBazelModule create(
           .get(tag.getTagName())
           .add(TypeCheckedTag.create(tagClass, tag, labelConverter));
     }
+    boolean hasDevUseExtensionOnly =
+        usage != null && usage.getHasDevUseExtension() && !usage.getHasNonDevUseExtension();
     return new StarlarkBazelModule(
         module.getName(),
         module.getVersion().getOriginal(),
         new Tags(Maps.transformValues(typeCheckedTags, StarlarkList::immutableCopyOf)),
-        module.getKey().equals(ModuleKey.ROOT));
+        module.getKey().equals(ModuleKey.ROOT),
+        hasDevUseExtensionOnly);
   }
 
   @Override
@@ -169,4 +179,8 @@ public Tags getTags() {
   public boolean isRoot() {
     return isRootModule;
   }
+
+  public boolean hasDevUseExtensionOnly() {
+    return hasDevUseExtensionOnly;
+  }
 }