Refactor PlatformInfo.Builder to detect and report invalid configurations.

Change-Id: Ib2bbd1b35985c4ec2d1e411aea4b32af7433a426
PiperOrigin-RevId: 153856560

2 files changed, 112 insertions, 70 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/platform/Platform.java b/src/main/java/com/google/devtools/build/lib/rules/platform/Platform.java
index fde1b73184..c39660585b 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/platform/Platform.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/platform/Platform.java
@@ -14,10 +14,6 @@
 
 package com.google.devtools.build.lib.rules.platform;
 
-import com.google.common.collect.ArrayListMultimap;
-import com.google.common.collect.ImmutableMap;
-import com.google.common.collect.Iterables;
-import com.google.common.collect.Multimap;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.FileProvider;
 import com.google.devtools.build.lib.analysis.FilesToRunProvider;
@@ -26,6 +22,7 @@ import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
 import com.google.devtools.build.lib.analysis.RuleContext;
 import com.google.devtools.build.lib.analysis.RunfilesProvider;
 import com.google.devtools.build.lib.rules.RuleConfiguredTargetFactory;
+import com.google.devtools.build.lib.rules.platform.PlatformInfo.DuplicateConstraintException;
 import com.google.devtools.build.lib.syntax.Type;
 import java.util.Map;
 
@@ -39,71 +36,27 @@ public class Platform implements RuleConfiguredTargetFactory {
         ConstraintValueInfo.fromTargets(
             ruleContext.getPrerequisites(PlatformRule.CONSTRAINT_VALUES_ATTR, Mode.DONT_CHECK));
 
-    // Verify the constraints - no two values for the same setting, and construct the map.
-    ImmutableMap<ConstraintSettingInfo, ConstraintValueInfo> constraints =
-        validateConstraints(ruleContext, constraintValues);
-    if (constraints == null) {
-      // An error occurred, return null.
-      return null;
-    }
-
     PlatformInfo.Builder platformBuilder = PlatformInfo.builder();
-    platformBuilder.addConstraints(constraints.values());
+    platformBuilder.addConstraints(constraintValues);
 
     Map<String, String> remoteExecutionProperties =
         ruleContext.attributes().get(PlatformRule.REMOTE_EXECUTION_PROPS_ATTR, Type.STRING_DICT);
     platformBuilder.addRemoteExecutionProperties(remoteExecutionProperties);
 
+    PlatformInfo platformInfo;
+    try {
+      platformInfo = platformBuilder.build();
+    } catch (DuplicateConstraintException e) {
+      // Report the error and return null.
+      ruleContext.attributeError(PlatformRule.CONSTRAINT_VALUES_ATTR, e.getMessage());
+      return null;
+    }
+
     return new RuleConfiguredTargetBuilder(ruleContext)
         .addProvider(RunfilesProvider.class, RunfilesProvider.EMPTY)
         .addProvider(FileProvider.class, FileProvider.EMPTY)
         .addProvider(FilesToRunProvider.class, FilesToRunProvider.EMPTY)
-        .addNativeDeclaredProvider(platformBuilder.build())
+        .addNativeDeclaredProvider(platformInfo)
         .build();
   }
