Preserve insertion order in maps that are traversed

Consumers of an `Immutable{List,Map,Set}` cannot know whether it has been created from a source with unspecified iteration order, such as `HashMap`. Since `ImmutableMap` itself prescribes that it preserves insertion order, it's reasonable for consumers to expect a specified, reasonable, deterministic order. While the current implementation of `HashMap` in the OpenJDK still results in a deterministic order, this kind of "sequenced-washing" doesn't cause non-determinism, but it still runs the risk of having the order of elements change between JDK versions or vendors.

The locations changed in this PR have been identified by instrumenting the factory methods of the `Immutable{List,Map,Set}` classes to track whether they 1) have been created from an unsequenced collection and 2) are iterated over. I then manually picked out those cases in which I couldn't rule out that a change in order would be observable to users.

Related to google/guava#6903 (comment)

Author: fmeum

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

@@ -66,7 +66,7 @@
 import java.io.OutputStream;
 import java.lang.reflect.Constructor;
 import java.util.ArrayList;
-import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -152,10 +152,10 @@ public static final class Builder implements RuleDefinitionEnvironment {
     private final List<Class<? extends Fragment>> configurationFragmentClasses = new ArrayList<>();
     private final List<Class<? extends FragmentOptions>> configurationOptions = new ArrayList<>();
 
-    private final Map<String, RuleClass> ruleClassMap = new HashMap<>();
-    private final Map<String, RuleDefinition> ruleDefinitionMap = new HashMap<>();
-    private final Map<String, NativeAspectClass> nativeAspectClassMap = new HashMap<>();
-    private final Map<Class<? extends RuleDefinition>, RuleClass> ruleMap = new HashMap<>();
+    private final Map<String, RuleClass> ruleClassMap = new LinkedHashMap<>();
+    private final Map<String, RuleDefinition> ruleDefinitionMap = new LinkedHashMap<>();
+    private final Map<String, NativeAspectClass> nativeAspectClassMap = new LinkedHashMap<>();
+    private final Map<Class<? extends RuleDefinition>, RuleClass> ruleMap = new LinkedHashMap<>();
     private final Digraph<Class<? extends RuleDefinition>> dependencyGraph = new Digraph<>();
     private final List<Class<? extends Fragment>> universalFragments = new ArrayList<>();
     @Nullable private TransitionFactory<RuleTransitionData> trimmingTransitionFactory = null;

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

@@ -24,7 +24,6 @@
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.ImmutableTable;
 import com.google.common.collect.Iterables;
-import com.google.common.collect.Maps;
 import com.google.common.collect.Table;
 import com.google.devtools.build.lib.analysis.config.ToolchainTypeRequirement;
 import com.google.devtools.build.lib.analysis.platform.PlatformInfo;
@@ -60,7 +59,7 @@ public static ImmutableMap<String, ExecGroup> process(
       ImmutableSet<ToolchainTypeRequirement> defaultToolchainTypes,
       boolean useAutoExecGroups) {
     var processedGroups =
-        Maps.<String, ExecGroup>newHashMapWithExpectedSize(
+        ImmutableMap.<String, ExecGroup>builderWithExpectedSize(
             useAutoExecGroups
                 ? (execGroups.size() + defaultToolchainTypes.size())
                 : execGroups.size());
@@ -108,7 +107,7 @@ public static ImmutableMap<String, ExecGroup> process(
                 .build());
       }
     }
-    return ImmutableMap.copyOf(processedGroups);
+    return processedGroups.buildOrThrow();
   }
 
   /** Builder class for correctly constructing ExecGroupCollection instances. */

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

@@ -14,7 +14,7 @@
 package com.google.devtools.build.lib.analysis;
 
 import static com.google.common.collect.ImmutableSet.toImmutableSet;
-import static com.google.common.collect.Maps.newHashMapWithExpectedSize;
+import static com.google.common.collect.Maps.newLinkedHashMapWithExpectedSize;
 import static java.util.Objects.requireNonNull;
 
 import com.google.common.base.Preconditions;
@@ -25,8 +25,8 @@
 import com.google.devtools.build.lib.packages.ExecGroup;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
