Don't crash when building environment groups directly

(following PackageGroup's precedent).

Also cleanup: generalize the pattern by which we determine
non-configurable target types.

--
MOS_MIGRATED_REVID=104125803

14 files changed, 97 insertions, 10 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
index e1c0defd2a..1a2488f881 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredTargetFactory.java
@@ -35,6 +35,7 @@ import com.google.devtools.build.lib.events.EventHandler;
 import com.google.devtools.build.lib.packages.AspectParameters;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.ConstantRuleVisibility;
+import com.google.devtools.build.lib.packages.EnvironmentGroup;
 import com.google.devtools.build.lib.packages.InputFile;
 import com.google.devtools.build.lib.packages.OutputFile;
 import com.google.devtools.build.lib.packages.PackageGroup;
@@ -201,6 +202,8 @@ public final class ConfiguredTargetFactory {
     } else if (target instanceof PackageGroup) {
       PackageGroup packageGroup = (PackageGroup) target;
       return new PackageGroupConfiguredTarget(targetContext, packageGroup);
+    } else if (target instanceof EnvironmentGroup) {
+      return new EnvironmentGroupConfiguredTarget(targetContext, (EnvironmentGroup) target);
     } else {
       throw new AssertionError("Unexpected target class: " + target.getClass().getName());
     }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/EnvironmentGroupConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/EnvironmentGroupConfiguredTarget.java
new file mode 100644
index 0000000000..41741c7201
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/analysis/EnvironmentGroupConfiguredTarget.java
@@ -0,0 +1,46 @@
+// Copyright 2015 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.analysis;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.UnmodifiableIterator;
+import com.google.devtools.build.lib.packages.EnvironmentGroup;
+
+/**
+ * Dummy ConfiguredTarget for environment groups. Contains no functionality, since
+ * environment groups are not really first-class Targets.
+ */
+public final class EnvironmentGroupConfiguredTarget extends AbstractConfiguredTarget {
+  EnvironmentGroupConfiguredTarget(TargetContext targetContext, EnvironmentGroup envGroup) {
+    super(targetContext);
+    Preconditions.checkArgument(targetContext.getConfiguration() == null);
+  }
+
+  @Override
+  public EnvironmentGroup getTarget() {
+    return (EnvironmentGroup) super.getTarget();
+  }
+
+  @Override
+  public Object get(String providerKey) {
+    // No providers.
+    return null;
+  }
+
+  @Override
+  public UnmodifiableIterator<TransitiveInfoProvider> iterator() {
+    throw new UnsupportedOperationException();
+  }
+}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/PackageGroupConfiguredTarget.java b/src/main/java/com/google/devtools/build/lib/analysis/PackageGroupConfiguredTarget.java
index c238b74ea5..d0382dc967 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/PackageGroupConfiguredTarget.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/PackageGroupConfiguredTarget.java
@@ -73,6 +73,6 @@ public final class PackageGroupConfiguredTarget extends AbstractConfiguredTarget
 
   @Override
   public UnmodifiableIterator<TransitiveInfoProvider> iterator() {
-    throw new IllegalStateException();
+    throw new UnsupportedOperationException();
   }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationCollection.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationCollection.java
index 6fada4c226..82d8e49bb9 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationCollection.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationCollection.java
@@ -23,8 +23,6 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.Attribute.SplitTransition;
 import com.google.devtools.build.lib.packages.Attribute.Transition;
-import com.google.devtools.build.lib.packages.InputFile;
-import com.google.devtools.build.lib.packages.PackageGroup;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.packages.Target;
 
@@ -77,7 +75,7 @@ public final class BuildConfigurationCollection {
 
   public static BuildConfiguration configureTopLevelTarget(BuildConfiguration topLevelConfiguration,
       Target toTarget) {
-    if (toTarget instanceof InputFile || toTarget instanceof PackageGroup) {
+    if (!toTarget.isConfigurable()) {
       return null;
     }
     return topLevelConfiguration.getTransitions().toplevelConfigurationHook(toTarget);
diff --git a/src/main/java/com/google/devtools/build/lib/packages/EnvironmentGroup.java b/src/main/java/com/google/devtools/build/lib/packages/EnvironmentGroup.java
index a5b977c3c8..0536dc3a68 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/EnvironmentGroup.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/EnvironmentGroup.java
@@ -310,6 +310,11 @@ public class EnvironmentGroup implements Target {
     return ConstantRuleVisibility.PRIVATE; // No rule should be referencing an environment_group.
   }
 
+  @Override
+  public boolean isConfigurable() {
+    return false;
+  }
+
   public static String targetKind() {
     return "environment group";
   }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/InputFile.java b/src/main/java/com/google/devtools/build/lib/packages/InputFile.java
index 1cb2cc0956..f136aeff01 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/InputFile.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/InputFile.java
@@ -71,6 +71,11 @@ public final class InputFile extends FileTarget {
     }
   }
 
+  @Override
+  public boolean isConfigurable() {
+    return false;
+  }
+
   public boolean isLicenseSpecified() {
     return license != null && license != License.NO_LICENSE;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/OutputFile.java b/src/main/java/com/google/devtools/build/lib/packages/OutputFile.java
index b7259dab12..eed4c63f3f 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/OutputFile.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/OutputFile.java
@@ -38,6 +38,11 @@ public final class OutputFile extends FileTarget {
     return generatingRule.getVisibility();
   }
 
+  @Override
+  public boolean isConfigurable() {
+    return true;
+  }
+
   /**
    * Returns the rule which generates this output file.
    */
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java b/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java
index d7fe3df4c4..3695e367d4 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageGroup.java
@@ -146,6 +146,11 @@ public class PackageGroup implements Target {
     return ConstantRuleVisibility.PUBLIC;
   }
 
+  @Override
+  public boolean isConfigurable() {
+    return false;
+  }
+
   public static String targetKind() {
     return "package group";
   }
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Rule.java b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
index 6f8e60cbcd..24ed1f7a1a 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Rule.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Rule.java
@@ -604,6 +604,11 @@ public final class Rule implements Target {
   }
 
   @Override
+  public boolean isConfigurable() {
+    return true;
+  }
+
+  @Override
   @SuppressWarnings("unchecked")
   public Set<DistributionType> getDistributions() {
     if (isAttrDefined("distribs", BuildType.DISTRIBUTIONS)
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Target.java b/src/main/java/com/google/devtools/build/lib/packages/Target.java
index f515f4f9c3..7fb42b66c7 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Target.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Target.java
@@ -78,4 +78,9 @@ public interface Target {
    * Returns the visibility of this target.
    */
   RuleVisibility getVisibility();
+
+  /**
+   * Returns whether this target type can be configured (e.g. accepts non-null configurations).
+   */
+  boolean isConfigurable();
 }
diff --git a/src/main/java/com/google/devtools/build/lib/query2/FakeSubincludeTarget.java b/src/main/java/com/google/devtools/build/lib/query2/FakeSubincludeTarget.java
index c42f7cbe3b..7ce3a88a09 100644
--- a/src/main/java/com/google/devtools/build/lib/query2/FakeSubincludeTarget.java
+++ b/src/main/java/com/google/devtools/build/lib/query2/FakeSubincludeTarget.java
@@ -83,4 +83,9 @@ public class FakeSubincludeTarget implements Target {
   public RuleVisibility getVisibility() {
     return ConstantRuleVisibility.PUBLIC;
   }
+
+  @Override
+  public boolean isConfigurable() {
+    return true;
+  }
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
index acb45eb5ec..c98da4e978 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
@@ -170,11 +170,7 @@ final class ConfiguredTargetFunction implements SkyFunction {
     // that doesn't match; we can even have the same value multiple times. However, I think it's
     // only triggered in tests (i.e., in normal operation, the configuration passed in is already
     // null).
-    if (target instanceof InputFile) {
-      // InputFileConfiguredTarget expects its configuration to be null since it's not used.
-      configuration = null;
-    } else if (target instanceof PackageGroup) {
-      // Same for PackageGroupConfiguredTarget.
+    if (!target.isConfigurable()) {
       configuration = null;
     }
     TargetAndConfiguration ctgValue =
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/constraints/AbstractConstraintsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/constraints/AbstractConstraintsTest.java
index e7033d9a96..9b44af9493 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/constraints/AbstractConstraintsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/constraints/AbstractConstraintsTest.java
@@ -68,8 +68,9 @@ public abstract class AbstractConstraintsTest extends BuildViewTestCase {
             .append(getAttrDef("fulfills", fulfillsMap.get(env).toArray(new String[0])))
             .append(")\n");
       }
+      String envGroupName = name.contains("/") ? name.substring(name.lastIndexOf("/") + 1) : name;
       builder.append("environment_group(\n")
-          .append("    name = '" + name + "',\n")
+          .append("    name = '" + envGroupName + "',\n")
           .append(getAttrDef("environments", environments.toArray(new String[0])) + ",\n")
           .append(getAttrDef("defaults", defaults.toArray(new String[0])) + ",\n")
           .append(")");
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/constraints/ConstraintsTest.java b/src/test/java/com/google/devtools/build/lib/analysis/constraints/ConstraintsTest.java
index c019d9691a..007ac1a92f 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/constraints/ConstraintsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/constraints/ConstraintsTest.java
@@ -800,4 +800,12 @@ public class ConstraintsTest extends AbstractConstraintsTest {
     assertNull(getConfiguredTarget("//ihave:nolimits"));
     assertContainsEvent("no such attribute 'restricted_to' in 'totally_free_rule'");
   }
+
+  public void testBuildingEnvironmentGroupDirectlyDoesntCrash() throws Exception {
+    new EnvironmentGroupMaker("buildenv/foo")
+        .setEnvironments("a", "b")
+        .setDefaults("a")
+        .make();
+    assertNotNull(getConfiguredTarget("//buildenv/foo:foo"));
+  }
 }