Python provider is now available in Skylark

Using mandatoryProvidersList to validate python rules' dependency.
Added a SkylarkProvider named 'py' which is a SkylarkClassObject in Java and a
struct in Skylark. Native python rule and Skylark python rule should have this provider
so that they can depend on each other.

RELNOTES[NEW]: Native python rule can depend on skylark rule as long as skylark
rule provides 'py' provider.

--
MOS_MIGRATED_REVID=116241504

8 files changed, 154 insertions, 96 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 9fd4fd4666..8fa5cb55a2 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
@@ -16,6 +16,7 @@ package com.google.devtools.build.lib.analysis;
 
 import com.google.common.base.Joiner;
 import com.google.common.base.Predicate;
+import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
@@ -1557,16 +1558,10 @@ public final class RuleContext extends TargetContext
         }
       }
 
-      if (attribute.isStrictLabelCheckingEnabled()) {
-        if (prerequisiteTarget instanceof Rule) {
-          RuleClass ruleClass = ((Rule) prerequisiteTarget).getRuleClassObject();
-          if (!attribute.getAllowedRuleClassesPredicate().apply(ruleClass)) {
-            boolean allowedWithWarning = attribute.getAllowedRuleClassesWarningPredicate()
-                .apply(ruleClass);
-            reportBadPrerequisite(attribute, prerequisiteTarget.getTargetKind(), prerequisiteLabel,
-                "expected " + attribute.getAllowedRuleClassesPredicate(), allowedWithWarning);
-          }
-        } else if (prerequisiteTarget instanceof FileTarget) {
+      if (prerequisiteTarget instanceof Rule) {
+        validateRuleDependency(prerequisite, attribute);
+      } else if (prerequisiteTarget instanceof FileTarget) {
+        if (attribute.isStrictLabelCheckingEnabled()) {
           if (!attribute.getAllowedFileTypesPredicate()
               .apply(((FileTarget) prerequisiteTarget).getFilename())) {
             if (prerequisiteTarget instanceof InputFile
@@ -1660,10 +1655,10 @@ public final class RuleContext extends TargetContext
       }
     }
 
-    private void validateMandatoryProviders(ConfiguredTarget prerequisite, Attribute attribute) {
+    private String getMissingMandatoryProviders(ConfiguredTarget prerequisite, Attribute attribute){
       List<ImmutableSet<String>> mandatoryProvidersList = attribute.getMandatoryProvidersList();
       if (mandatoryProvidersList.isEmpty()) {
-        return;
+        return null;
       }
       List<List<String>> missingProvidersList = new ArrayList<>();
       for (ImmutableSet<String> providers : mandatoryProvidersList) {
@@ -1674,7 +1669,7 @@ public final class RuleContext extends TargetContext
           }
         }
         if (missing.isEmpty()) {
-          return;
+          return null;
         } else {
           missingProvidersList.add(missing);
         }
@@ -1691,16 +1686,55 @@ public final class RuleContext extends TargetContext
         missingProviders.append("'")
                         .append((providers.size() > 1) ? "]" : "");
       }
-      attributeError(
-          attribute.getName(),
-          "'" + prerequisite.getLabel() + "' does not have mandatory provider "
-                  + missingProviders);
+      return missingProviders.toString();
+    }
+
+    /**
+     * 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.
+     */
+    private void validateRuleDependency(ConfiguredTarget prerequisite, Attribute attribute) {
+      Target prerequisiteTarget = prerequisite.getTarget();
+      Label prerequisiteLabel = prerequisiteTarget.getLabel();
+      RuleClass ruleClass = ((Rule) prerequisiteTarget).getRuleClassObject();
+      Boolean allowed = null;
+      Boolean allowedWithWarning = null;
+
+      if (attribute.getAllowedRuleClassesPredicate() != Predicates.<RuleClass>alwaysTrue()) {
+        allowed = attribute.getAllowedRuleClassesPredicate().apply(ruleClass);
+        if (allowed) {
+          return;
+        }
+      }
+
+      if (attribute.getAllowedRuleClassesWarningPredicate()
+              != Predicates.<RuleClass>alwaysTrue()) {
+        allowedWithWarning = attribute.getAllowedRuleClassesWarningPredicate().apply(ruleClass);
+        if (allowedWithWarning) {
+          reportBadPrerequisite(attribute, prerequisiteTarget.getTargetKind(), prerequisiteLabel,
+                  "expected " + attribute.getAllowedRuleClassesPredicate(), true);
+          return;
+        }
+      }
+
+      if (!attribute.getMandatoryProvidersList().isEmpty()) {
+        String missingMandatoryProviders = getMissingMandatoryProviders(prerequisite, attribute);
+        if (missingMandatoryProviders != null) {
+          attributeError(
+                  attribute.getName(),
+                  "'" + prerequisite.getLabel() + "' does not have mandatory provider "
+                          + missingMandatoryProviders);
+        }
+      } else if (Boolean.FALSE.equals(allowed)) {
+        reportBadPrerequisite(attribute, prerequisiteTarget.getTargetKind(), prerequisiteLabel,
+                "expected " + attribute.getAllowedRuleClassesPredicate(), false);
+      }
     }
 
     private void validateDirectPrerequisite(Attribute attribute, ConfiguredTarget prerequisite) {
       validateDirectPrerequisiteType(prerequisite, attribute);
       validateDirectPrerequisiteFileTypes(prerequisite, attribute);
-      validateMandatoryProviders(prerequisite, attribute);
       if (attribute.performPrereqValidatorCheck()) {
         prerequisiteValidator.validate(this, prerequisite, attribute);
       }
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviders.java b/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviders.java
index 53708cf243..1f8f1280a0 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviders.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/SkylarkProviders.java
@@ -16,6 +16,8 @@ package com.google.devtools.build.lib.analysis;
 import com.google.common.collect.ImmutableCollection;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
+import com.google.devtools.build.lib.syntax.EvalException;
+import com.google.devtools.build.lib.syntax.SkylarkType;
 import com.google.devtools.build.lib.util.Preconditions;
 
 import java.util.HashSet;
@@ -49,6 +51,18 @@ public final class SkylarkProviders implements TransitiveInfoProvider {
   }
 
   /**
+   * Returns a Skylark provider and try to cast it into the specified type
+   */
+  public <TYPE> TYPE getValue(String key, Class<TYPE> type) throws EvalException {
+    Object obj = skylarkProviders.get(key);
+    if (obj == null) {
+      return null;
+    }
+    SkylarkType.checkType(obj, type, key);
+    return type.cast(obj);
+  }
+
+  /**
    * Merges skylark providers. The set of providers must be disjoint.
    *
    * @param providers providers to merge {@code this} with.
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyRuleClasses.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyRuleClasses.java
index 7c0fc717b4..35b23497d0 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyRuleClasses.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/python/BazelPyRuleClasses.java
@@ -30,6 +30,7 @@ import com.google.devtools.build.lib.bazel.rules.cpp.BazelCppRuleClasses;
 import com.google.devtools.build.lib.packages.RuleClass;
 import com.google.devtools.build.lib.packages.RuleClass.Builder.RuleClassType;
 import com.google.devtools.build.lib.packages.TriState;
+import com.google.devtools.build.lib.rules.python.PyCommon;
 import com.google.devtools.build.lib.rules.python.PyRuleClasses;
 import com.google.devtools.build.lib.rules.python.PythonVersion;
 import com.google.devtools.build.lib.util.FileType;
@@ -40,11 +41,6 @@ import com.google.devtools.build.lib.util.FileType;
 public final class BazelPyRuleClasses {
   public static final FileType PYTHON_SOURCE = FileType.of(".py");
 
-  public static final String[] ALLOWED_RULES_IN_DEPS = new String[] {
-      "py_binary",
-      "py_library",
-  };
-
   /**
    * Base class for Python rule definitions.
    */
@@ -63,7 +59,7 @@ public final class BazelPyRuleClasses {
           <a href="c-cpp.html#cc_library"><code>cc_library</code></a> rules,
           <!-- #END_BLAZE_RULE.ATTRIBUTE --> */
           .override(builder.copy("deps")
-              .allowedRuleClasses(ALLOWED_RULES_IN_DEPS)
+              .mandatoryProviders(ImmutableList.of(PyCommon.PYTHON_SKYLARK_PROVIDER_NAME))
               .allowedFileTypes())
           /* <!-- #BLAZE_RULE($base_py).ATTRIBUTE(imports) -->
           List of import directories to be added to the <code>PYTHONPATH</code>.
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 f8923e7565..1db5c46136 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
@@ -713,7 +713,9 @@ public final class Attribute implements Comparable<Attribute> {
     }
 
     public Builder<TYPE> mandatoryProviders(Iterable<String> providers) {
-      mandatoryProvidersList(ImmutableList.of(providers));
+      if (providers.iterator().hasNext()) {
+        mandatoryProvidersList(ImmutableList.of(providers));
+      }
       return this;
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java
index ae048a0a03..ab8d490e4b 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyBinary.java
@@ -76,7 +76,7 @@ public abstract class PyBinary implements RuleConfiguredTargetFactory {
     Runfiles commonRunfiles = collectCommonRunfiles(ruleContext, common, semantics);
 
     Runfiles.Builder defaultRunfilesBuilder = new Runfiles.Builder(ruleContext.getWorkspaceName())
-        .merge(commonRunfiles);
+                                    .merge(commonRunfiles);
     semantics.collectDefaultRunfilesForBinary(ruleContext, defaultRunfilesBuilder);
     Runfiles defaultRunfiles = defaultRunfilesBuilder.build();
 
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java
index 9b6813fad5..eff1239649 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java
@@ -15,6 +15,7 @@ package com.google.devtools.build.lib.rules.python;
 
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.ActionOwner;
@@ -30,6 +31,7 @@ import com.google.devtools.build.lib.analysis.PseudoAction;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTarget.Mode;
 import com.google.devtools.build.lib.analysis.RuleConfiguredTargetBuilder;
 import com.google.devtools.build.lib.analysis.RuleContext;
+import com.google.devtools.build.lib.analysis.SkylarkProviders;
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.analysis.Util;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
@@ -44,6 +46,11 @@ import com.google.devtools.build.lib.rules.cpp.CppFileTypes;
 import com.google.devtools.build.lib.rules.test.InstrumentedFilesCollector;
 import com.google.devtools.build.lib.rules.test.InstrumentedFilesCollector.LocalMetadataCollector;
 import com.google.devtools.build.lib.rules.test.InstrumentedFilesProvider;
+import com.google.devtools.build.lib.syntax.ClassObject.SkylarkClassObject;
+import com.google.devtools.build.lib.syntax.EvalException;
+import com.google.devtools.build.lib.syntax.EvalUtils;
+import com.google.devtools.build.lib.syntax.SkylarkNestedSet;
+import com.google.devtools.build.lib.syntax.SkylarkType;
 import com.google.devtools.build.lib.syntax.Type;
 import com.google.devtools.build.lib.util.FileType;
 import com.google.devtools.build.lib.util.Preconditions;
@@ -61,6 +68,10 @@ import java.util.UUID;
  */
 public final class PyCommon {
 
+  public static final String PYTHON_SKYLARK_PROVIDER_NAME = "py";
+  public static final String TRANSITIVE_PYTHON_SRCS = "transitive_sources";
+  public static final String IS_USING_SHARED_LIBRARY = "uses_shared_libraries";
+
   private static final LocalMetadataCollector METADATA_COLLECTOR = new LocalMetadataCollector() {
     @Override
     public void collectMetadataArtifacts(Iterable<Artifact> artifacts,
@@ -125,13 +136,15 @@ public final class PyCommon {
 
   public void addCommonTransitiveInfoProviders(RuleConfiguredTargetBuilder builder,
       PythonSemantics semantics, NestedSet<Artifact> filesToBuild) {
-    PythonSourcesProvider sourcesProvider =
-        new PythonSourcesProvider(transitivePythonSources, usesSharedLibraries());
+
+    SkylarkClassObject sourcesProvider =
+        new SkylarkClassObject(ImmutableMap.<String, Object>of(
+            TRANSITIVE_PYTHON_SRCS, SkylarkNestedSet.of(Artifact.class, transitivePythonSources),
+            IS_USING_SHARED_LIBRARY, usesSharedLibraries()), "No such attribute '%s'");
     builder
         .add(InstrumentedFilesProvider.class, InstrumentedFilesCollector.collect(ruleContext,
             semantics.getCoverageInstrumentationSpec(), METADATA_COLLECTOR, filesToBuild))
-        .add(PythonSourcesProvider.class, sourcesProvider)
-        .addSkylarkTransitiveInfo(PythonSourcesProvider.SKYLARK_NAME, sourcesProvider)
+        .addSkylarkTransitiveInfo(PYTHON_SKYLARK_PROVIDER_NAME, sourcesProvider)
         // Python targets are not really compilable. The best we can do is make sure that all
         // generated source files are ready.
         .addOutputGroup(OutputGroupProvider.FILES_TO_COMPILE, transitivePythonSources)
@@ -260,12 +273,44 @@ public final class PyCommon {
     return ruleContext.getPrerequisites("deps", Mode.TARGET);
   }
 
+  private NestedSet<Artifact> getTransitivePythonSourcesFromSkylarkProvider(
+          TransitiveInfoCollection dep) {
+    SkylarkClassObject pythonSkylarkProvider = null;
+    SkylarkProviders skylarkProviders = dep.getProvider(SkylarkProviders.class);
+    try {
+      if (skylarkProviders != null) {
+        pythonSkylarkProvider = skylarkProviders.getValue(PYTHON_SKYLARK_PROVIDER_NAME,
+                SkylarkClassObject.class);
+      }
+
+      if (pythonSkylarkProvider != null) {
+        Object sourceFiles = pythonSkylarkProvider.getValue(TRANSITIVE_PYTHON_SRCS);
+        String errorType;
+        if (sourceFiles == null) {
+          errorType = "null";
+        } else {
+          errorType = EvalUtils.getDataTypeNameFromClass(sourceFiles.getClass());
+        }
+        String errorMsg = "Illegal Argument: attribute '%s' in provider '%s' is "
+                + "of unexpected type. Should be a set, but got a '%s'";
+        NestedSet<Artifact> pythonSourceFiles = SkylarkType.cast(
+                sourceFiles, SkylarkNestedSet.class, Artifact.class, null,
+                errorMsg, TRANSITIVE_PYTHON_SRCS, PYTHON_SKYLARK_PROVIDER_NAME, errorType)
+                .getSet(Artifact.class);
+        return pythonSourceFiles;
+      }
+    } catch (EvalException e) {
+      ruleContext.ruleError(e.getMessage());
+    }
+    return null;
+  }
+
   private void collectTransitivePythonSourcesFrom(
       Iterable<? extends TransitiveInfoCollection> deps, NestedSetBuilder<Artifact> builder) {
     for (TransitiveInfoCollection dep : deps) {
-      if (dep.getProvider(PythonSourcesProvider.class) != null) {
-        PythonSourcesProvider provider = dep.getProvider(PythonSourcesProvider.class);
-        builder.addTransitive(provider.getTransitivePythonSources());
+      NestedSet<Artifact> pythonSourceFiles = getTransitivePythonSourcesFromSkylarkProvider(dep);
+      if (pythonSourceFiles != null) {
+        builder.addTransitive(pythonSourceFiles);
       } else {
         // TODO(bazel-team): We also collect .py source files from deps (e.g. for proto_library
         // rules). Rules should implement PythonSourcesProvider instead.
@@ -381,9 +426,14 @@ public final class PyCommon {
   }
 
   public boolean usesSharedLibraries() {
-    return checkForSharedLibraries(Iterables.concat(
-        ruleContext.getPrerequisites("deps", Mode.TARGET),
-        ruleContext.getPrerequisites("data", Mode.DATA)));
+    try {
+      return checkForSharedLibraries(Iterables.concat(
+              ruleContext.getPrerequisites("deps", Mode.TARGET),
+              ruleContext.getPrerequisites("data", Mode.DATA)));
+    } catch (EvalException e) {
+      ruleContext.ruleError(e.getMessage());
+      return false;
+    }
   }
 
   protected static final ResourceSet PY_COMPILE_RESOURCE_SET =
@@ -453,11 +503,18 @@ public final class PyCommon {
   /**
    * Returns true if this target has an .so file in its transitive dependency closure.
    */
-  public static boolean checkForSharedLibraries(Iterable<TransitiveInfoCollection> deps) {
+  public static boolean checkForSharedLibraries(Iterable<TransitiveInfoCollection> deps)
+          throws EvalException{
     for (TransitiveInfoCollection dep : deps) {
-      PythonSourcesProvider provider = dep.getProvider(PythonSourcesProvider.class);
+      SkylarkProviders providers = dep.getProvider(SkylarkProviders.class);
+      SkylarkClassObject provider = null;
+      if (providers != null) {
+        provider = providers.getValue(PYTHON_SKYLARK_PROVIDER_NAME,
+                SkylarkClassObject.class);
+      }
       if (provider != null) {
-        if (provider.usesSharedLibraries()) {
+        Boolean isUsingSharedLibrary = provider.getValue(IS_USING_SHARED_LIBRARY, Boolean.class);
+        if (Boolean.TRUE.equals(isUsingSharedLibrary)) {
           return true;
         }
       } else if (FileType.contains(
diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PythonSourcesProvider.java b/src/main/java/com/google/devtools/build/lib/rules/python/PythonSourcesProvider.java
deleted file mode 100644
index b8189875b5..0000000000
--- a/src/main/java/com/google/devtools/build/lib/rules/python/PythonSourcesProvider.java
+++ /dev/null
@@ -1,57 +0,0 @@
-// Copyright 2014 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.rules.python;
-
-import com.google.devtools.build.lib.actions.Artifact;
-import com.google.devtools.build.lib.analysis.TransitiveInfoProvider;
-import com.google.devtools.build.lib.collect.nestedset.NestedSet;
-import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
-import com.google.devtools.build.lib.skylarkinterface.SkylarkCallable;
-import com.google.devtools.build.lib.skylarkinterface.SkylarkModule;
-
-/**
- * A provider interface for configured targets that provide source files to
- * Python targets.
- */
-@Immutable
-@SkylarkModule(name = "PythonSourcesProvider", doc = "")
-public final class PythonSourcesProvider implements TransitiveInfoProvider {
-  /** The name of the field in Skylark used to access this class. */
-  public static final String SKYLARK_NAME = "py";
-
-  private final NestedSet<Artifact> transitivePythonSources;
-  private final boolean usesSharedLibraries;
-
-  public PythonSourcesProvider(NestedSet<Artifact> transitivePythonSources,
-      boolean usesSharedLibraries) {
-    this.transitivePythonSources = transitivePythonSources;
-    this.usesSharedLibraries = usesSharedLibraries;
-  }
-
-  /**
-   * Returns the Python sources in the transitive closure of this target.
-   */
-  @SkylarkCallable(
-      name = "transitive_sources", doc = "The transitive set of Python sources", structField = true)
-  public NestedSet<Artifact> getTransitivePythonSources() {
-    return transitivePythonSources;
-  }
-
-  /**
-   * Returns true if this target transitively depends on any shared libraries.
-   */
-  public boolean usesSharedLibraries() {
-    return usesSharedLibraries;
-  }
-}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ClassObject.java b/src/main/java/com/google/devtools/build/lib/syntax/ClassObject.java
index 69d821755c..cb13c65a30 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ClassObject.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ClassObject.java
@@ -100,6 +100,18 @@ public interface ClassObject {
       return values.get(name);
     }
 
+    /**
+     *  Returns a value and try to cast it into specified type
+     */
+    public <TYPE> TYPE getValue(String key, Class<TYPE> type) throws EvalException {
+      Object obj = values.get(key);
+      if (obj == null) {
+        return null;
+      }
+      SkylarkType.checkType(obj, type, key);
+      return type.cast(obj);
+    }
+
     @Override
     public ImmutableCollection<String> getKeys() {
       return values.keySet();