-import java.util.HashMap;
-import java.util.Map;
+import java.util.LinkedHashMap;
+import java.util.SequencedMap;
 
 /**
  * A wrapper class for a map of exec_group names to their relevant ToolchainContext.
@@ -88,14 +88,14 @@ public static <T extends ToolchainContext> Builder<T> builderWithExpectedSize(in
   /** Builder for ToolchainCollection. */
   public static final class Builder<T extends ToolchainContext> {
     // This is not immutable so that we can check for duplicate keys easily.
-    private final Map<String, T> toolchainContexts;
+    private final SequencedMap<String, T> toolchainContexts;
 
     private Builder() {
-      this.toolchainContexts = new HashMap<>();
+      this.toolchainContexts = new LinkedHashMap<>();
     }
 
     private Builder(int expectedSize) {
-      this.toolchainContexts = newHashMapWithExpectedSize(expectedSize);
+      this.toolchainContexts = newLinkedHashMapWithExpectedSize(expectedSize);
     }
 
     public ToolchainCollection<T> build() {

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

@@ -600,11 +600,11 @@ public boolean shouldCollectCodeCoverageForGeneratedFiles() {
    * <p>Command-line definitions of make environments override variables defined by {@code
    * Fragment.addGlobalMakeVariables()}.
    */
-  public Map<String, String> getMakeEnvironment() {
-    Map<String, String> makeEnvironment = new HashMap<>();
+  public ImmutableMap<String, String> getMakeEnvironment() {
+    ImmutableMap.Builder<String, String> makeEnvironment = ImmutableMap.builder();
     makeEnvironment.putAll(globalMakeEnv);
     makeEnvironment.putAll(commandLineBuildVariables);
-    return ImmutableMap.copyOf(makeEnvironment);
+    return makeEnvironment.build();
   }
 
   /**

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

@@ -29,12 +29,12 @@
 import com.google.devtools.common.options.OptionDefinition;
 import com.google.devtools.common.options.OptionsParser;
 import java.util.ArrayList;
-import java.util.HashMap;
 import java.util.HashSet;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.SequencedMap;
 import java.util.Set;
 
 /**
@@ -138,7 +138,7 @@ public static OptionsDiff diff(OptionsDiff diff, BuildOptions first, BuildOption
   private final Map<Label, Object> starlarkSecond = new LinkedHashMap<>();
 
   private final List<Label> extraStarlarkOptionsFirst = new ArrayList<>();
-  private final Map<Label, Object> extraStarlarkOptionsSecond = new HashMap<>();
+  private final SequencedMap<Label, Object> extraStarlarkOptionsSecond = new LinkedHashMap<>();
 
   private boolean hasStarlarkOptions = false;
 

src/main/java/com/google/devtools/build/lib/analysis/platform/PlatformProperties.java

@@ -17,8 +17,9 @@
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableMap;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
-import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.Map;
+import java.util.SequencedMap;
 import javax.annotation.Nullable;
 
 /** Proepeties set on a specific {@link PlatformInfo}. */
@@ -70,8 +71,8 @@ private static ImmutableMap<String, String> mergeParent(
         return properties;
       }
 
-      HashMap<String, String> result = new HashMap<>();
-      if (parent != null && !parent.properties().isEmpty()) {
+      SequencedMap<String, String> result = new LinkedHashMap<>();
+      if (!parent.properties().isEmpty()) {
         result.putAll(parent.properties());
       }
 

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Discovery.java

@@ -60,7 +60,7 @@ public static Result run(Environment env, RootModuleFileValue root)
       throws InterruptedException, ExternalDepsException {
     String rootModuleName = root.getModule().getName();
     ImmutableMap<String, ModuleOverride> overrides = root.getOverrides();
-    Map<ModuleKey, InterimModule> depGraph = new HashMap<>();
+    Map<ModuleKey, InterimModule> depGraph = new LinkedHashMap<>();
     depGraph.put(
         ModuleKey.ROOT,
         root.getModule()

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunction.java

@@ -70,10 +70,11 @@
 import com.google.errorprone.annotations.FormatMethod;
 import java.io.IOException;
 import java.util.ArrayList;
-import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.SequencedMap;
 import java.util.stream.Stream;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.Dict;
@@ -138,7 +139,8 @@ private static class State implements Environment.SkyKeyComputeState {
     // the `includeLabelToCompiledModuleFile` map for use during actual Starlark execution.
     CompiledModuleFile compiledRootModuleFile;
     ImmutableList<IncludeStatement> horizon;
-    HashMap<String, CompiledModuleFile> includeLabelToCompiledModuleFile = new HashMap<>();
+    SequencedMap<String, CompiledModuleFile> includeLabelToCompiledModuleFile =
+        new LinkedHashMap<>();
   }
 
   @Nullable
@@ -313,7 +315,7 @@ private SkyValue computeForRootModule(
    */
   @Nullable
   private static ImmutableList<IncludeStatement> advanceHorizon(
-      HashMap<String, CompiledModuleFile> includeLabelToCompiledModuleFile,
+      SequencedMap<String, CompiledModuleFile> includeLabelToCompiledModuleFile,
       ImmutableList<IncludeStatement> horizon,
       Environment env,
       StarlarkSemantics starlarkSemantics,

src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/HttpConnectorMultiplexer.java

@@ -31,7 +31,6 @@
 import java.net.URISyntaxException;
 import java.net.URL;
 import java.net.URLConnection;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
@@ -139,17 +138,16 @@ static Function<URL, ImmutableMap<String, List<String>>> getHeaderFunction(
     Preconditions.checkNotNull(credentials);
 
     return url -> {
-      Map<String, List<String>> headers = new HashMap<>(baseHeaders);
       try {
-        headers.putAll(credentials.getRequestMetadata(url.toURI()));
+        return ImmutableMap.copyOf(credentials.getRequestMetadata(url.toURI()));
       } catch (URISyntaxException | IOException e) {
         // If we can't convert the URL to a URI (because it is syntactically malformed), or fetching
         // credentials fails for any other reason, still try to do the connection, not adding
         // authentication information as we cannot look it up.
         eventHandler.handle(
             Event.warn("Error retrieving auth headers, continuing without: " + e.getMessage()));
+        return ImmutableMap.of();
       }
-      return ImmutableMap.copyOf(headers);
     };
   }
 }

src/main/java/com/google/devtools/build/lib/cmdline/RepositoryMapping.java

@@ -88,23 +88,28 @@ public String toString() {
   }
 
   public static RepositoryMapping create(
-      Map<String, RepositoryName> entries, RepositoryName ownerRepo) {
+      ImmutableMap<String, RepositoryName> entries, RepositoryName ownerRepo) {
     return new RepositoryMapping(
         Preconditions.checkNotNull(entries), Preconditions.checkNotNull(ownerRepo));
   }
 
-  public static RepositoryMapping createAllowingFallback(Map<String, RepositoryName> entries) {
+  public static RepositoryMapping createAllowingFallback(ImmutableMap<String, RepositoryName> entries) {
     return new RepositoryMapping(Preconditions.checkNotNull(entries), null);
   }
 
   /**
    * Create a new {@link RepositoryMapping} instance based on existing repo mappings and given
    * additional mappings. If there are conflicts, existing mappings will take precedence.
    */
-  public RepositoryMapping withAdditionalMappings(Map<String, RepositoryName> additionalMappings) {
-    HashMap<String, RepositoryName> allMappings = new HashMap<>(additionalMappings);
-    allMappings.putAll(entries());
-    return new RepositoryMapping(allMappings, ownerRepo());
+  public RepositoryMapping withAdditionalMappings(
+      ImmutableMap<String, RepositoryName> additionalMappings) {
+    return new RepositoryMapping(
+        ImmutableMap.<String, RepositoryName>builderWithExpectedSize(
+                entries().size() + additionalMappings.size())
+            .putAll(additionalMappings)
+            .putAll(entries())
+            .buildKeepingLast(),
+        ownerRepo());
   }
 
   /**

src/main/java/com/google/devtools/build/lib/exec/TestPolicy.java

@@ -19,6 +19,7 @@
 import com.google.devtools.build.lib.vfs.PathFragment;
 import java.time.Duration;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.Map;
 
 /**
@@ -64,7 +65,7 @@ public Map<String, String> computeTestEnvironment(
       Duration timeout,
       PathFragment relativeRunfilesDir,
       PathFragment tmpDir) {
-    Map<String, String> env = new HashMap<>();
+    Map<String, String> env = new LinkedHashMap<>();
 
     // Add all env variables, allow some string replacements and inheritance.
     String userProp = UserUtils.getUserName();

src/main/java/com/google/devtools/build/lib/metrics/PostGCMemoryUseRecorder.java

@@ -18,6 +18,7 @@
 import com.google.common.base.Supplier;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSortedMap;
 import com.google.common.flogger.GoogleLogger;
 import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
 import com.google.devtools.build.lib.bugreport.BugReporter;
@@ -128,7 +129,7 @@ public synchronized boolean wasMemoryUsageReportedZero() {
    * Returns the number of bytes garbage collected during this invocation. Broken down by GC space.
    */
   public synchronized ImmutableMap<String, Long> getGarbageStats() {
-    return ImmutableMap.copyOf(garbageStats);
+    return ImmutableSortedMap.copyOf(garbageStats);
   }
 
   public synchronized void reset() {

src/main/java/com/google/devtools/build/lib/packages/BazelStarlarkEnvironment.java

@@ -18,8 +18,10 @@
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Sets;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.SequencedMap;
 import java.util.Set;
 import javax.annotation.Nullable;
 import net.starlark.java.eval.GuardedValue;
@@ -458,11 +460,11 @@ private ImmutableMap<String, Object> createBzlEnvUsingInjection(
       throws InjectionException {
     Map<String, Boolean> overridesMap = parseInjectionOverridesList(overridesList);
 
-    Map<String, Object> env = new HashMap<>(bzlToplevelsWithoutNative);
+    SequencedMap<String, Object> env = new LinkedHashMap<>(bzlToplevelsWithoutNative);
 
     // Determine "native" bindings.
     // TODO(#11954): See above comment in createUninjectedBuildBzlEnv.
-    Map<String, Object> nativeBindings = new HashMap<>(nativeBase);
+    SequencedMap<String, Object> nativeBindings = new LinkedHashMap<>(nativeBase);
     for (Map.Entry<String, Object> entry : exportedRules.entrySet()) {
       String key = entry.getKey();
       String name = getKeySuffix(key);
@@ -508,7 +510,7 @@ public ImmutableMap<String, Object> createBuildEnvUsingInjection(
       Map<String, Object> exportedRules, List<String> overridesList) throws InjectionException {
     Map<String, Boolean> overridesMap = parseInjectionOverridesList(overridesList);
 
-    HashMap<String, Object> env = new HashMap<>(uninjectedBuildEnv);
+    SequencedMap<String, Object> env = new LinkedHashMap<>(uninjectedBuildEnv);
     for (Map.Entry<String, Object> entry : exportedRules.entrySet()) {
       String key = entry.getKey();
       String name = getKeySuffix(key);

src/main/java/com/google/devtools/build/lib/packages/Package.java

@@ -61,12 +61,14 @@
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.HashSet;
+import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Optional;
 import java.util.OptionalInt;
 import java.util.OptionalLong;
+import java.util.SequencedMap;
 import java.util.Set;
 import java.util.concurrent.Semaphore;
 import javax.annotation.Nullable;
@@ -1107,7 +1109,7 @@ default boolean precomputeTransitiveLoads() {
 
     // The map from each repository to that repository's remappings map.
     // This is only used in the //external package, it is an empty map for all other packages.
-    private final HashMap<RepositoryName, HashMap<String, RepositoryName>>
+    private final HashMap<RepositoryName, LinkedHashMap<String, RepositoryName>>
         externalPackageRepositoryMappings = new HashMap<>();
 
     /** Estimates the package overhead of this package. */
@@ -1357,7 +1359,7 @@ Builder addRepositoryMappingEntry(
         RepositoryName repoWithin, String localName, RepositoryName mappedName) {
       HashMap<String, RepositoryName> mapping =
           externalPackageRepositoryMappings.computeIfAbsent(
-              repoWithin, (RepositoryName k) -> new HashMap<>());
+              repoWithin, (RepositoryName k) -> new LinkedHashMap<>());
       mapping.put(localName, mappedName);
       return this;
     }
@@ -1387,11 +1389,11 @@ Builder addRepositoryMappings(Package aPackage) {
      * the main workspace to the canonical main name '@').
      */
     RepositoryMapping getRepositoryMappingFor(RepositoryName name) {
-      Map<String, RepositoryName> mapping = externalPackageRepositoryMappings.get(name);
+      SequencedMap<String, RepositoryName> mapping = externalPackageRepositoryMappings.get(name);
       if (mapping == null) {
         return RepositoryMapping.ALWAYS_FALLBACK;
       } else {
-        return RepositoryMapping.createAllowingFallback(mapping);
+        return RepositoryMapping.createAllowingFallback(ImmutableMap.copyOf(mapping));
       }
     }
 

src/main/java/com/google/devtools/build/lib/runtime/StarlarkOptionsParser.java

@@ -40,6 +40,7 @@
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
@@ -110,13 +111,13 @@ public abstract static class Builder {
   private final Map<String, String> scopes = new TreeMap<>();
 
   // Map of parsed starlark options to their loaded BuildSetting objects (used for canonicalization)
-  private final Map<String, BuildSetting> parsedBuildSettings = new HashMap<>();
+  private final Map<String, BuildSetting> parsedBuildSettings = new LinkedHashMap<>();
 
   // Local cache of build settings so we don't repeatedly load them.
   private final Map<String, Target> buildSettings = new HashMap<>();
 
   // The default value for each build setting.
-  private final Map<String, Object> buildSettingDefaults = new HashMap<>();
+  private final Map<String, Object> buildSettingDefaults = new LinkedHashMap<>();
 
   // whether options explicitly set to their default values are added to {@code starlarkOptions}
   private final boolean includeDefaultValues;