From f48d46c75c942f13c85060c3ea61d983adf8384c Mon Sep 17 00:00:00 2001
From: twerth <twerth@google.com>
Date: Fri, 11 Dec 2020 00:34:21 -0800
Subject: [PATCH] Automated rollback of commit
 b654a974876f3fd915a1e5118ebad57b1d55984f.

*** Reason for rollback ***

Needs a more careful rollout.

PiperOrigin-RevId: 346948380
---
 .../lib/packages/StarlarkNativeModule.java    | 33 ++++++++++---------
 .../StarlarkNativeModuleApi.java              |  3 +-
 .../lib/starlark/StarlarkRuleContextTest.java | 10 ++----
 3 files changed, 20 insertions(+), 26 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java
index 0345fede025b52..d7cfe1075abb81 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/StarlarkNativeModule.java
@@ -352,12 +352,12 @@ private static Object starlarkifyValue(Mutability mu, Object val, Package pkg)
         if (elt == null) {
           continue;
         }
+
         l.add(elt);
       }
 
       return Tuple.copyOf(l);
     }
-
     if (val instanceof Map) {
       Dict.Builder<Object, Object> m = Dict.builder();
       for (Map.Entry<?, ?> e : ((Map<?, ?>) val).entrySet()) {
@@ -372,7 +372,6 @@ private static Object starlarkifyValue(Mutability mu, Object val, Package pkg)
       }
       return m.build(mu);
     }
-
     if (val.getClass().isAnonymousClass()) {
       // Computed defaults. They will be represented as
       // "deprecation": com.google.devtools.build.lib.analysis.BaseRuleClasses$2@6960884a,
@@ -386,22 +385,24 @@ private static Object starlarkifyValue(Mutability mu, Object val, Package pkg)
       return null;
     }
 
-    if (val instanceof BuildType.SelectorList) {
-      List<Object> selectors = new ArrayList<>();
-      for (BuildType.Selector<?> selector : ((BuildType.SelectorList<?>) val).getSelectors()) {
-        selectors.add(
-            new SelectorValue(
-                ((Map<?, ?>) starlarkifyValue(mu, selector.getEntries(), pkg)),
-                selector.getNoMatchError()));
-      }
-      try {
-        return SelectorList.of(selectors);
-      } catch (EvalException e) {
-        throw new NotRepresentableException(e.getMessage());
-      }
+    if (val instanceof StarlarkValue) {
+      return val;
     }
 
-    if (val instanceof StarlarkValue) {
+    if (val instanceof BuildType.SelectorList) {
+      // This is terrible:
+      //  1) this value is opaque, and not a BUILD value, so it cannot be used in rule arguments
+      //  2) its representation has a pointer address, so it breaks hermeticity.
+      //
+      // Even though this is clearly imperfect, we return this value because otherwise
+      // native.rules() fails if there is any rule using a select() in the BUILD file.
+      //
+      // To remedy this, we should return a SelectorList. To do so, we have to
+      // 1) recurse into the Selector contents of SelectorList, so those values are Starlarkified
+      //    too
+      // 2) get the right Class<?> value. We could probably get at that by looking at
+      //    ((SelectorList)val).getSelectors().first().getEntries().first().getClass().
+
       return val;
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkNativeModuleApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkNativeModuleApi.java
index 298322af2113cb..c3a2470a15d476 100644
--- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkNativeModuleApi.java
+++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/StarlarkNativeModuleApi.java
@@ -105,8 +105,7 @@ Sequence<?> glob(
               + " package.</li>" //
               + "<li>Lists are represented as tuples, and dicts are converted to new, mutable"
               + " dicts. Their elements are recursively converted in the same fashion.</li>" //
-              + "<li><code>select</code> values are returned with their contents transformed as " //
-              + "described above.</li>" //
+              + "<li><code>select</code> values are returned as is." //
               + "<li>Attributes for which no value was specified during rule instantiation and"
               + " whose default value is computed are excluded from the result. (Computed defaults"
               + " cannot be computed until the analysis phase.).</li>" //
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java
index f065c8da99cbf8..a19f514ff745a8 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleContextTest.java
@@ -561,21 +561,15 @@ public void existingRuleWithSelect() throws Exception {
         "test/existing_rule.bzl",
         "def macro():",
         "  s = select({'//foo': ['//bar']})",
-        "  print('Passed: ' + repr(s))",
         "  native.cc_library(name = 'x', srcs = s)",
-        "  print('Returned: ' + repr(native.existing_rule('x')['srcs']))",
-        // The value returned here should round-trip fine.
-        "  native.cc_library(name = 'y', srcs = native.existing_rule('x')['srcs'])");
+        "  print(native.existing_rule('x')['srcs'])");
     scratch.file(
         "test/BUILD",
         "load('//test:existing_rule.bzl', 'macro')",
         "macro()",
         "cc_library(name = 'a', srcs = [])");
     getConfiguredTarget("//test:a");
-    assertContainsEvent("Passed: select({\"//foo\": [\"//bar\"]}");
-    // The short labels are now in their canonical form, and the sequence is represented as
-    // tuple instead of list, but the meaning is unchanged.
-    assertContainsEvent("Returned: select({\"//foo:foo\": (\"//bar:bar\",)}");
+    assertContainsEvent("select({Label(\"//foo:foo\"): [Label(\"//bar:bar\")]})");
   }
 
   @Test