Move LabelConversionContext to a new top-level class.

Also add a convenience method to create a label converter for a starlark thread. This makes it more easily available for other uses. Part of Optional Toolchains (#14726). Closes #14815. PiperOrigin-RevId: 429410754
bazelbuild · Feb 17, 2022 · f0cadba · f0cadba
1 parent ace0cb9
commit f0cadba
Show file tree

Hide file tree

Showing 12 changed files with 196 additions and 164 deletions.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttrModule.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkAttrModule.java
@@ -33,6 +33,7 @@
 import com.google.devtools.build.lib.packages.BazelModuleContext;
 import com.google.devtools.build.lib.packages.BazelStarlarkContext;
 import com.google.devtools.build.lib.packages.BuildType;
+import com.google.devtools.build.lib.packages.LabelConverter;
 import com.google.devtools.build.lib.packages.Provider;
 import com.google.devtools.build.lib.packages.StarlarkAspect;
 import com.google.devtools.build.lib.packages.StarlarkCallbackHelper;
@@ -44,7 +45,6 @@
 import com.google.devtools.build.lib.starlarkbuildapi.StarlarkAttrModuleApi;
 import com.google.devtools.build.lib.util.FileType;
 import com.google.devtools.build.lib.util.FileTypeSet;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import javax.annotation.Nullable;
@@ -145,10 +145,7 @@ private static Attribute.Builder<?> createAttribute(
         BazelModuleContext moduleContext =
             BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread));
         builder.defaultValue(
-            defaultValue,
-            new BuildType.LabelConversionContext(
-                moduleContext.label(), moduleContext.repoMapping(), new HashMap<>()),
-            DEFAULT_ARG);
+            defaultValue, LabelConverter.forModuleContext(moduleContext), DEFAULT_ARG);
       }
     }
 

diff --git a/...main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java b/...main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkRuleClassFunctions.java
@@ -68,12 +68,12 @@
 import com.google.devtools.build.lib.packages.BazelStarlarkContext;
 import com.google.devtools.build.lib.packages.BuildSetting;
 import com.google.devtools.build.lib.packages.BuildType;
-import com.google.devtools.build.lib.packages.BuildType.LabelConversionContext;
 import com.google.devtools.build.lib.packages.ConfigurationFragmentPolicy.MissingFragmentPolicy;
 import com.google.devtools.build.lib.packages.ExecGroup;
 import com.google.devtools.build.lib.packages.FunctionSplitTransitionAllowlist;
 import com.google.devtools.build.lib.packages.ImplicitOutputsFunction.StarlarkImplicitOutputsFunctionWithCallback;
 import com.google.devtools.build.lib.packages.ImplicitOutputsFunction.StarlarkImplicitOutputsFunctionWithMap;
+import com.google.devtools.build.lib.packages.LabelConverter;
 import com.google.devtools.build.lib.packages.Package.NameConflictException;
 import com.google.devtools.build.lib.packages.PackageFactory.PackageContext;
 import com.google.devtools.build.lib.packages.PredicateWithMessage;
