Fix javacopts semantics broken by storing them as depsets

The final javacopts passed to `JavaBuilder`/`javac` is computed (in `java_common.compile`) by concatenating the following sets of options (in order):

1. The `javacopts` attribute on `java_toolchain`
2. The options provided with `--javacopt` on the commandline
3. The matching `compatible_javacopts` attribute on `java_toolchain`
4. The matching options from `java_toolchain.package_config`
5. The `javacopts` attribute on the `java_*` rule

We already de-tokenize each of these sets into a single space-separated string before adding them to a `depset` (not doing so would mean `['-source', '8', '-target', '8', '-Xmx1G']` would result in getting back `['-source', '8', '-target', '-Xmx1G']` from the depset).

In most cases, this de-tokenization is enough to ensure we get back the same set of options as we would if we stored them as a list. However, in the rare case that any of the above sets is *exactly* identical to another, it gets de-duped upon flattening.

Since the right-most option should win, this change makes it so, that after de-duping, the right-most occurrence is preserved. A depset always performs left-to-right traversal, so all we need to do is add the above 5 sets of options in reverse order, and then reverse once more after flattening.

PiperOrigin-RevId: 573895869
Change-Id: Id4758597e6d93fe5c71856a742915cafc1287a68

Author: hvadehra

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

@@ -93,13 +93,24 @@ private static String filterLauncherForTarget(RuleContext ruleContext) {
   }
 
   /**
-   * Javac options require special processing - People use them and expect the options to be
-   * tokenized.
+   * Flattens a set of javacopts and tokenizes the contents.
+   *
+   * <p>Since javac allows passing the same option multiple times, and the right-most one wins,
+   * multiple instances of the same option+value get de-duped by the NestedSet. So when combining
+   * multiple NestedSets, we store them in reverse order, and reverse again after flattening. This
+   * preserves the right-most occurrence in its correct position, thus achieving the correct
+   * semantics.
+   *
+   * @param inOpts the set of opts to tokenize
    */
   public static ImmutableList<String> tokenizeJavaOptions(NestedSet<String> inOpts) {
-    return tokenizeJavaOptions(inOpts.toList());
+    return tokenizeJavaOptions(inOpts.toList().reverse());
   }
 
+  /**
+   * Javac options require special processing - People use them and expect the options to be
+   * tokenized.
+   */
   public static ImmutableList<String> tokenizeJavaOptions(Iterable<String> inOpts) {
     // Ideally, this would be in the options parser. Unfortunately,
     // the options parser can't handle a converter that expands

src/main/starlark/builtins_bzl/common/java/java_common_internal_for_builtins.bzl

@@ -150,7 +150,11 @@ def compile(
     # detokenize target's javacopts, it will be tokenized before compilation
     all_javac_opts.append(helper.detokenize_javacopts(helper.tokenize_javacopts(ctx, javac_opts)))
 
-    all_javac_opts = depset(order = "preorder", transitive = all_javac_opts)
+    # we reverse the list of javacopts depsets, so that we keep the right-most set
+    # in case it's deduped. When this depset is flattened, we will reverse again,
+    # and then tokenize before passing to javac. This way, right-most javacopts will
+    # be retained and "win out".
+    all_javac_opts = depset(order = "preorder", transitive = reversed(all_javac_opts))
 
     # Optimization: skip this if there are no annotation processors, to avoid unnecessarily
     # disabling the direct classpath optimization if `enable_annotation_processor = False`

src/main/starlark/builtins_bzl/common/java/java_helper.bzl

@@ -378,15 +378,17 @@ def _shell_escape(s):
 def _tokenize_javacopts(ctx, opts):
     """Tokenizes a list or depset of options to a list.
 
+    Iff opts is a depset, we reverse the flattened list to ensure right-most
+    duplicates are preserved in their correct position.
+
     Args:
         ctx: (RuleContext) the rule context
         opts: (depset[str]|[str]) the javac options to tokenize
-
     Returns:
         [str] list of tokenized options
     """
     if hasattr(opts, "to_list"):
-        opts = opts.to_list()
+        opts = reversed(opts.to_list())
     return [
         token
         for opt in opts
@@ -397,7 +399,7 @@ def _detokenize_javacopts(opts):
     """Detokenizes a list of options to a depset.
 
     Args:
-        opts: (depset[str]) the javac options to tokenize
+        opts: ([str]) the javac options to detokenize
 
     Returns:
         (depset[str]) depset of detokenized options