Allow passing list into user_compile_flags and user_link_flags

Depset has advantage that it doesn't grow quadratically in memory, but a
disadvantage, that it removes duplicates. Which is especially bad when
user_compile_flags contains [ "-isystem", "a", "-isystem", "b" ] as it will
result in [ "-isystem", "a", "b" ] passed to the compiler, and the compiler
won't be happy.

RELNOTES: None.
PiperOrigin-RevId: 212223261

src/main/java/com/google/devtools/build/lib/rules/cpp/CcModule.java

@@ -208,7 +208,7 @@ public CcToolchainVariables getCompileBuildVariables(
         /* dwoFile= */ null,
         /* ltoIndexingFile= */ null,
         /* includes= */ ImmutableList.of(),
-        asStringNestedSet(userCompileFlags),
+        userFlagsToIterable(ccToolchainProvider.getCppConfiguration(), userCompileFlags),
         /* cppModuleMap= */ null,
         usePic,
         /* fakeOutputFile= */ null,
@@ -254,7 +254,7 @@ public CcToolchainVariables getLinkBuildVariables(
         featureConfiguration,
         useTestOnlyFlags,
         /* isLtoIndexing= */ false,
-        asStringNestedSet(userLinkFlags),
+        userFlagsToIterable(ccToolchainProvider.getCppConfiguration(), userLinkFlags),
         /* interfaceLibraryBuilder= */ null,
         /* interfaceLibraryOutput= */ null,
         /* ltoOutputRootPrefix= */ null,
@@ -285,7 +285,7 @@ protected static <T> T convertFromNoneable(Object obj, @Nullable T defaultValue)
     return (T) obj;
   }
 
-  /** Converts an object that can be the either SkylarkNestedSet or None into NestedSet. */
+  /** Converts an object that can be ether SkylarkNestedSet or None into NestedSet. */
   protected NestedSet<String> asStringNestedSet(Object o) {
     SkylarkNestedSet skylarkNestedSet = convertFromNoneable(o, /* defaultValue= */ null);
     if (skylarkNestedSet != null) {
@@ -295,6 +295,39 @@ protected NestedSet<String> asStringNestedSet(Object o) {
     }
   }
 
+  /** Converts an object that can be either SkylarkList, or None into ImmutableList. */
+  protected ImmutableList<String> asStringImmutableList(Object o) {
+    SkylarkList skylarkList = convertFromNoneable(o, /* defaultValue= */ null);
+    if (skylarkList != null) {
+      return skylarkList.getImmutableList();
+    } else {
+      return ImmutableList.of();
+    }
+  }
+
+  /**
+   * Converts an object that represents user flags and can be either SkylarkNestedSet , SkylarkList,
+   * or None into Iterable.
+   */
+  protected Iterable<String> userFlagsToIterable(CppConfiguration cppConfiguration, Object o)
+      throws EvalException {
+    if (o instanceof SkylarkNestedSet) {
+      if (cppConfiguration.disableDepsetInUserFlags()) {
+        throw new EvalException(
+            Location.BUILTIN,
+            "Passing depset into user flags is deprecated (see "
+                + "--incompatible_disable_depset_in_cc_user_flags), use list instead.");
+      }
+      return asStringNestedSet(o);
+    } else if (o instanceof SkylarkList) {
+      return asStringImmutableList(o);
+    } else if (o instanceof NoneType) {
+      return ImmutableList.of();
+    } else {
+      throw new EvalException(Location.BUILTIN, "Only depset and list is allowed.");
+    }
+  }
+
   @Override
   public LibraryToLink createLibraryLinkerInput(
       SkylarkRuleContext skylarkRuleContext, Artifact library, String skylarkArtifactCategory)

src/main/java/com/google/devtools/build/lib/rules/cpp/CppConfiguration.java

@@ -1175,6 +1175,10 @@ public boolean disableEmittingStaticLibgcc() {
     return cppOptions.disableEmittingStaticLibgcc;
   }
 
+  public boolean disableDepsetInUserFlags() {
+    return cppOptions.disableDepsetInUserFlags;
+  }
+
   private void checkForToolchainSkylarkApiAvailability() throws EvalException {
     if (cppOptions.disableLegacyToolchainSkylarkApi
         || !cppOptions.enableLegacyToolchainSkylarkApi) {

src/main/java/com/google/devtools/build/lib/rules/cpp/CppOptions.java

@@ -892,6 +892,21 @@ public Label getFdoPrefetchHintsLabel() {
               + "across subpackage boundaries.")
   public boolean experimentalIncludesAttributeSubpackageTraversal;
 
+  @Option(
+      name = "incompatible_disable_depset_in_cc_user_flags",
+      defaultValue = "false",
+      documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
+      effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS},
+      metadataTags = {
+        OptionMetadataTag.INCOMPATIBLE_CHANGE,
+        OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
+      },
+      help =
+          "If true, C++ toolchain Skylark API will not accept depset in `user_compile_flags` "
+              + "param of `create_compile_variables`, and in `user_link_flags` of "
+              + "`create_link_variables`. Use list instead.")
+  public boolean disableDepsetInUserFlags;
+
   @Override
   public FragmentOptions getHost() {
     CppOptions host = (CppOptions) getDefault();

src/main/java/com/google/devtools/build/lib/skylarkbuildapi/cpp/CcModuleApi.java

@@ -229,13 +229,16 @@ SkylarkDict<String, String> getEnvironmentVariable(
             noneable = true),
         @Param(
             name = "user_compile_flags",
-            doc = "Depset of additional compilation flags (copts).",
+            doc =
+                "List of additional compilation flags (copts). Passing depset is deprecated and "
+                    + "will be removed by --incompatible_disable_depset_in_cc_user_flags flag.",
             positional = false,
             named = true,
             defaultValue = "None",
             noneable = true,
             allowedTypes = {
               @ParamType(type = NoneType.class),
+              @ParamType(type = SkylarkList.class),
               @ParamType(type = SkylarkNestedSet.class)
             }),
         @Param(
@@ -352,13 +355,16 @@ CcToolchainVariablesT getCompileBuildVariables(
             }),
         @Param(
             name = "user_link_flags",
-            doc = "Depset of additional link flags (linkopts).",
+            doc =
+                "List of additional link flags (linkopts). Passing depset is deprecated and "
+                    + "will be removed by --incompatible_disable_depset_in_cc_user_flags flag.",
             positional = false,
             named = true,
             defaultValue = "None",
             noneable = true,
             allowedTypes = {
               @ParamType(type = NoneType.class),
+              @ParamType(type = SkylarkList.class),
               @ParamType(type = SkylarkNestedSet.class)
             }),
         @Param(

src/test/java/com/google/devtools/build/lib/rules/cpp/SkylarkCcCommonTest.java

@@ -505,6 +505,35 @@ public void testUserCompileFlags() throws Exception {
         .contains("-foo");
   }
 
+  @Test
+  public void testUserCompileFlagsAsList() throws Exception {
+    assertThat(
+            commandLineForVariables(
+                CppActionNames.CPP_COMPILE,
+                "cc_common.create_compile_variables(",
+                "feature_configuration = feature_configuration,",
+                "cc_toolchain = toolchain,",
+                "user_compile_flags = ['-foo']",
+                ")"))
+        .contains("-foo");
+  }
+
+  @Test
+  public void testUserCompileFlagsAsDepsetWhenDisabled() throws Exception {
+    useConfiguration("--incompatible_disable_depset_in_cc_user_flags");
+    reporter.removeHandler(failFastHandler);
+    assertThat(
+            commandLineForVariables(
+                CppActionNames.CPP_COMPILE,
+                "cc_common.create_compile_variables(",
+                "feature_configuration = feature_configuration,",
+                "cc_toolchain = toolchain,",
+                "user_compile_flags = depset(['-foo'])",
+                ")"))
+        .isNull();
+    assertContainsEvent("Passing depset into user flags is deprecated");
+  }
+
   @Test
   public void testEmptyLinkVariables() throws Exception {
     useConfiguration("--linkopt=-foo");
@@ -622,6 +651,35 @@ public void testUserLinkFlagsLinkVariables() throws Exception {
         .contains("-avocado");
   }
 
+  @Test
+  public void testUserLinkFlagsLinkVariablesAsList() throws Exception {
+    assertThat(
+            commandLineForVariables(
+                CppActionNames.CPP_LINK_EXECUTABLE,
+                "cc_common.create_link_variables(",
+                "feature_configuration = feature_configuration,",
+                "cc_toolchain = toolchain,",
+                "user_link_flags = [ '-avocado' ],",
+                ")"))
+        .contains("-avocado");
+  }
+
+  @Test
+  public void testUserLinkFlagsLinkVariablesAsDepsetWhenDisabled() throws Exception {
+    useConfiguration("--incompatible_disable_depset_in_cc_user_flags");
+    reporter.removeHandler(failFastHandler);
+    assertThat(
+            commandLineForVariables(
+                CppActionNames.CPP_LINK_EXECUTABLE,
+                "cc_common.create_link_variables(",
+                "feature_configuration = feature_configuration,",
+                "cc_toolchain = toolchain,",
+                "user_link_flags = depset([ '-avocado' ]),",
+                ")"))
+        .isNull();
+    assertContainsEvent("Passing depset into user flags is deprecated");
+  }
+
   @Test
   public void testIfsoRelatedVariablesAreNotExposed() throws Exception {
     AnalysisMock.get()
@@ -901,6 +959,9 @@ private SkylarkList<String> commandLineForVariables(
     /** Calling {@link #getTarget} to get loading errors */
     getTarget("//a" + pkgSuffix + ":r");
     ConfiguredTarget r = getConfiguredTarget("//a" + pkgSuffix + ":r");
+    if (r == null) {
+      return null;
+    }
     @SuppressWarnings("unchecked")
     SkylarkList<String> result = (SkylarkList<String>) r.get("command_line");
     return result;