@@ -568,17 +568,10 @@ private static void checkAttributeName(String name) throws EvalException {
   private static ImmutableList<Label> parseLabels(
       Iterable<String> inputs, StarlarkThread thread, String adjective) throws EvalException {
     ImmutableList.Builder<Label> parsedLabels = new ImmutableList.Builder<>();
-    BazelStarlarkContext bazelStarlarkContext = BazelStarlarkContext.from(thread);
-    BazelModuleContext moduleContext =
-        BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(thread));
-    LabelConversionContext context =
-        new LabelConversionContext(
-            moduleContext.label(),
-            moduleContext.repoMapping(),
-            bazelStarlarkContext.getConvertedLabelsInPackage());
+    LabelConverter converter = LabelConverter.forThread(thread);
     for (String input : inputs) {
       try {
-        Label label = context.convert(input);
+        Label label = converter.convert(input);
         parsedLabels.add(label);
       } catch (LabelSyntaxException e) {
         throw Starlark.errorf(

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkToolchainContext.java b/src/main/java/com/google/devtools/build/lib/analysis/starlark/StarlarkToolchainContext.java
@@ -21,13 +21,10 @@
 import com.google.devtools.build.lib.analysis.platform.ToolchainTypeInfo;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
-import com.google.devtools.build.lib.packages.BazelModuleContext;
-import com.google.devtools.build.lib.packages.BazelStarlarkContext;
-import com.google.devtools.build.lib.packages.BuildType.LabelConversionContext;
+import com.google.devtools.build.lib.packages.LabelConverter;
 import com.google.devtools.build.lib.starlarkbuildapi.platform.ToolchainContextApi;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.EvalException;
-import net.starlark.java.eval.Module;
 import net.starlark.java.eval.Printer;
 import net.starlark.java.eval.Starlark;
 import net.starlark.java.eval.StarlarkSemantics;
@@ -88,15 +85,8 @@ private Label transformKey(StarlarkThread starlarkThread, Object key) throws Eva
       return ((ToolchainTypeInfo) key).typeLabel();
     } else if (key instanceof String) {
       try {
-        BazelStarlarkContext bazelStarlarkContext = BazelStarlarkContext.from(starlarkThread);
-        BazelModuleContext moduleContext =
-            BazelModuleContext.of(Module.ofInnermostEnclosingStarlarkFunction(starlarkThread));
-        LabelConversionContext context =
-            new LabelConversionContext(
-                moduleContext.label(),
-                moduleContext.repoMapping(),
-                bazelStarlarkContext.getConvertedLabelsInPackage());
-        return context.convert((String) key);
+        LabelConverter converter = LabelConverter.forThread(starlarkThread);
+        return converter.convert((String) key);
       } catch (LabelSyntaxException e) {
         throw Starlark.errorf("Unable to parse toolchain label '%s': %s", key, e.getMessage());
       }

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelModuleResolutionFunction.java
@@ -27,15 +27,14 @@
 import com.google.devtools.build.lib.bazel.repository.RepositoryOptions.CheckDirectDepsMode;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
 import com.google.devtools.build.lib.events.Event;
-import com.google.devtools.build.lib.packages.BuildType.LabelConversionContext;
+import com.google.devtools.build.lib.packages.LabelConverter;
 import com.google.devtools.build.lib.server.FailureDetails.ExternalDeps.Code;
 import com.google.devtools.build.lib.skyframe.PrecomputedValue.Precomputed;
 import com.google.devtools.build.skyframe.SkyFunction;
 import com.google.devtools.build.skyframe.SkyFunctionException;
 import com.google.devtools.build.skyframe.SkyFunctionException.Transience;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
-import java.util.HashMap;
 import java.util.Map;
 import java.util.Objects;
 
@@ -126,17 +125,15 @@ static BazelModuleResolutionValue createValue(
     ImmutableTable.Builder<ModuleExtensionId, ModuleKey, ModuleExtensionUsage>
         extensionUsagesTableBuilder = ImmutableTable.builder();
     for (Module module : depGraph.values()) {
-      LabelConversionContext labelConversionContext =
-          new LabelConversionContext(
+      LabelConverter labelConverter =
+          new LabelConverter(
               StarlarkBazelModule.createModuleRootLabel(module.getCanonicalRepoName()),
-              module.getRepoMappingWithBazelDepsOnly(),
-              new HashMap<>());
+              module.getRepoMappingWithBazelDepsOnly());
       for (ModuleExtensionUsage usage : module.getExtensionUsages()) {
         try {
           ModuleExtensionId moduleExtensionId =
               ModuleExtensionId.create(
-                  labelConversionContext.convert(usage.getExtensionBzlFile()),
-                  usage.getExtensionName());
+                  labelConverter.convert(usage.getExtensionBzlFile()), usage.getExtensionName());
           extensionUsagesTableBuilder.put(moduleExtensionId, module.getKey(), usage);
         } catch (LabelSyntaxException e) {
           throw new BazelModuleResolutionFunctionException(

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModule.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModule.java
@@ -22,7 +22,7 @@
 import com.google.devtools.build.lib.cmdline.PackageIdentifier;
 import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.cmdline.RepositoryName;
-import com.google.devtools.build.lib.packages.BuildType.LabelConversionContext;
+import com.google.devtools.build.lib.packages.LabelConverter;
 import com.google.devtools.build.lib.server.FailureDetails.ExternalDeps.Code;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
@@ -112,11 +112,8 @@ public static StarlarkBazelModule create(
       RepositoryMapping repoMapping,
       @Nullable ModuleExtensionUsage usage)
       throws ExternalDepsException {
-    LabelConversionContext labelConversionContext =
-        new LabelConversionContext(
-            createModuleRootLabel(module.getCanonicalRepoName()),
-            repoMapping,
-            /* convertedLabelsInPackage= */ new HashMap<>());
+    LabelConverter labelConverter =
+        new LabelConverter(createModuleRootLabel(module.getCanonicalRepoName()), repoMapping);
     ImmutableList<Tag> tags = usage == null ? ImmutableList.of() : usage.getTags();
     HashMap<String, ArrayList<TypeCheckedTag>> typeCheckedTags = new HashMap<>();
     for (String tagClassName : extension.getTagClasses().keySet()) {
@@ -138,7 +135,7 @@ public static StarlarkBazelModule create(
       // (for example, String to Label).
       typeCheckedTags
           .get(tag.getTagName())
-          .add(TypeCheckedTag.create(tagClass, tag, labelConversionContext));
+          .add(TypeCheckedTag.create(tagClass, tag, labelConverter));
     }
     return new StarlarkBazelModule(
         module.getName(),

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTag.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTag.java
@@ -16,7 +16,7 @@
 
 import com.google.common.collect.ImmutableCollection;
 import com.google.devtools.build.lib.packages.Attribute;
-import com.google.devtools.build.lib.packages.BuildType.LabelConversionContext;
+import com.google.devtools.build.lib.packages.LabelConverter;
 import com.google.devtools.build.lib.packages.Type.ConversionException;
 import com.google.devtools.build.lib.server.FailureDetails.ExternalDeps.Code;
 import java.util.Map;
@@ -40,8 +40,7 @@ private TypeCheckedTag(TagClass tagClass, Object[] attrValues) {
   }
 
   /** Creates a {@link TypeCheckedTag}. */
-  public static TypeCheckedTag create(
-      TagClass tagClass, Tag tag, LabelConversionContext labelConversionContext)
+  public static TypeCheckedTag create(TagClass tagClass, Tag tag, LabelConverter labelConverter)
       throws ExternalDepsException {
     Object[] attrValues = new Object[tagClass.getAttributes().size()];
     for (Map.Entry<String, Object> attrValue : tag.getAttributeValues().entrySet()) {
@@ -57,8 +56,7 @@ public static TypeCheckedTag create(
       Object nativeValue;
       try {
         nativeValue =
-            attr.getType()
-                .convert(attrValue.getValue(), attr.getPublicName(), labelConversionContext);
+            attr.getType().convert(attrValue.getValue(), attr.getPublicName(), labelConverter);
       } catch (ConversionException e) {
         throw ExternalDepsException.withCauseAndMessage(
             Code.BAD_MODULE,

diff --git a/src/main/java/com/google/devtools/build/lib/packages/BuildType.java b/src/main/java/com/google/devtools/build/lib/packages/BuildType.java
@@ -21,7 +21,6 @@
 import com.google.common.collect.Maps;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
-import com.google.devtools.build.lib.cmdline.RepositoryMapping;
 import com.google.devtools.build.lib.packages.License.DistributionType;
 import com.google.devtools.build.lib.packages.License.LicenseParsingException;
 import com.google.devtools.build.lib.packages.Type.ConversionException;
@@ -31,7 +30,6 @@
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.SerializationConstant;
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.HashMap;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
@@ -142,8 +140,7 @@ public static boolean isLabelType(Type<?> type) {
    * <p>The caller is responsible for casting the returned value appropriately.
    */
   public static <T> Object selectableConvert(
-      Type<T> type, Object x, Object what, LabelConversionContext context)
-      throws ConversionException {
+      Type<T> type, Object x, Object what, LabelConverter context) throws ConversionException {
     if (x instanceof com.google.devtools.build.lib.packages.SelectorList) {
       return new SelectorList<>(
           ((com.google.devtools.build.lib.packages.SelectorList) x).getElements(),
@@ -155,53 +152,6 @@ public static <T> Object selectableConvert(
     }
   }
 
-  /** Context in which to evaluate a label with repository remappings */
-  public static class LabelConversionContext {
-    private final Label label;
-    private final RepositoryMapping repositoryMapping;
-    private final HashMap<String, Label> convertedLabelsInPackage;
-
-    public LabelConversionContext(
-        Label label,
-        RepositoryMapping repositoryMapping,
-        HashMap<String, Label> convertedLabelsInPackage) {
-      this.label = label;
-      this.repositoryMapping = repositoryMapping;
-      this.convertedLabelsInPackage = convertedLabelsInPackage;
-    }
-
-    Label getLabel() {
-      return label;
-    }
-
-    /** Returns the Label corresponding to the input, using the current conversion context. */
-    public Label convert(String input) throws LabelSyntaxException {
-      // Optimization: First check the package-local map, avoiding Label validation, Label
-      // construction, and global Interner lookup. This approach tends to be very profitable
-      // overall, since it's common for the targets in a single package to have duplicate
-      // label-strings across all their attribute values.
-      Label converted = convertedLabelsInPackage.get(input);
-      if (converted == null) {
-        converted = label.getRelativeWithRemapping(input, repositoryMapping);
-        convertedLabelsInPackage.put(input, converted);
-      }
-      return converted;
-    }
-
-    RepositoryMapping getRepositoryMapping() {
-      return repositoryMapping;
-    }
-
-    HashMap<String, Label> getConvertedLabelsInPackage() {
-      return convertedLabelsInPackage;
-    }
-
-    @Override
-    public String toString() {
-      return label.toString();
-    }
-  }
-
   private static class LabelType extends Type<Label> {
     private final LabelClass labelClass;
 
@@ -253,9 +203,9 @@ public Label convert(Object x, Object what, Object context) throws ConversionExc
           // TODO(b/110308446): remove instances of context being a Label
         } else if (context instanceof Label) {
           return ((Label) context).getRelativeWithRemapping(str, ImmutableMap.of());
-        } else if (context instanceof LabelConversionContext) {
-          LabelConversionContext labelConversionContext = (LabelConversionContext) context;
-          return labelConversionContext.convert(str);
+        } else if (context instanceof LabelConverter) {
+          LabelConverter labelConverter = (LabelConverter) context;
+          return labelConverter.convert(str);
         } else {
           throw new ConversionException("invalid context '" + context + "' in " + what);
         }
@@ -441,16 +391,13 @@ public Label convert(Object x, Object what, Object context) throws ConversionExc
       }
       try {
         // Enforce value is relative to the context.
-        Label currentRule;
-        RepositoryMapping repositoryMapping;
-        if (context instanceof LabelConversionContext) {
-          currentRule = ((LabelConversionContext) context).getLabel();
-          repositoryMapping = ((LabelConversionContext) context).getRepositoryMapping();
-        } else {
+        if (!(context instanceof LabelConverter)) {
           throw new ConversionException("invalid context '" + context + "' in " + what);
         }
-        Label result = currentRule.getRelativeWithRemapping(value, repositoryMapping);
-        if (!result.getPackageIdentifier().equals(currentRule.getPackageIdentifier())) {
+
+        LabelConverter converter = (LabelConverter) context;
+        Label result = converter.convert(value);
+        if (!result.getPackageIdentifier().equals(converter.getBase().getPackageIdentifier())) {
           throw new ConversionException("label '" + value + "' is not in the current package");
         }
         return result;
@@ -474,7 +421,7 @@ public static final class SelectorList<T> implements StarlarkValue {
 
     @VisibleForTesting
     SelectorList(
-        List<Object> x, Object what, @Nullable LabelConversionContext context, Type<T> originalType)
+        List<Object> x, Object what, @Nullable LabelConverter context, Type<T> originalType)
         throws ConversionException {
       if (x.size() > 1 && originalType.concat(ImmutableList.of()) == null) {
         throw new ConversionException(
@@ -588,10 +535,7 @@ public static final class Selector<T> {
 
     /** Creates a new Selector using the default error message when no conditions match. */
     Selector(
-        ImmutableMap<?, ?> x,
-        Object what,
-        @Nullable LabelConversionContext context,
-        Type<T> originalType)
+        ImmutableMap<?, ?> x, Object what, @Nullable LabelConverter context, Type<T> originalType)
         throws ConversionException {
       this(x, what, context, originalType, "");
     }
@@ -600,7 +544,7 @@ public static final class Selector<T> {
     Selector(
         ImmutableMap<?, ?> x,
         Object what,
-        @Nullable LabelConversionContext context,
+        @Nullable LabelConverter context,
         Type<T> originalType,
         String noMatchError)
         throws ConversionException {