Fix wrapping of GlobList into SkylarkList

Make sure to wrap a GlobList into a MutableList in convertToSkylark.
Indeed, GlobList is somehow a SkylarkValue, but is not actually acceptable
as a Skylark value. Until this is cleaned up, tweak convertToSkylark.

Also, robustify ClassObject by eagerly calling convertToSkylark on values.

--
MOS_MIGRATED_REVID=113421790

3 files changed, 50 insertions, 5 deletions

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 359f4e4f18..69d821755c 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
@@ -75,17 +75,26 @@ public interface ClassObject {
      * exactly one '%s' parameter to substitute the struct field name.
      */
     public SkylarkClassObject(Map<String, Object> values, String errorMessage) {
-      this.values = ImmutableMap.copyOf(values);
+      this.values = copyValues(values);
       this.creationLoc = null;
       this.errorMessage = Preconditions.checkNotNull(errorMessage);
     }
 
     public SkylarkClassObject(Map<String, Object> values, Location creationLoc) {
-      this.values = ImmutableMap.copyOf(values);
+      this.values = copyValues(values);
       this.creationLoc = Preconditions.checkNotNull(creationLoc);
       this.errorMessage = DEFAULT_ERROR_MESSAGE;
     }
 
+    // Ensure that values are all acceptable to Skylark before to stuff them in a ClassObject
+    private ImmutableMap<String, Object> copyValues(Map<String, Object> values) {
+      ImmutableMap.Builder<String, Object> builder = ImmutableMap.builder();
+      for (Map.Entry<String, Object> e : values.entrySet()) {
+        builder.put(e.getKey(), SkylarkType.convertToSkylark(e.getValue(), null));
+      }
+      return builder.build();
+    }
+
     @Override
     public Object getValue(String name) {
       return values.get(name);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java
index a90ba7e975..3f4d6410cd 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java
@@ -714,12 +714,12 @@ public abstract class SkylarkType implements Serializable {
    * Converts an object to a Skylark-compatible type if possible.
    */
   public static Object convertToSkylark(Object object, @Nullable Environment env) {
+    if (object instanceof List && !(object instanceof SkylarkList)) {
+      return new MutableList<>((List<?>) object, env);
+    }
     if (object instanceof SkylarkValue) {
       return object;
     }
-    if (object instanceof List) {
-      return new MutableList<>((List<?>) object, env);
-    }
     if (object instanceof Map) {
       return SkylarkDict.<Object, Object>copyOf(env, (Map<?, ?>) object);
     }
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
index 68178db8ec..10c7a6c685 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java
@@ -26,6 +26,7 @@ import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
+import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.Runfiles;
 import com.google.devtools.build.lib.analysis.actions.FileWriteAction;
 import com.google.devtools.build.lib.analysis.actions.SpawnAction;
@@ -954,6 +955,41 @@ public class SkylarkRuleImplementationFunctionsTest extends SkylarkTestCase {
     assertContainsEvent("native.glob() can only be called during the loading phase");
   }
 
+  @Test
+  public void testImplicitOutputsFromGlob() throws Exception {
+    scratch.file("test/glob.bzl",
+        "def _impl(ctx):",
+        "  outs = ctx.outputs",
+        "  for i in ctx.attr.srcs:",
+        "    o = getattr(outs, 'foo_' + i.label.name)",
+        "    ctx.file_action(",
+        "      output = o,",
+        "      content = 'hoho')",
+        "",
+        "def _foo(attr_map):",
+        "  outs = {}",
+        "  for i in attr_map.srcs:",
+        "    outs['foo_' + i.name] = i.name + '.out'",
+        "  return outs",
+        "",
+        "glob_rule = rule(",
+        "    attrs = {",
+        "        'srcs': attr.label_list(allow_files = True),",
+        "    },",
+        "    outputs = _foo,",
+        "    implementation = _impl,",
+        ")");
+    scratch.file("test/a.bar", "a");
+    scratch.file("test/b.bar", "b");
+    scratch.file("test/BUILD",
+        "load('/test/glob', 'glob_rule')",
+        "glob_rule(name = 'my_glob', srcs = glob(['*.bar']))");
+    ConfiguredTarget ct = getConfiguredTarget("//test:my_glob");
+    assertThat(ct).isNotNull();
+    assertThat(getGeneratingAction(getBinArtifact("a.bar.out", ct))).isNotNull();
+    assertThat(getGeneratingAction(getBinArtifact("b.bar.out", ct))).isNotNull();
+  }
+
   private void setupThrowFunction(BuiltinFunction func) throws Exception {
     throwFunction = func;
     throwFunction.configure(