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

2 files changed, 58 insertions, 1 deletions

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
@@ -102,6 +102,11 @@ public interface MockRule extends RuleDefinition {
   }
 
   /**
+   * 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))