Allow objc rule deps to include any rule that exports an "objc" provider.

This change was motivated by a need to write pure Skylark rules that expose their own objc providers so they can be used as deps to other libraries/application targets (e.g., SceneKit/SpriteKit compiled resources, []) without having to whitelist them and wait for a Blaze release.

This CL fixes what seems to be a bug in validateRuleDependency, where the behavior in the doc comment implies that it will accept a whitelisted rule name *or* a list of mandatory providers, but as implemented today it seems to require the rule to be whitelisted even if the mandatory native providers matched.

RELNOTES: objc_* rules can now depend on any target that returns an "objc" provider.

--
MOS_MIGRATED_REVID=128835096

3 files changed, 55 insertions, 36 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index 8965cb596a..b429606b48 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -77,6 +77,7 @@ import com.google.devtools.build.lib.syntax.EvalException;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.FileTypeSet;
 import com.google.devtools.build.lib.util.Preconditions;
+import com.google.devtools.build.lib.util.StringUtil;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.ArrayList;
@@ -1613,19 +1614,28 @@ public final class RuleContext extends TargetContext
       reporter.attributeWarning(attrName, message);
     }
 
-    private void reportBadPrerequisite(Attribute attribute, String targetKind,
-        ConfiguredTarget prerequisite, String reason, boolean isWarning) {
+    private String badPrerequisiteMessage(String targetKind, ConfiguredTarget prerequisite,
+        String reason, boolean isWarning) {
       String msgPrefix = targetKind != null ? targetKind + " " : "";
       String msgReason = reason != null ? " (" + reason + ")" : "";
       if (isWarning) {
-        attributeWarning(attribute.getName(), String.format(
+        return String.format(
             "%s'%s'%s is unexpected here%s; continuing anyway",
             msgPrefix, prerequisite.getLabel(), AliasProvider.printVisibilityChain(prerequisite),
-            msgReason));
+            msgReason);
+      }
+      return String.format(
+          "%s'%s'%s is misplaced here%s", msgPrefix, prerequisite.getLabel(),
+          AliasProvider.printVisibilityChain(prerequisite), msgReason);
+    }
+
+    private void reportBadPrerequisite(Attribute attribute, String targetKind,
+        ConfiguredTarget prerequisite, String reason, boolean isWarning) {
+      String message = badPrerequisiteMessage(targetKind, prerequisite, reason, isWarning);
+      if (isWarning) {
+        attributeWarning(attribute.getName(), message);
       } else {
-        attributeError(attribute.getName(), String.format(
-            "%s'%s'%s is misplaced here%s", msgPrefix, prerequisite.getLabel(),
-            AliasProvider.printVisibilityChain(prerequisite), msgReason));
+        attributeError(attribute.getName(), message);
       }
     }
 
@@ -1803,19 +1813,20 @@ public final class RuleContext extends TargetContext
 
     /**
      * Because some rules still have to use allowedRuleClasses to do rule dependency validation.
-     * We implemented the allowedRuleClasses OR mandatoryProvidersList mechanism. Either condition
-     * is satisfied, we consider the dependency valid.
+     * A dependency is valid if it is from a rule in allowedRuledClasses, OR if all of the providers
+     * in mandatoryNativeProviders AND mandatoryProvidersList are provided by the target.
      */
     private void validateRuleDependency(ConfiguredTarget prerequisite, Attribute attribute) {
       Target prerequisiteTarget = prerequisite.getTarget();
       RuleClass ruleClass = ((Rule) prerequisiteTarget).getRuleClassObject();
-      Boolean allowed = null;
+      HashSet<String> unfulfilledRequirements = new HashSet<>();
 
       if (attribute.getAllowedRuleClassesPredicate() != Predicates.<RuleClass>alwaysTrue()) {
-        allowed = attribute.getAllowedRuleClassesPredicate().apply(ruleClass);
-        if (allowed) {
+        if (attribute.getAllowedRuleClassesPredicate().apply(ruleClass)) {
           return;
         }
+        unfulfilledRequirements.add(badPrerequisiteMessage(prerequisiteTarget.getTargetKind(),
+            prerequisite, "expected " + attribute.getAllowedRuleClassesPredicate(), false));
       }
 
       if (attribute.getAllowedRuleClassesWarningPredicate()
@@ -1828,27 +1839,38 @@ public final class RuleContext extends TargetContext
         }
       }
 
-      if (!attribute.getMandatoryNativeProviders().isEmpty()) {
-        String missing = getMissingMandatoryNativeProviders(prerequisite, attribute);
-        if (missing != null) {
-          attributeError(
-              attribute.getName(),
-              "'" + prerequisite.getLabel() + "' does not have mandatory providers: " + missing);
-          return;
+      // This condition is required; getMissingMandatory[Native]Providers() would be vacuously true
+      // if no providers were mandatory (thus, none are missing), which would cause an early return
+      // below without emitting the error message about the not-allowed rule class if that
+      // requirement was unfulfilled.
+      if (!attribute.getMandatoryNativeProviders().isEmpty()
+          || !attribute.getMandatoryProvidersList().isEmpty()) {
+        boolean hadAllMandatoryProviders = true;
+
+        String missingNativeProviders = getMissingMandatoryNativeProviders(prerequisite, attribute);
+        if (missingNativeProviders != null) {
+          unfulfilledRequirements.add(
+              "'" + prerequisite.getLabel() + "' does not have mandatory providers: "
+                  + missingNativeProviders);
+          hadAllMandatoryProviders = false;
         }
-      }
 
-      if (!attribute.getMandatoryProvidersList().isEmpty()) {
-        String missingMandatoryProviders = getMissingMandatoryProviders(prerequisite, attribute);
-        if (missingMandatoryProviders != null) {
-          attributeError(
-              attribute.getName(),
+        String missingProviders = getMissingMandatoryProviders(prerequisite, attribute);
+        if (missingProviders != null) {
+          unfulfilledRequirements.add(
               "'" + prerequisite.getLabel() + "' does not have mandatory provider "
-                  + missingMandatoryProviders);
+                  + missingProviders);
+          hadAllMandatoryProviders = false;
+        }
+
+        if (hadAllMandatoryProviders) {
+          return;
         }
-      } else if (Boolean.FALSE.equals(allowed)) {
-        reportBadPrerequisite(attribute, prerequisiteTarget.getTargetKind(), prerequisite,
-            "expected " + attribute.getAllowedRuleClassesPredicate(), false);
+      }
+
+      if (!unfulfilledRequirements.isEmpty()) {
+        attributeError(
+            attribute.getName(), StringUtil.joinEnglishList(unfulfilledRequirements, "and"));
       }
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
index 2ac485195d..c53b92af54 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/Attribute.java
@@ -1706,6 +1706,8 @@ public final class Attribute implements Comparable<Attribute> {
     builder.allowedFileTypesForLabels = allowedFileTypesForLabels;
     builder.allowedRuleClassesForLabels = allowedRuleClassesForLabels;
     builder.allowedRuleClassesForLabelsWarning = allowedRuleClassesForLabelsWarning;
+    builder.mandatoryNativeProviders = mandatoryNativeProviders;
+    builder.mandatoryProvidersList = mandatoryProvidersList;
     builder.validityPredicate = validityPredicate;
     builder.condition = condition;
     builder.configTransition = configTransition;
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcRuleClasses.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcRuleClasses.java
index 81c8d9bf5e..e499a3b6cf 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcRuleClasses.java
@@ -682,15 +682,8 @@ public class ObjcRuleClasses {
      */
     static final Iterable<String> ALLOWED_DEPS_RULE_CLASSES =
         ImmutableSet.of(
-            "objc_library",
-            "objc_import",
-            "objc_framework",
-            "objc_proto_library",
-            "j2objc_library",
             "cc_library",
             "cc_inc_library",
-            "ios_framework",
-            "swift_library",
             "experimental_objc_library");
 
     @Override
@@ -736,6 +729,7 @@ public class ObjcRuleClasses {
               attr("deps", LABEL_LIST)
                   .direct_compile_time_input()
                   .allowedRuleClasses(ALLOWED_DEPS_RULE_CLASSES)
+                  .mandatoryNativeProviders(ObjcProvider.class)
                   .allowedFileTypes())
           /* <!-- #BLAZE_RULE($objc_compiling_rule).ATTRIBUTE(runtime_deps) -->
           The list of framework targets that are late loaded at runtime.  They are included in the
@@ -758,6 +752,7 @@ public class ObjcRuleClasses {
               attr("non_propagated_deps", LABEL_LIST)
                   .direct_compile_time_input()
                   .allowedRuleClasses(ALLOWED_DEPS_RULE_CLASSES)
+                  .mandatoryNativeProviders(ObjcProvider.class)
                   .allowedFileTypes())
           /* <!-- #BLAZE_RULE($objc_compiling_rule).ATTRIBUTE(defines) -->
            Extra <code>-D</code> flags to pass to the compiler. They should be in