Always collect FileProvider's filesToBuild as data runfiles

src/main/java/com/google/devtools/build/lib/analysis/Runfiles.java

@@ -935,7 +935,10 @@ public Builder add(
     /** Collects runfiles from data dependencies of a target. */
     @CanIgnoreReturnValue
     public Builder addDataDeps(RuleContext ruleContext) {
-      addTargets(getPrerequisites(ruleContext, "data"), RunfilesProvider.DATA_RUNFILES);
+      addTargets(
+          getPrerequisites(ruleContext, "data"),
+          RunfilesProvider.DATA_RUNFILES,
+          ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData());
       return this;
     }
 
@@ -952,16 +955,20 @@ public Builder addNonDataDeps(
     @CanIgnoreReturnValue
     public Builder addTargets(
         Iterable<? extends TransitiveInfoCollection> targets,
-        Function<TransitiveInfoCollection, Runfiles> mapping) {
+        Function<TransitiveInfoCollection, Runfiles> mapping,
+        boolean alwaysIncludeFilesToBuildInData) {
       for (TransitiveInfoCollection target : targets) {
-        addTarget(target, mapping);
+        addTarget(target, mapping, alwaysIncludeFilesToBuildInData);
       }
       return this;
     }
 
-    public Builder addTarget(TransitiveInfoCollection target,
-        Function<TransitiveInfoCollection, Runfiles> mapping) {
-      return addTargetIncludingFileTargets(target, mapping);
+    @CanIgnoreReturnValue
+    public Builder addTarget(
+        TransitiveInfoCollection target,
+        Function<TransitiveInfoCollection, Runfiles> mapping,
+        boolean alwaysIncludeFilesToBuildInData) {
+      return addTargetIncludingFileTargets(target, mapping, alwaysIncludeFilesToBuildInData);
     }
 
     @CanIgnoreReturnValue
@@ -975,8 +982,10 @@ private Builder addTargetExceptFileTargets(
       return this;
     }
 
-    private Builder addTargetIncludingFileTargets(TransitiveInfoCollection target,
-        Function<TransitiveInfoCollection, Runfiles> mapping) {
+    private Builder addTargetIncludingFileTargets(
+        TransitiveInfoCollection target,
+        Function<TransitiveInfoCollection, Runfiles> mapping,
+        boolean alwaysIncludeFilesToBuildInData) {
       if (target.getProvider(RunfilesProvider.class) == null
           && mapping == RunfilesProvider.DATA_RUNFILES) {
         // RuleConfiguredTarget implements RunfilesProvider, so this will only be called on
@@ -988,6 +997,17 @@ private Builder addTargetIncludingFileTargets(TransitiveInfoCollection target,
         return this;
       }
 
+      if (alwaysIncludeFilesToBuildInData && mapping == RunfilesProvider.DATA_RUNFILES) {
+        // Ensure that `DefaultInfo.files` of Starlark rules is merged in so that native rules
+        // interoperate well with idiomatic Starlark rules..
+        // https://bazel.build/extending/rules#runfiles_features_to_avoid
+        // Internal tests fail if the order of filesToBuild is preserved.
+        addTransitiveArtifacts(
+            NestedSetBuilder.<Artifact>stableOrder()
+                .addTransitive(target.getProvider(FileProvider.class).getFilesToBuild())
+                .build());
+      }
+
       return addTargetExceptFileTargets(target, mapping);
     }
 

src/main/java/com/google/devtools/build/lib/analysis/config/BuildConfigurationValue.java

@@ -636,6 +636,13 @@ public boolean legacyExternalRunfiles() {
     return options.legacyExternalRunfiles;
   }
 
+  /**
+   * Returns true if Runfiles should merge in FilesToBuild from deps when collecting data runfiles.
+   */
+  public boolean alwaysIncludeFilesToBuildInData() {
+    return options.alwaysIncludeFilesToBuildInData;
+  }
+
   /**
    * Returns user-specified test environment variables and their values, as set by the --test_env
    * options.

src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java

@@ -433,6 +433,18 @@ public ExecConfigurationDistinguisherSchemeConverter() {
               + ".runfiles/wsname/external/repo (in addition to .runfiles/repo).")
   public boolean legacyExternalRunfiles;
 
+  @Option(
+      name = "incompatible_always_include_files_in_data",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.OUTPUT_SELECTION,
+      effectTags = {OptionEffectTag.AFFECTS_OUTPUTS},
+      metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+      help =
+          "If true, native rules add <code>DefaultInfo.files</code> of data dependencies to "
+              + "their runfiles, which matches the recommended behavior for Starlark rules ("
+              + "https://bazel.build/extending/rules#runfiles_features_to_avoid).")
+  public boolean alwaysIncludeFilesToBuildInData;
+
   @Option(
       name = "check_fileset_dependencies_recursively",
       defaultValue = "true",
@@ -930,6 +942,7 @@ public FragmentOptions getHost() {
     host.legacyExternalRunfiles = legacyExternalRunfiles;
     host.remotableSourceManifestActions = remotableSourceManifestActions;
     host.skipRunfilesManifests = skipRunfilesManifests;
+    host.alwaysIncludeFilesToBuildInData = alwaysIncludeFilesToBuildInData;
 
     // === Filesets ===
     host.strictFilesetOutput = strictFilesetOutput;

src/main/java/com/google/devtools/build/lib/rules/android/AndroidInstrumentationTestBase.java

@@ -89,7 +89,10 @@ public ConfiguredTarget create(RuleContext ruleContext)
             .addArtifact(testExecutable)
             .addArtifact(getInstrumentationApk(ruleContext))
             .addArtifact(getTargetApk(ruleContext))
-            .addTargets(runfilesDeps, RunfilesProvider.DEFAULT_RUNFILES)
+            .addTargets(
+                runfilesDeps,
+                RunfilesProvider.DEFAULT_RUNFILES,
+                ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData())
             .addTransitiveArtifacts(AndroidCommon.getSupportApks(ruleContext))
             .addTransitiveArtifacts(getAdb(ruleContext).getFilesToRun())
             .merge(getAapt(ruleContext).getRunfilesSupport())

src/main/java/com/google/devtools/build/lib/rules/android/AndroidLocalTestBase.java

@@ -469,7 +469,10 @@ private Runfiles collectDefaultRunfiles(
     // runtime jars always in naive link order, incompatible with compile order runfiles.
     builder.addArtifacts(getRuntimeJarsForTargets(getAndCheckTestSupport(ruleContext)).toList());
 
-    builder.addTargets(depsForRunfiles, RunfilesProvider.DEFAULT_RUNFILES);
+    builder.addTargets(
+        depsForRunfiles,
+        RunfilesProvider.DEFAULT_RUNFILES,
+        ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData());
 
     // We assume that the runtime jars will not have conflicting artifacts
     // with the same root relative path

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

@@ -739,7 +739,10 @@ private void collectDefaultRunfiles(
         builder.addSymlinks(runfiles.getSymlinks());
         builder.addRootSymlinks(runfiles.getRootSymlinks());
       } else {
-        builder.addTarget(defaultLauncher, RunfilesProvider.DEFAULT_RUNFILES);
+        builder.addTarget(
+            defaultLauncher,
+            RunfilesProvider.DEFAULT_RUNFILES,
+            ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData());
       }
     }
 
@@ -748,7 +751,10 @@ private void collectDefaultRunfiles(
 
     List<? extends TransitiveInfoCollection> runtimeDeps =
         ruleContext.getPrerequisites("runtime_deps");
-    builder.addTargets(runtimeDeps, RunfilesProvider.DEFAULT_RUNFILES);
+    builder.addTargets(
+        runtimeDeps,
+        RunfilesProvider.DEFAULT_RUNFILES,
+        ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData());
 
     builder.addTransitiveArtifactsWrappedInStableOrder(common.getRuntimeClasspath());
 

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

@@ -886,11 +886,17 @@ public static Runfiles getRunfiles(
       depsForRunfiles.addAll(ruleContext.getPrerequisites("exports"));
     }
 
-    runfilesBuilder.addTargets(depsForRunfiles, RunfilesProvider.DEFAULT_RUNFILES);
+    runfilesBuilder.addTargets(
+        depsForRunfiles,
+        RunfilesProvider.DEFAULT_RUNFILES,
+        ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData());
 
     TransitiveInfoCollection launcher = JavaHelper.launcherForTarget(semantics, ruleContext);
     if (launcher != null) {
-      runfilesBuilder.addTarget(launcher, RunfilesProvider.DATA_RUNFILES);
+      runfilesBuilder.addTarget(
+          launcher,
+          RunfilesProvider.DATA_RUNFILES,
+          ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData());
     }
 
     semantics.addRunfilesForLibrary(ruleContext, runfilesBuilder);

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

@@ -140,7 +140,10 @@ public ConfiguredTarget create(RuleContext ruleContext)
                     ruleContext.getConfiguration().legacyExternalRunfiles())
                 // add the jars to the runfiles
                 .addArtifacts(javaArtifacts.getRuntimeJars())
-                .addTargets(targets, RunfilesProvider.DEFAULT_RUNFILES)
+                .addTargets(
+                    targets,
+                    RunfilesProvider.DEFAULT_RUNFILES,
+                    ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData())
                 .addRunfiles(ruleContext, RunfilesProvider.DEFAULT_RUNFILES)
                 .build();
 

src/main/java/com/google/devtools/build/lib/rules/test/TestSuite.java

@@ -79,10 +79,15 @@ public ConfiguredTarget create(RuleContext ruleContext)
       directTestsAndSuitesBuilder.add(dep);
     }
 
-    Runfiles runfiles = new Runfiles.Builder(
-        ruleContext.getWorkspaceName(), ruleContext.getConfiguration().legacyExternalRunfiles())
-        .addTargets(directTestsAndSuitesBuilder, RunfilesProvider.DATA_RUNFILES)
-        .build();
+    Runfiles runfiles =
+        new Runfiles.Builder(
+                ruleContext.getWorkspaceName(),
+                ruleContext.getConfiguration().legacyExternalRunfiles())
+            .addTargets(
+                directTestsAndSuitesBuilder,
+                RunfilesProvider.DATA_RUNFILES,
+                ruleContext.getConfiguration().alwaysIncludeFilesToBuildInData())
+            .build();
 
     return new RuleConfiguredTargetBuilder(ruleContext)
         .add(RunfilesProvider.class,

src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java

@@ -713,6 +713,134 @@ public void testDefaultInfoWithRunfilesConstructor() throws Exception {
     assertThat(getConfiguredTarget("//src:r_tools")).isNotNull();
   }
 
+  @Test
+  public void testDefaultInfoFilesAddedToCcBinaryTargetRunfiles() throws Exception {
+    scratch.file(
+        "test/starlark/extension.bzl",
+        "def custom_rule_impl(ctx):",
+        "  out = ctx.actions.declare_file(ctx.attr.name + '.out')",
+        "  ctx.actions.write(out, 'foobar')",
+        "  return [DefaultInfo(files = depset([out]))]",
+        "",
+        "custom_rule = rule(implementation = custom_rule_impl)");
+
+    scratch.file(
+        "test/starlark/BUILD",
+        "load('//test/starlark:extension.bzl', 'custom_rule')",
+        "",
+        "custom_rule(name = 'cr')",
+        "cc_binary(name = 'binary', data = [':cr'])");
+
+    useConfiguration("--incompatible_always_include_files_in_data");
+    ConfiguredTarget target = getConfiguredTarget("//test/starlark:binary");
+
+    assertThat(target.getLabel().toString()).isEqualTo("//test/starlark:binary");
+    assertThat(
+            ActionsTestUtil.baseArtifactNames(
+                target.getProvider(RunfilesProvider.class).getDefaultRunfiles().getAllArtifacts()))
+        .contains("cr.out");
+    assertThat(
+            ActionsTestUtil.baseArtifactNames(
+                target.getProvider(RunfilesProvider.class).getDataRunfiles().getAllArtifacts()))
+        .contains("cr.out");
+  }
+
+  @Test
+  public void testDefaultInfoFilesAddedToJavaBinaryTargetRunfiles() throws Exception {
+    scratch.file(
+        "test/starlark/extension.bzl",
+        "def custom_rule_impl(ctx):",
+        "  out = ctx.actions.declare_file(ctx.attr.name + '.out')",
+        "  ctx.actions.write(out, 'foobar')",
+        "  return [DefaultInfo(files = depset([out]))]",
+        "",
+        "custom_rule = rule(implementation = custom_rule_impl)");
+
+    scratch.file(
+        "test/starlark/BUILD",
+        "load('//test/starlark:extension.bzl', 'custom_rule')",
+        "",
+        "custom_rule(name = 'cr')",
+        "java_binary(name = 'binary', data = [':cr'], srcs = ['Foo.java'], main_class = 'Foo')");
+
+    useConfiguration("--incompatible_always_include_files_in_data");
+    ConfiguredTarget target = getConfiguredTarget("//test/starlark:binary");
+
+    assertThat(target.getLabel().toString()).isEqualTo("//test/starlark:binary");
+    assertThat(
+            ActionsTestUtil.baseArtifactNames(
+                target.getProvider(RunfilesProvider.class).getDefaultRunfiles().getAllArtifacts()))
+        .contains("cr.out");
+    assertThat(
+            ActionsTestUtil.baseArtifactNames(
+                target.getProvider(RunfilesProvider.class).getDataRunfiles().getAllArtifacts()))
+        .contains("cr.out");
+  }
+
+  @Test
+  public void testDefaultInfoFilesAddedToPyBinaryTargetRunfiles() throws Exception {
+    scratch.file(
+        "test/starlark/extension.bzl",
+        "def custom_rule_impl(ctx):",
+        "  out = ctx.actions.declare_file(ctx.attr.name + '.out')",
+        "  ctx.actions.write(out, 'foobar')",
+        "  return [DefaultInfo(files = depset([out]))]",
+        "",
+        "custom_rule = rule(implementation = custom_rule_impl)");
+
+    scratch.file(
+        "test/starlark/BUILD",
+        "load('//test/starlark:extension.bzl', 'custom_rule')",
+        "",
+        "custom_rule(name = 'cr')",
+        "py_binary(name = 'binary', data = [':cr'], srcs = ['binary.py'])");
+
+    useConfiguration("--incompatible_always_include_files_in_data");
+    ConfiguredTarget target = getConfiguredTarget("//test/starlark:binary");
+
+    assertThat(target.getLabel().toString()).isEqualTo("//test/starlark:binary");
+    assertThat(
+            ActionsTestUtil.baseArtifactNames(
+                target.getProvider(RunfilesProvider.class).getDefaultRunfiles().getAllArtifacts()))
+        .contains("cr.out");
+    assertThat(
+            ActionsTestUtil.baseArtifactNames(
+                target.getProvider(RunfilesProvider.class).getDataRunfiles().getAllArtifacts()))
+        .contains("cr.out");
+  }
+
+  @Test
+  public void testDefaultInfoFilesAddedToShBinaryTargetRunfiles() throws Exception {
+    scratch.file(
+        "test/starlark/extension.bzl",
+        "def custom_rule_impl(ctx):",
+        "  out = ctx.actions.declare_file(ctx.attr.name + '.out')",
+        "  ctx.actions.write(out, 'foobar')",
+        "  return [DefaultInfo(files = depset([out]))]",
+        "",
+        "custom_rule = rule(implementation = custom_rule_impl)");
+
+    scratch.file(
+        "test/starlark/BUILD",
+        "load('//test/starlark:extension.bzl', 'custom_rule')",
+        "",
+        "custom_rule(name = 'cr')",
+        "sh_binary(name = 'binary', data = [':cr'], srcs = ['script.sh'])");
+
+    useConfiguration("--incompatible_always_include_files_in_data");
+    ConfiguredTarget target = getConfiguredTarget("//test/starlark:binary");
+
+    assertThat(target.getLabel().toString()).isEqualTo("//test/starlark:binary");
+    assertThat(
+            ActionsTestUtil.baseArtifactNames(
+                target.getProvider(RunfilesProvider.class).getDefaultRunfiles().getAllArtifacts()))
+        .contains("cr.out");
+    assertThat(
+            ActionsTestUtil.baseArtifactNames(
+                target.getProvider(RunfilesProvider.class).getDataRunfiles().getAllArtifacts()))
+        .contains("cr.out");
+  }
+
   @Test
   public void testInstrumentedFilesProviderWithCodeCoverageDisabled() throws Exception {
     setBuildLanguageOptions("--incompatible_disallow_struct_provider_syntax=false");