From da4c959603280ded71ad8fbc2d30d4917b175750 Mon Sep 17 00:00:00 2001
From: gregce <gregce@google.com>
Date: Thu, 27 Jul 2017 22:09:34 +0200
Subject: Fix confusing semantics for Attribute.Builder.allowedRuleClasses.

1) Update the javadocs.
2) Clarify that allowedRuleClases and allowedRuleClassesWithWarning
   must be disjoint sets.
3) Enforce 2).
4) Fix error messaging when only "with warnings" is set.

PiperOrigin-RevId: 163379567
---
 .../devtools/build/lib/analysis/BuildViewTest.java | 53 ++++++++++++++++++++++
 .../devtools/build/lib/analysis/util/MockRule.java |  6 ++-
 2 files changed, 58 insertions(+), 1 deletion(-)

(limited to 'src/test/java/com/google/devtools/build/lib/analysis')

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
index 4098a2b02f..6d107b3ee6 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
@@ -16,6 +16,7 @@ package com.google.devtools.build.lib.analysis;
 
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
+import static com.google.devtools.build.lib.packages.Attribute.attr;
 import static com.google.devtools.build.lib.testutil.MoreAsserts.assertEventCount;
 import static com.google.devtools.build.lib.testutil.MoreAsserts.assertEventCountAtLeast;
 import static org.junit.Assert.fail;
@@ -36,8 +37,10 @@ import com.google.devtools.build.lib.analysis.config.InvalidConfigurationExcepti
 import com.google.devtools.build.lib.analysis.util.AnalysisMock;
 import com.google.devtools.build.lib.analysis.util.BuildViewTestBase;
 import com.google.devtools.build.lib.analysis.util.ExpectedDynamicConfigurationErrors;
+import com.google.devtools.build.lib.analysis.util.MockRule;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.Attribute;
+import com.google.devtools.build.lib.packages.BuildType;
 import com.google.devtools.build.lib.packages.Rule;
 import com.google.devtools.build.lib.skyframe.SkyFunctions;
 import com.google.devtools.build.lib.skyframe.TargetPatternValue.TargetPatternKey;
@@ -1340,6 +1343,56 @@ public class BuildViewTest extends BuildViewTestBase {
     assertDoesNotContainEvent("implicitly depends upon");
   }
 
+  @Test
+  public void allowedRuleClassesAndAllowedRuleClassesWithWarning() throws Exception {
+    setRulesAvailableInTests(
+        (MockRule) () -> MockRule.define(
+            "custom_rule",
+            attr("deps", BuildType.LABEL_LIST)
+                .allowedFileTypes()
+                .allowedRuleClasses("java_library", "java_binary")
+                .allowedRuleClassesWithWarning("genrule")));
+
+    scratch.file("foo/BUILD",
+        "genrule(",
+        "    name = 'genlib',",
+        "    srcs = [],",
+        "    outs = ['genlib.out'],",
+        "    cmd = 'echo hi > $@')",
+        "custom_rule(",
+        "    name = 'foo',",
+        "    deps = [':genlib'])");
+
+    update("//foo");
+    assertContainsEvent("WARNING /workspace/foo/BUILD:8:12: in deps attribute of custom_rule rule "
+        + "//foo:foo: genrule rule '//foo:genlib' is unexpected here (expected java_library or "
+        + "java_binary); continuing anyway");
+  }
+
+  @Test
+  public void onlyAllowedRuleClassesWithWarning() throws Exception {
+    setRulesAvailableInTests(
+        (MockRule) () -> MockRule.define(
+            "custom_rule",
+            attr("deps", BuildType.LABEL_LIST)
+                .allowedFileTypes()
+                .allowedRuleClassesWithWarning("genrule")));
+
+    scratch.file("foo/BUILD",
+        "genrule(",
+        "    name = 'genlib',",
+        "    srcs = [],",
+        "    outs = ['genlib.out'],",
+        "    cmd = 'echo hi > $@')",
+        "custom_rule(",
+        "    name = 'foo',",
+        "    deps = [':genlib'])");
+
+    update("//foo");
+    assertContainsEvent("WARNING /workspace/foo/BUILD:8:12: in deps attribute of custom_rule rule "
+        + "//foo:foo: genrule rule '//foo:genlib' is unexpected here; continuing anyway");
+  }
+
   /** Runs the same test with the reduced loading phase. */
   @TestSpec(size = Suite.SMALL_TESTS)
   @RunWith(JUnit4.class)
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/MockRule.java b/src/test/java/com/google/devtools/build/lib/analysis/util/MockRule.java
index 9446a04ded..bba074bec4 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/MockRule.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/MockRule.java
@@ -101,6 +101,11 @@ public interface MockRule extends RuleDefinition {
   default void customize(RuleClass.Builder builder, RuleDefinitionEnvironment environment) {
   }
 
+  /**
+   * Default <code>"deps"</code> attribute for rule classes that don't need special behavior.
+   */
+  Attribute.Builder<?> DEPS_ATTRIBUTE = attr("deps", BuildType.LABEL_LIST).allowedFileTypes();
+
   /**
    * Builds out this rule with default attributes Blaze expects of all rules plus the custom
    * attributes defined by this implementation's {@link State}.
@@ -110,7 +115,6 @@ public interface MockRule extends RuleDefinition {
   @Override
   default RuleClass build(RuleClass.Builder builder, RuleDefinitionEnvironment environment) {
     builder
-        .add(attr("deps", BuildType.LABEL_LIST).allowedFileTypes())
         .add(attr("testonly", BOOLEAN).nonconfigurable("test").value(false))
         .add(attr("deprecation", STRING).nonconfigurable("test").value((String) null))
         .add(attr("tags", STRING_LIST))
-- 
cgit v1.2.3