Add --incompatible_strict_objc_module_maps.

This flag changes the behavior of objc_library module map propagation so that module maps are only propagated to direct dependents, not transitive dependents. swift_library targets that import Objective-C code must then list those dependencies directly in its deps instead of depending on them being transitively present.

PiperOrigin-RevId: 187184692

7 files changed, 98 insertions, 27 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommandLineOptions.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommandLineOptions.java
index 64637c39d8..c836e345fe 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommandLineOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommandLineOptions.java
@@ -304,13 +304,26 @@ public class ObjcCommandLineOptions extends FragmentOptions {
     defaultValue = "null",
     converter = LabelConverter.class,
     documentationCategory = OptionDocumentationCategory.TOOLCHAIN,
-    effectTags = {OptionEffectTag.CHANGES_INPUTS, OptionEffectTag.LOADING_AND_ANALYSIS},
+    effectTags = {OptionEffectTag.AFFECTS_OUTPUTS, OptionEffectTag.LOADING_AND_ANALYSIS},
     help =
         "Location of target that will provide the appropriate Apple SDK for the current build "
             + "configuration."
   )
   public Label appleSdk;
 
+  @Option(
+    name = "incompatible_strict_objc_module_maps",
+    category = "incompatible changes",
+    defaultValue = "false",
+    documentationCategory = OptionDocumentationCategory.TOOLCHAIN,
+    metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
+    effectTags = {OptionEffectTag.CHANGES_INPUTS, OptionEffectTag.LOADING_AND_ANALYSIS},
+    help =
+        "Propagates Objective-C module maps only to direct dependencies in the 'objc' provider, "
+            + "not to all transitive dependencies."
+  )
+  public boolean strictObjcModuleMaps;
+
   @Override
   public FragmentOptions getHost() {
     ObjcCommandLineOptions host = (ObjcCommandLineOptions) super.getHost();
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommon.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommon.java
index 3019d7a1a3..fbfa23e121 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommon.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcCommon.java
@@ -545,8 +545,13 @@ public final class ObjcCommon {
         if (umbrellaHeader.isPresent()) {
           objcProvider.add(UMBRELLA_HEADER, umbrellaHeader.get());
         }
-        objcProvider.add(MODULE_MAP, moduleMap.getArtifact());
-        objcProvider.add(TOP_LEVEL_MODULE_MAP, moduleMap);
+        if (context.getFragment(ObjcConfiguration.class).useStrictObjcModuleMaps()) {
+          objcProvider.addForDirectDependents(MODULE_MAP, moduleMap.getArtifact());
+          objcProvider.addForDirectDependents(TOP_LEVEL_MODULE_MAP, moduleMap);
+        } else {
+          objcProvider.add(MODULE_MAP, moduleMap.getArtifact());
+          objcProvider.add(TOP_LEVEL_MODULE_MAP, moduleMap);
+        }
       }
 
       objcProvider
diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcConfiguration.java b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcConfiguration.java
index 6139c2a541..d95e40ca7c 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcConfiguration.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/objc/ObjcConfiguration.java
@@ -80,6 +80,7 @@ public class ObjcConfiguration extends BuildConfiguration.Fragment {
   private final int objcHeaderThinningPartitionSize;
   private final Label objcHeaderScannerTool;
   private final Label appleSdk;
+  private final boolean strictObjcModuleMaps;
 
   ObjcConfiguration(ObjcCommandLineOptions objcOptions, BuildConfiguration.Options options) {
     this.iosSimulatorDevice =
@@ -115,6 +116,7 @@ public class ObjcConfiguration extends BuildConfiguration.Fragment {
     this.objcHeaderThinningPartitionSize = objcOptions.objcHeaderThinningPartitionSize;
     this.objcHeaderScannerTool = objcOptions.objcHeaderScannerTool;
     this.appleSdk = objcOptions.appleSdk;
+    this.strictObjcModuleMaps = objcOptions.strictObjcModuleMaps;
   }
 
   @AutoCodec.Instantiator
@@ -142,7 +144,8 @@ public class ObjcConfiguration extends BuildConfiguration.Fragment {
       boolean experimentalHeaderThinning,
       int objcHeaderThinningPartitionSize,
       Label objcHeaderScannerTool,
-      Label appleSdk) {
+      Label appleSdk,
+      boolean strictObjcModuleMaps) {
     this.iosSimulatorVersion = iosSimulatorVersion;
     this.iosSimulatorDevice = iosSimulatorDevice;
     this.watchosSimulatorVersion = watchosSimulatorVersion;
@@ -167,6 +170,7 @@ public class ObjcConfiguration extends BuildConfiguration.Fragment {
     this.objcHeaderThinningPartitionSize = objcHeaderThinningPartitionSize;
     this.objcHeaderScannerTool = objcHeaderScannerTool;
     this.appleSdk = appleSdk;
+    this.strictObjcModuleMaps = strictObjcModuleMaps;
   }
 
   /**
@@ -375,4 +379,9 @@ public class ObjcConfiguration extends BuildConfiguration.Fragment {
   public Label getAppleSdk() {
     return appleSdk;
   }
+
+  /** Returns true if Objective-C module maps should only be propagated to direct dependencies. */
+  public boolean useStrictObjcModuleMaps() {
+    return strictObjcModuleMaps;
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/AppleBinaryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/AppleBinaryTest.java
index b940580870..cc23c7cf83 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/objc/AppleBinaryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/objc/AppleBinaryTest.java
@@ -1460,7 +1460,7 @@ public class AppleBinaryTest extends ObjcRuleTestCase {
 
   @Test
   public void testCustomModuleMap() throws Exception {
-    checkCustomModuleMap(getRuleType());
+    checkCustomModuleMapNotPropagatedByTargetUnderTest(getRuleType());
   }
 
   @Test
diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java
index ec3dea8d41..0a83e02975 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcLibraryTest.java
@@ -1928,7 +1928,7 @@ public class ObjcLibraryTest extends ObjcRuleTestCase {
 
   @Test
   public void testCustomModuleMap() throws Exception {
-    checkCustomModuleMap(RULE_TYPE);
+    checkCustomModuleMapPropagatedByTargetUnderTest(RULE_TYPE);
   }
 
   private boolean containsObjcFeature(String srcName) throws Exception {
diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcProtoLibraryTest.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcProtoLibraryTest.java
index 955ce658a7..2dbd7db711 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcProtoLibraryTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcProtoLibraryTest.java
@@ -399,27 +399,32 @@ public class ObjcProtoLibraryTest extends ObjcRuleTestCase {
 
   @Test
   public void testModulemapCreatedForNonLinkingTargets() throws Exception {
-    checkOnlyLibModuleMapsArePresentForTarget("//package:opl_protobuf");
+    // TODO(b/73943026): Remove this flag once everyone has migrated to the new strict behavior and
+    // it is made the default.
+    useConfiguration("--incompatible_strict_objc_module_maps");
+
+    // The library target should propagate its module map.
+    ObjcProvider provider = providerForTarget("//package:opl_protobuf");
+    assertThat(Artifact.toRootRelativePaths(provider.get(ObjcProvider.MODULE_MAP).toSet()))
+        .containsExactly("package/opl_protobuf.modulemaps/module.modulemap");
   }
 
   @Test
   public void testModulemapNotCreatedForLinkingTargets() throws Exception {
-    checkOnlyLibModuleMapsArePresentForTarget("//package:opl_binary");
+    // TODO(b/73943026): Remove this flag once everyone has migrated to the new strict behavior and
+    // it is made the default.
+    useConfiguration("--incompatible_strict_objc_module_maps");
+
+    // The binary target should not propagate the module map from the library it depends on.
+    ObjcProvider provider = providerForTarget("//package:opl_binary");
+    assertThat(Artifact.toRootRelativePaths(provider.get(ObjcProvider.MODULE_MAP).toSet()))
+        .isEmpty();
   }
 
   private static String sortedJoin(Iterable<String> elements) {
     return Joiner.on('\n').join(Ordering.natural().immutableSortedCopy(elements));
   }
 
-  private void checkOnlyLibModuleMapsArePresentForTarget(String target) throws Exception {
-    ObjcProvider provider = providerForTarget(target);
-    assertThat(Artifact.toRootRelativePaths(provider.get(ObjcProvider.MODULE_MAP).toSet()))
-        .containsExactly(
-            "package/opl_protobuf.modulemaps/module.modulemap",
-            TestConstants.TOOLS_REPOSITORY_PATH_PREFIX
-                + "objcproto/protobuf_lib.modulemaps/module.modulemap");
-  }
-
   @Test
   public void testObjcProvider() throws Exception {
     ConfiguredTarget target = getConfiguredTarget("//package:opl_protobuf");
diff --git a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java
index 6da95762ab..9a8dfff2db 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/objc/ObjcRuleTestCase.java
@@ -3351,19 +3351,40 @@ public abstract class ObjcRuleTestCase extends BuildViewTestCase {
         .isEqualTo("a.o");
   }
 
-  protected void checkCustomModuleMap(RuleType ruleType) throws Exception {
-    useConfiguration("--experimental_objc_enable_module_maps");
-    ruleType.scratchTarget(scratch, "deps", "['//z:testModuleMap']");
+  protected void checkCustomModuleMapNotPropagatedByTargetUnderTest(
+      RuleType ruleType) throws Exception {
+    checkCustomModuleMap(ruleType, false);
+  }
+
+  protected void checkCustomModuleMapPropagatedByTargetUnderTest(
+      RuleType ruleType) throws Exception {
+    checkCustomModuleMap(ruleType, true);
+  }
+
+  private void checkCustomModuleMap(RuleType ruleType, boolean targetUnderTestShouldPropagate)
+      throws Exception {
+    useConfiguration(
+        "--experimental_objc_enable_module_maps", "--incompatible_strict_objc_module_maps");
+    ruleType.scratchTarget(scratch, "deps", "['//z:a']");
+    scratch.file("z/a.m");
+    scratch.file("z/a.h");
     scratch.file("z/b.m");
     scratch.file("z/b.h");
     scratch.file("y/module.modulemap", "module my_module_b { export *\n header b.h }");
-    scratch.file("z/BUILD",
+    scratch.file(
+        "z/BUILD",
+        "objc_library(",
+        "name = 'testModuleMap',",
+        "hdrs = ['b.h'],",
+        "srcs = ['b.m'],",
+        "module_map = '//y:mm'",
+        ")",
         "objc_library(",
-            "name = 'testModuleMap',",
-            "hdrs = ['b.h'],",
-            "srcs = ['b.m'],",
-            "module_map = '//y:mm'",
-         ")");
+        "name = 'a',",
+        "hdrs = ['a.h'],",
+        "srcs = ['a.m'],",
+        "deps = [':testModuleMap']",
+        ")");
     scratch.file("y/BUILD",
         "filegroup(",
             "name = 'mm',",
@@ -3374,12 +3395,30 @@ public abstract class ObjcRuleTestCase extends BuildViewTestCase {
     assertThat(compileActionA.getArguments()).doesNotContain("-fmodule-maps");
     assertThat(compileActionA.getArguments()).doesNotContain("-fmodule-name");
 
+    String x8664Genfiles =
+        configurationGenfiles("x86_64", ConfigurationDistinguisher.UNKNOWN, null);
+
+    // The target with the module map should propagate it to its direct dependers...
     ObjcProvider provider = providerForTarget("//z:testModuleMap");
     assertThat(Artifact.toExecPaths(provider.get(MODULE_MAP)))
         .containsExactly("y/module.modulemap");
 
+    // ...and the target depending on //z:testModuleMap will see it (as well as its own)...
+    provider = providerForTarget("//z:a");
+    assertThat(Artifact.toExecPaths(provider.get(MODULE_MAP)))
+        .containsExactly(x8664Genfiles + "/z/a.modulemaps/module.modulemap", "y/module.modulemap");
+
     provider = providerForTarget("//x:x");
-    assertThat(Artifact.toExecPaths(provider.get(MODULE_MAP))).contains("y/module.modulemap");
+    if (targetUnderTestShouldPropagate) {
+      // ...and //x:x should propagate //z:a but not //z:testModuleMap.
+      assertThat(Artifact.toExecPaths(provider.get(MODULE_MAP)))
+          .containsExactly(
+              x8664Genfiles + "/x/x.modulemaps/module.modulemap",
+              x8664Genfiles + "/z/a.modulemaps/module.modulemap");
+    } else {
+      // ...but //x:x should not see them.
+      assertThat(Artifact.toExecPaths(provider.get(MODULE_MAP))).isEmpty();
+    }
   }
 
   /**