Move depset order validation from PyCommon to PyStructUtils

The fact that the field is a depset is already an invariant of the provider, so we may as well include the order in that requirement. This is not a breaking change because it's currently enforced by PyCommon. Moving the validation out of PyCommon will make it easier to refactor the "get this field from that dep" helpers out of PyCommon and into a shared PyProviderUtils helper. Work toward bazelbuild#7010. RELNOTES: None PiperOrigin-RevId: 230780935
weixiao-huang · Jan 31, 2019 · c119d2a · c119d2a
1 parent 36c9543
commit c119d2a
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 43 deletions.
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
@@ -234,13 +234,7 @@ private static void collectTransitivePythonSourcesFromDeps(
         try {
           StructImpl info = PyStructUtils.getProvider(dep);
           NestedSet<Artifact> sources = PyStructUtils.getTransitiveSources(info);
-          if (!builder.getOrder().isCompatible(sources.getOrder())) {
-            ruleContext.ruleError(
-                getOrderErrorMessage(
-                    PyStructUtils.TRANSITIVE_SOURCES, builder.getOrder(), sources.getOrder()));
-          } else {
-            builder.addTransitive(sources);
-          }
+          builder.addTransitive(sources);
         } catch (EvalException e) {
           // Either the provider type or field type is bad.
           ruleContext.ruleError(e.getMessage());

diff --git a/src/main/java/com/google/devtools/build/lib/rules/python/PyStructUtils.java b/src/main/java/com/google/devtools/build/lib/rules/python/PyStructUtils.java
@@ -20,6 +20,7 @@
 import com.google.devtools.build.lib.analysis.TransitiveInfoCollection;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.collect.nestedset.NestedSetBuilder;
+import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.packages.StructImpl;
 import com.google.devtools.build.lib.packages.StructProvider;
 import com.google.devtools.build.lib.syntax.EvalException;
@@ -38,8 +39,8 @@ private PyStructUtils() {}
   public static final String PROVIDER_NAME = "py";
 
   /**
-   * Name of field holding a postorder depset of transitive sources (i.e., .py files in srcs and in
-   * srcs of transitive deps).
+   * Name of field holding a postorder-compatible depset of transitive sources (i.e., .py files in
+   * {@code srcs} and in {@code srcs} of transitive {@code deps}).
    */
   public static final String TRANSITIVE_SOURCES = "transitive_sources";
 
@@ -132,7 +133,16 @@ public static NestedSet<Artifact> getTransitiveSources(StructImpl info) throws E
             PROVIDER_NAME,
             TRANSITIVE_SOURCES,
             EvalUtils.getDataTypeNameFromClass(fieldValue.getClass()));
-    return castValue.getSet(Artifact.class);
+    NestedSet<Artifact> unwrappedValue = castValue.getSet(Artifact.class);
+    if (!unwrappedValue.getOrder().isCompatible(Order.COMPILE_ORDER)) {
+      throw new EvalException(
+          /*location=*/ null,
+          String.format(
+              "Incompatible depset order for 'transitive_sources': expected 'default' or "
+                  + "'postorder', but got '%s'",
+              unwrappedValue.getOrder().getSkylarkName()));
+    }
+    return unwrappedValue;
   }
 
   /**

diff --git a/src/test/java/com/google/devtools/build/lib/rules/python/PyStructUtilsTest.java b/src/test/java/com/google/devtools/build/lib/rules/python/PyStructUtilsTest.java
@@ -231,39 +231,15 @@ public void getTransitiveSources_WrongType() {
 
   @Test
   public void getTransitiveSources_OrderMismatch() throws Exception {
-    reporter.removeHandler(failFastHandler);
-    // Depset order mismatches should be caught as rule errors.
-    scratch.file("rules/BUILD");
-    scratch.file(
-        "rules/badorderrule.bzl",
-        "def _badorderrule_impl(ctx):",
-        // Native rules use "compile" / "postorder", so using "preorder" here creates a conflict.
-        "    info = struct(transitive_sources=depset(direct=[], order='preorder'))",
-        "    return struct(py=info)",
-        "",
-        "badorderrule = rule(",
-        "    implementation = _badorderrule_impl",
-        ")");
-    scratch.file(
-        "badordertarget/BUILD",
-        "load('//rules:badorderrule.bzl', 'badorderrule')",
-        "",
-        "badorderrule(",
-        "    name = 'badorderdep',",
-        ")",
-        "py_library(",
-        "    name = 'pylib',",
-        "    srcs = ['pylib.py'],",
-        ")",
-        "py_binary(",
-        "    name = 'pybin',",
-        "    srcs = ['pybin.py'],",
-        "    deps = [':pylib', ':badorderdep'],",
-        ")");
-    getConfiguredTarget("//badordertarget:pybin");
-    assertContainsEvent(
-        "Incompatible order for transitive_sources: expected 'default' or 'postorder', got "
-            + "'preorder'");
+    NestedSet<Artifact> sources = NestedSetBuilder.emptySet(Order.NAIVE_LINK_ORDER);
+    StructImpl info =
+        makeStruct(
+            ImmutableMap.of(
+                PyStructUtils.TRANSITIVE_SOURCES, SkylarkNestedSet.of(Artifact.class, sources)));
+    assertThrowsEvalExceptionContaining(
+        () -> PyStructUtils.getTransitiveSources(info),
+        "Incompatible depset order for 'transitive_sources': expected 'default' or 'postorder', "
+            + "but got 'preorder'");
   }
 
   @Test