commit ff938b33b2480951a87816a5bb8c7d82d993d443
author Michael Staib <mstaib@google.com> Thu Aug 27 19:22:59 2015 +0000
committer Lukacs Berki <lberki@google.com> Fri Aug 28 09:21:34 2015 +0000
tree f43724db48a6961ce53b775b013146620943cc31
parent 28a67609c922deb87537865e0a18912f61d4b13a

Verify values for selects in attributes with allowed values.

Currently, the selector list is passed to the allowed value predicate.
Since the allowed value predicate obviously does not understand this,
it fails with an ugly error. This change causes it to instead check
every possible value of the attribute to ensure they're all valid.

--
MOS_MIGRATED_REVID=101705850

src/main/java/com/google/devtools/build/lib/packages/RuleClass.java

diff --git a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
index 43a8ea0..3affb16 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/RuleClass.java
@@ -1364,9 +1364,9 @@
         continue;
       }
 
-      checkAllowedValues(rule, attribute, value.getValue(), eventHandler);
       rule.setAttributeValue(attribute, value.getValue(), value.getExplicitlySpecified());
       rule.setAttributeLocation(attribute, value.getLocation());
+      checkAllowedValues(rule, attribute, eventHandler);
 
       if (attribute.getName().equals("visibility")) {
         // TODO(bazel-team): Verify that this cast works
@@ -1437,8 +1437,8 @@
         } else {
           Object defaultValue = getAttributeNoncomputedDefaultValue(attr, pkgBuilder);
           checkAttrValNonEmpty(rule, eventHandler, defaultValue, attrIndex);
-          checkAllowedValues(rule, attr, defaultValue, eventHandler);
           rule.setAttributeValue(attr, defaultValue, /*explicit=*/false);
+          checkAllowedValues(rule, attr, eventHandler);
         }
       }
     }
@@ -1653,19 +1653,32 @@
       rule.setVisibility(PackageFactory.getVisibility(attrList));
     }
 
-    checkAllowedValues(rule, attr, converted, eventHandler);
     rule.setAttributeValue(attr, converted, /*explicit=*/true);
+    checkAllowedValues(rule, attr, eventHandler);
     return attrIndex;
   }
 
-  private void checkAllowedValues(Rule rule, Attribute attribute, Object value,
-      EventHandler eventHandler) {
+  /**
+   * Verifies that the rule has a valid value for the attribute according to its allowed values.
+   *
+   * <p>If the value for the given attribute on the given rule is invalid, an error will be recorded
+   * in the given EventHandler.
+   *
+   * <p>If the rule is configurable, all of its potential values are evaluated, and errors for each
+   * of the invalid values are reported.
+   */
+  private void checkAllowedValues(Rule rule, Attribute attribute, EventHandler eventHandler) {
     if (attribute.checkAllowedValues()) {
       PredicateWithMessage<Object> allowedValues = attribute.getAllowedValues();
-      if (!allowedValues.apply(value)) {
-        rule.reportError(String.format(rule.getLabel() + ": invalid value in '%s' attribute: %s",
-            attribute.getName(),
-            allowedValues.getErrorReason(value)), eventHandler);
+      Iterable<?> values =
+          AggregatingAttributeMapper.of(rule).visitAttribute(
+              attribute.getName(), attribute.getType());
+      for (Object value : values) {
+        if (!allowedValues.apply(value)) {
+          rule.reportError(String.format(rule.getLabel() + ": invalid value in '%s' attribute: %s",
+              attribute.getName(),
+              allowedValues.getErrorReason(value)), eventHandler);
+        }
       }
     }
   }