-
-  private ImmutableMap<ConstraintSettingInfo, ConstraintValueInfo> validateConstraints(
-      RuleContext ruleContext, Iterable<ConstraintValueInfo> constraintValues) {
-    Multimap<ConstraintSettingInfo, ConstraintValueInfo> constraints = ArrayListMultimap.create();
-
-    for (ConstraintValueInfo constraintValue : constraintValues) {
-      constraints.put(constraintValue.constraint(), constraintValue);
-    }
-
-    // Are there any settings with more than one value?
-    boolean foundError = false;
-    for (ConstraintSettingInfo constraintSetting : constraints.keySet()) {
-      if (constraints.get(constraintSetting).size() > 1) {
-        foundError = true;
-        // error
-        StringBuilder constraintValuesDescription = new StringBuilder();
-        for (ConstraintValueInfo constraintValue : constraints.get(constraintSetting)) {
-          if (constraintValuesDescription.length() > 0) {
-            constraintValuesDescription.append(", ");
-          }
-          constraintValuesDescription.append(constraintValue.label());
-        }
-        ruleContext.attributeError(
-            PlatformRule.CONSTRAINT_VALUES_ATTR,
-            String.format(
-                "Duplicate constraint_values for constraint_setting %s: %s",
-                constraintSetting.label(), constraintValuesDescription.toString()));
-      }
-    }
-
-    if (foundError) {
-      return null;
-    }
-
-    // Convert to a flat map.
-    ImmutableMap.Builder<ConstraintSettingInfo, ConstraintValueInfo> builder =
-        new ImmutableMap.Builder<>();
-    for (ConstraintSettingInfo constraintSetting : constraints.keySet()) {
-      ConstraintValueInfo constraintValue =
-          Iterables.getOnlyElement(constraints.get(constraintSetting));
-      builder.put(constraintSetting, constraintValue);
-    }
-
-    return builder.build();
-  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/rules/platform/PlatformInfo.java b/src/main/java/com/google/devtools/build/lib/rules/platform/PlatformInfo.java
index 7406c1404d..3f980733cf 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/platform/PlatformInfo.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/platform/PlatformInfo.java
@@ -15,9 +15,12 @@
 package com.google.devtools.build.lib.rules.platform;
 
 import com.google.common.base.Function;
+import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Multimap;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.packages.ClassObjectConstructor;
@@ -53,8 +56,8 @@ public class PlatformInfo extends SkylarkClassObject {
   public static final SkylarkProviderIdentifier SKYLARK_IDENTIFIER =
       SkylarkProviderIdentifier.forKey(SKYLARK_CONSTRUCTOR.getKey());
 
-  private ImmutableList<ConstraintValueInfo> constraints;
-  private ImmutableMap<String, String> remoteExecutionProperties;
+  private final ImmutableList<ConstraintValueInfo> constraints;
+  private final ImmutableMap<String, String> remoteExecutionProperties;
 
   private PlatformInfo(
       ImmutableList<ConstraintValueInfo> constraints,
@@ -101,22 +104,33 @@ public class PlatformInfo extends SkylarkClassObject {
         });
   }
 
+  /** Returns a new {@link Builder} for creating a fresh {@link PlatformInfo} instance. */
   public static Builder builder() {
     return new Builder();
   }
 
-  /**
-   * Builder class to facilitate creating valid {@link PlatformInfo} instances.
-   */
+  /** Builder class to facilitate creating valid {@link PlatformInfo} instances. */
   public static class Builder {
-    private List<ConstraintValueInfo> constraints = new ArrayList<>();
-    private Map<String, String> remoteExecutionProperties = new HashMap<>();
-
+    private final List<ConstraintValueInfo> constraints = new ArrayList<>();
+    private final Map<String, String> remoteExecutionProperties = new HashMap<>();
+
+    /**
+     * Adds the given constraint value to the constraints that define this {@link PlatformInfo}.
+     *
+     * @param constraint the constraint to add
+     * @return the {@link Builder} instance for method chaining
+     */
     public Builder addConstraint(ConstraintValueInfo constraint) {
       this.constraints.add(constraint);
       return this;
     }
 
+    /**
+     * Adds the given constraint values to the constraints that define this {@link PlatformInfo}.
+     *
+     * @param constraints the constraints to add
+     * @return the {@link Builder} instance for method chaining
+     */
     public Builder addConstraints(Iterable<ConstraintValueInfo> constraints) {
       for (ConstraintValueInfo constraint : constraints) {
         this.addConstraint(constraint);
@@ -125,11 +139,26 @@ public class PlatformInfo extends SkylarkClassObject {
       return this;
     }
 
+    /**
+     * Adds the given key/value pair to the data being sent to a potential remote executor. If the
+     * key already exists in the map, the previous value will be overwritten.
+     *
+     * @param key the key to be used
+     * @param value the value to be used
+     * @return the {@link Builder} instance for method chaining
+     */
     public Builder addRemoteExecutionProperty(String key, String value) {
       this.remoteExecutionProperties.put(key, value);
       return this;
     }
 
+    /**
+     * Adds the given properties to the data being sent to a potential remote executor. If any key
+     * already exists in the map, the previous value will be overwritten.
+     *
+     * @param properties the properties to be added
+     * @return the {@link Builder} instance for method chaining
+     */
     public Builder addRemoteExecutionProperties(Map<String, String> properties) {
       for (Map.Entry<String, String> entry : properties.entrySet()) {
         this.addRemoteExecutionProperty(entry.getKey(), entry.getValue());
@@ -137,9 +166,69 @@ public class PlatformInfo extends SkylarkClassObject {
       return this;
     }
 
-    public PlatformInfo build() {
-      return new PlatformInfo(
-          ImmutableList.copyOf(constraints), ImmutableMap.copyOf(remoteExecutionProperties));
+    /**
+     * Returns the new {@link PlatformInfo} instance.
+     *
+     * @throws DuplicateConstraintException if more than one constraint value exists for the same
+     *     constraint setting
+     */
+    public PlatformInfo build() throws DuplicateConstraintException {
+      ImmutableList<ConstraintValueInfo> validatedConstraints = validateConstraints(constraints);
+      return new PlatformInfo(validatedConstraints, ImmutableMap.copyOf(remoteExecutionProperties));
+    }
+
+    private ImmutableList<ConstraintValueInfo> validateConstraints(
+        Iterable<ConstraintValueInfo> constraintValues) throws DuplicateConstraintException {
+      Multimap<ConstraintSettingInfo, ConstraintValueInfo> constraints = ArrayListMultimap.create();
+
+      for (ConstraintValueInfo constraintValue : constraintValues) {
+        constraints.put(constraintValue.constraint(), constraintValue);
+      }
+
+      // Are there any settings with more than one value?
+      for (ConstraintSettingInfo constraintSetting : constraints.keySet()) {
+        if (constraints.get(constraintSetting).size() > 1) {
+          // Only reports the first case of this error.
+          throw new DuplicateConstraintException(
+              constraintSetting, constraints.get(constraintSetting));
+        }
+      }
+
+      return ImmutableList.copyOf(constraints.values());
+    }
+  }
+
+  /**
+   * Exception class used when more than one {@link ConstraintValueInfo} for the same {@link
+   * ConstraintSettingInfo} is added to a {@link Builder}.
+   */
+  public static class DuplicateConstraintException extends Exception {
+    private final ImmutableSet<ConstraintValueInfo> duplicateConstraints;
+
+    public DuplicateConstraintException(
+        ConstraintSettingInfo constraintSetting,
+        Iterable<ConstraintValueInfo> duplicateConstraintValues) {
+      super(formatError(constraintSetting, duplicateConstraintValues));
+      this.duplicateConstraints = ImmutableSet.copyOf(duplicateConstraintValues);
+    }
+
+    public ImmutableSet<ConstraintValueInfo> duplicateConstraints() {
+      return duplicateConstraints;
+    }
+
+    private static String formatError(
+        ConstraintSettingInfo constraintSetting,
+        Iterable<ConstraintValueInfo> duplicateConstraints) {
+      StringBuilder constraintValuesDescription = new StringBuilder();
+      for (ConstraintValueInfo constraintValue : duplicateConstraints) {
+        if (constraintValuesDescription.length() > 0) {
+          constraintValuesDescription.append(", ");
+        }
+        constraintValuesDescription.append(constraintValue.label());
+      }
+      return String.format(
+          "Duplicate constraint_values for constraint_setting %s: %s",
+          constraintSetting.label(), constraintValuesDescription.toString());
     }
   }
 }