Improve checksum computation for starlark options.

This is important now that there is such a thing as `config.string_set`.

* A `set` should result in a different checksum than a `list` because the difference is observable to starlark code. Uses `Starlark#type` in the checksum computation.
* `set([])` and `set([""])` should result in different checksums.

This fixes a nondeterminism bug whereby `BuildOptions` with the same checksum but different collection types can be incorrectly cached across invocations.

PiperOrigin-RevId: 863645964
Change-Id: I257f7a723ce33751141708417c888ef6c08752d0

Author: justinhorvitz

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

@@ -188,9 +188,9 @@ public String checksum() {
             for (FragmentOptions options : fragmentOptionsMap.values()) {
               fingerprint.addString(options.cacheKey());
             }
-            fingerprint.addString(OptionsBase.mapToCacheKey(starlarkOptionsMap));
+            fingerprint.addString(OptionsBase.starlarkMapToCacheKey(starlarkOptionsMap));
             fingerprint.addString(OptionsBase.mapToCacheKey(scopes));
-            fingerprint.addString(OptionsBase.mapToCacheKey(onLeaveScopeValuesMap));
+            fingerprint.addString(OptionsBase.starlarkMapToCacheKey(onLeaveScopeValuesMap));
             checksum = fingerprint.hexDigestAndReset();
           }
         }

src/main/java/com/google/devtools/common/options/OptionsBase.java

@@ -17,9 +17,11 @@
 import com.google.common.collect.Maps;
 import com.google.common.escape.CharEscaperBuilder;
 import com.google.common.escape.Escaper;
+import java.util.Collection;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import net.starlark.java.eval.Starlark;
 
 /**
  * Base class for all options classes. Extend this class, adding public instance fields annotated
@@ -84,32 +86,49 @@ public final String toString() {
   }
 
   /**
-   * Returns a string that uniquely identifies the options. This value is
-   * intended for analysis caching.
+   * Returns a string that uniquely identifies the options. This value is intended for analysis
+   * caching.
    */
   public final String cacheKey() {
     StringBuilder result = new StringBuilder(getClass().getName()).append("{");
     result.append(mapToCacheKey(asMap()));
     return result.append("}").toString();
   }
 
+  /**
+   * Like {@link #mapToCacheKey} but the returned key is sensitive to the {@link Starlark#type} of
+   * values in the map.
+   *
+   * <p>This is important because types are observable to starlark code. See b/478938163.
+   */
+  public static String starlarkMapToCacheKey(Map<?, ?> starlarkOptionsMap) {
+    return mapToCacheKey(starlarkOptionsMap, /* distinguishStarlarkTypes= */ true);
+  }
+
   public static String mapToCacheKey(Map<?, ?> optionsMap) {
+    return mapToCacheKey(optionsMap, /* distinguishStarlarkTypes= */ false);
+  }
+
+  private static String mapToCacheKey(Map<?, ?> optionsMap, boolean distinguishStarlarkTypes) {
     StringBuilder result = new StringBuilder();
     for (Map.Entry<?, ?> entry : optionsMap.entrySet()) {
       result.append(entry.getKey()).append("=");
 
       Object value = entry.getValue();
-      // This special case is needed because List.toString() prints the same
-      // ("[]") for an empty list and for a list with a single empty string.
-      if (value instanceof List<?> && ((List<?>) value).isEmpty()) {
-        result.append("EMPTY");
-      } else if (value == null) {
+
+      if (value == null) {
         result.append("NULL");
       } else {
-        result
-            .append('"')
-            .append(ESCAPER.escape(value.toString()))
-            .append('"');
+        if (distinguishStarlarkTypes) {
+          result.append(Starlark.type(value));
+        }
+        // This special case is needed because Collection.toString() prints the same ("[]") for an
+        // empty collection and for a collection with a single empty string.
+        if (value instanceof Collection<?> c && c.isEmpty()) {
+          result.append("EMPTY");
+        } else {
+          result.append('"').append(ESCAPER.escape(value.toString())).append('"');
+        }
       }
       result.append(", ");
     }

src/test/java/com/google/devtools/build/lib/analysis/config/BuildOptionsTest.java

@@ -19,6 +19,7 @@
 import com.google.common.collect.ImmutableClassToInstanceMap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Lists;
 import com.google.devtools.build.lib.analysis.config.BuildOptions.MapBackedChecksumCache;
 import com.google.devtools.build.lib.analysis.config.BuildOptions.OptionsChecksumCache;
 import com.google.devtools.build.lib.cmdline.Label;
@@ -34,6 +35,8 @@
 import com.google.protobuf.ByteString;
 import com.google.testing.junit.testparameterinjector.TestParameter;
 import com.google.testing.junit.testparameterinjector.TestParameterInjector;
+import java.util.ArrayList;
+import java.util.LinkedHashSet;
 import java.util.List;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -407,5 +410,50 @@ public void starlarkOptionsOrderedByLabel() {
     assertThat(backward.getStarlarkOptions())
         .containsExactly(label1, true, label2, false)
         .inOrder();
+    assertThat(backward).isEqualTo(forward);
+    assertThat(backward.checksum()).isEqualTo(forward.checksum());
+  }
+
+  @Test
+  public void listAndSetAreDifferent() {
+    Label label = Label.parseCanonicalUnchecked("//pkg:option");
+
+    BuildOptions optionsWithList =
+        BuildOptions.builder().addStarlarkOption(label, Lists.newArrayList("a")).build();
+    BuildOptions optionsWithSet =
+        BuildOptions.builder()
+            .addStarlarkOption(label, new LinkedHashSet<>(ImmutableList.of("a")))
+            .build();
+
+    assertThat(optionsWithList).isNotEqualTo(optionsWithSet);
+    assertThat(optionsWithList.checksum()).isNotEqualTo(optionsWithSet.checksum());
+  }
+
+  @Test
+  public void emptyListDifferentFromListWithEmptyString() {
+    Label label = Label.parseCanonicalUnchecked("//pkg:option");
+
+    BuildOptions emptyListOptions =
+        BuildOptions.builder().addStarlarkOption(label, new ArrayList<>()).build();
+    BuildOptions emptyStringListOptions =
+        BuildOptions.builder().addStarlarkOption(label, Lists.newArrayList("")).build();
+
+    assertThat(emptyListOptions).isNotEqualTo(emptyStringListOptions);
+    assertThat(emptyListOptions.checksum()).isNotEqualTo(emptyStringListOptions.checksum());
+  }
+
+  @Test
+  public void emptySetDifferentFromSetWithEmptyString() {
+    Label label = Label.parseCanonicalUnchecked("//pkg:option");
+
+    BuildOptions emptySetOptions =
+        BuildOptions.builder().addStarlarkOption(label, new LinkedHashSet<>()).build();
+    BuildOptions emptyStringSetOptions =
+        BuildOptions.builder()
+            .addStarlarkOption(label, new LinkedHashSet<>(ImmutableList.of("")))
+            .build();
+
+    assertThat(emptySetOptions).isNotEqualTo(emptyStringSetOptions);
+    assertThat(emptySetOptions.checksum()).isNotEqualTo(emptyStringSetOptions.checksum());
   }
 }