Fix rules that provide both 'executable' and 'files'

PiperOrigin-RevId: 161821800

src/main/java/com/google/devtools/build/lib/rules/SkylarkRuleConfiguredTargetUtil.java

@@ -421,18 +421,15 @@ private static void addSimpleProviders(
       Runfiles defaultRunfiles)
       throws EvalException {
 
-    // TODO(bazel-team) if both 'files' and 'executable' are provided 'files' override 'executalbe'
-    NestedSetBuilder<Artifact> filesToBuild =
-        NestedSetBuilder.<Artifact>stableOrder().addAll(ruleContext.getOutputArtifacts());
+    NestedSetBuilder<Artifact> filesToBuild = NestedSetBuilder.<Artifact>stableOrder()
+        .addAll(ruleContext.getOutputArtifacts());
     if (executable != null) {
       filesToBuild.add(executable);
     }
-    builder.setFilesToBuild(filesToBuild.build());
-
     if (files != null) {
-      // If we specify files_to_build we don't have the executable in it by default.
-      builder.setFilesToBuild(files.getSet(Artifact.class));
+      filesToBuild.addTransitive(files.getSet(Artifact.class));
     }
+    builder.setFilesToBuild(filesToBuild.build());
 
     if ((statelessRunfiles != null) && (dataRunfiles != null || defaultRunfiles != null)) {
       throw new EvalException(loc, "Cannot specify the provider 'runfiles' "

src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleImplementationFunctionsTest.java

@@ -704,16 +704,16 @@ public void testRunfilesBadSetGenericType() throws Exception {
   @Test
   public void testRunfilesBadMapGenericType() throws Exception {
     checkErrorContains(
-        "expected type 'string' for 'symlinks' key " + "but got type 'int' instead",
+        "expected type 'string' for 'symlinks' key but got type 'int' instead",
         "ruleContext.runfiles(symlinks = {123: ruleContext.files.srcs[0]})");
     checkErrorContains(
-        "expected type 'File' for 'symlinks' value " + "but got type 'int' instead",
+        "expected type 'File' for 'symlinks' value but got type 'int' instead",
         "ruleContext.runfiles(symlinks = {'some string': 123})");
     checkErrorContains(
-        "expected type 'string' for 'root_symlinks' key " + "but got type 'int' instead",
+        "expected type 'string' for 'root_symlinks' key but got type 'int' instead",
         "ruleContext.runfiles(root_symlinks = {123: ruleContext.files.srcs[0]})");
     checkErrorContains(
-        "expected type 'File' for 'root_symlinks' value " + "but got type 'int' instead",
+        "expected type 'File' for 'root_symlinks' value but got type 'int' instead",
         "ruleContext.runfiles(root_symlinks = {'some string': 123})");
   }
 
@@ -844,7 +844,7 @@ public void testStructPlusArtifactErrorMessage() throws Exception {
   public void testNoSuchProviderErrorMessage() throws Exception {
     checkErrorContains(
         createRuleContext("//foo:bar"),
-        "target (rule class of 'java_library') " + "doesn't have provider 'my_provider'.",
+        "target (rule class of 'java_library') doesn't have provider 'my_provider'.",
         "ruleContext.attr.srcs[0].my_provider");
   }
 
@@ -1345,7 +1345,7 @@ public void testDeclaredProvidersWrongKey() throws Exception {
     } catch (AssertionError expected) {
       assertThat(expected)
           .hasMessageThat()
-          .contains("Object of type Target doesn't " + "contain declared provider unused_provider");
+          .contains("Object of type Target doesn't contain declared provider unused_provider");
     }
   }
 
@@ -1391,7 +1391,7 @@ public void testDeclaredProvidersInvalidKey() throws Exception {
       assertThat(expected)
           .hasMessageThat()
           .contains(
-              "Type Target only supports indexing " + "by object constructors, got string instead");
+              "Type Target only supports indexing by object constructors, got string instead");
     }
   }
 
@@ -1421,7 +1421,7 @@ public void testDeclaredProvidersFileTarget() throws Exception {
     } catch (AssertionError expected) {
       assertThat(expected)
           .hasMessageThat()
-          .contains("Object of type Target doesn't " + "contain declared provider unused_provider");
+          .contains("Object of type Target doesn't contain declared provider unused_provider");
     }
   }
 
@@ -1504,7 +1504,7 @@ public void testDeclaredProvidersInOperatorInvalidKey() throws Exception {
       assertThat(expected)
           .hasMessageThat()
           .contains(
-              "Type Target only supports querying by object " + "constructors, got string instead");
+              "Type Target only supports querying by object constructors, got string instead");
     }
   }
 
@@ -1728,6 +1728,51 @@ public void testBuiltInFunctionAsRuleImplementation() throws Exception {
     getConfiguredTarget("//test:silly");
   }
 
+  @Test
+  public void testExecutable() throws Exception {
+    scratch.file(
+        "test/foo.bzl",
+        "def _impl(ctx):",
+        "    return DefaultInfo(",
+        "        files=depset(ctx.files.one),",
+        "        executable=ctx.files.two[0],",
+        "    )",
+        "foo_rule = rule(",
+        "    implementation = _impl,",
+        "    attrs = {",
+        "       'one': attr.label_list(allow_files=True),",
+        "       'two': attr.label_list(allow_files=True),",
+        "    }",
+        ")"
+    );
+    scratch.file(
+        "test/bar.bzl",
+        "def _impl(ctx):",
+        "    provider = ctx.attr.deps[0][DefaultInfo]",
+        "    return struct(",
+        "        provider = provider,",
+        "        files_to_run = provider.files_to_run,",
+        "    )",
+        "bar_rule = rule(",
+        "    implementation = _impl,",
+        "    attrs = {",
+        "       'deps': attr.label_list(allow_files=True),",
+        "    }",
+        ")");
+    scratch.file(
+        "test/BUILD",
+        "load(':foo.bzl', 'foo_rule')",
+        "load(':bar.bzl', 'bar_rule')",
+        "foo_rule(name = 'dep_rule', one = ['one.txt'], two = ['two.txt'])",
+        "bar_rule(name = 'my_rule', deps = [':dep_rule'])");
+    ConfiguredTarget configuredTarget = getConfiguredTarget("//test:my_rule");
+
+    List<Artifact> filesToRun = ((FilesToRunProvider) configuredTarget.get("files_to_run"))
+        .getFilesToRun().toList();
+    assertThat(filesToRun.get(0).getFilename()).isEqualTo("one.txt");
+    assertThat(filesToRun.get(1).getFilename()).isEqualTo("two.txt");
+  }
+
   private void setupThrowFunction(BuiltinFunction func) throws Exception {
     throwFunction = func;
     throwFunction.configure(