Skip to content

Commit

Permalink
Use LinkedHash(Map|Set) in more places for deterministic behavior
Browse files Browse the repository at this point in the history
It's important that the compiler always iterate over maps and sets
in a deterministic order.

PiperOrigin-RevId: 564534609
  • Loading branch information
brad4d authored and copybara-github committed Sep 11, 2023
1 parent 037ef68 commit 35345da
Show file tree
Hide file tree
Showing 17 changed files with 107 additions and 113 deletions.
8 changes: 5 additions & 3 deletions src/com/google/javascript/jscomp/CheckConformance.java
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
import com.google.protobuf.Descriptors;
import com.google.protobuf.TextFormat;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
import org.jspecify.nullness.Nullable;
Expand Down Expand Up @@ -106,7 +106,9 @@ private static final class Category {
}
}

/** @param configs The rules to check. */
/**
* @param configs The rules to check.
*/
CheckConformance(AbstractCompiler compiler, ImmutableList<ConformanceConfig> configs) {
this.compiler = compiler;
// Initialize the map of functions to inspect for renaming candidates.
Expand Down Expand Up @@ -183,7 +185,7 @@ private static ImmutableList<Category> initRules(
static List<Requirement> mergeRequirements(
AbstractCompiler compiler, List<ConformanceConfig> configs) {
List<Requirement.Builder> builders = new ArrayList<>();
Map<String, Requirement.Builder> extendable = new HashMap<>();
Map<String, Requirement.Builder> extendable = new LinkedHashMap<>();
for (ConformanceConfig config : configs) {
for (Requirement requirement : config.getRequirementList()) {
Requirement.Builder builder = requirement.toBuilder();
Expand Down
4 changes: 2 additions & 2 deletions src/com/google/javascript/jscomp/ConformanceRules.java
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
import java.lang.reflect.InvocationTargetException;
import java.util.ArrayList;
import java.util.Collection;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Locale;
import java.util.Set;
Expand Down Expand Up @@ -2021,7 +2021,7 @@ public static final class BanCreateElement extends AbstractRule {
public BanCreateElement(AbstractCompiler compiler, Requirement requirement)
throws InvalidRequirementSpec {
super(compiler, requirement);
bannedTags = new HashSet<>();
bannedTags = new LinkedHashSet<>();
for (String value : requirement.getValueList()) {
bannedTags.add(Ascii.toLowerCase(value));
}
Expand Down
12 changes: 6 additions & 6 deletions src/com/google/javascript/jscomp/CrossChunkCodeMotion.java
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,7 @@
import java.util.BitSet;
import java.util.Collection;
import java.util.Deque;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -79,7 +78,7 @@ class CrossChunkCodeMotion implements CompilerPass {
* Map from chunk to the node in that chunk that should parent variable declarations that have to
* be moved into that chunk
*/
private final Map<JSChunk, Node> moduleInsertionPointMap = new HashMap<>();
private final Map<JSChunk, Node> moduleInsertionPointMap = new LinkedHashMap<>();

private final boolean parentModuleCanSeeSymbolsDeclaredInChildren;

Expand Down Expand Up @@ -132,7 +131,7 @@ private void addInstanceofGuards(Collection<GlobalSymbol> globalSymbols) {
/** Collects all global symbols, their declaration statements and references. */
private class GlobalSymbolCollector {

final Map<Var, GlobalSymbol> globalSymbolforVar = new HashMap<>();
final Map<Var, GlobalSymbol> globalSymbolforVar = new LinkedHashMap<>();

/**
* Returning the symbols in the reverse order in which they are defined helps to minimize
Expand Down Expand Up @@ -251,9 +250,10 @@ private class GlobalSymbol {
*
* <p>This is a LinkedHashSet in order to enforce a consistent ordering when we iterate over
* these to identify cycles. This guarantees that the order in which statements are moved won't
* depend on the arbitrary ordering of HashSet.
* depend on the arbitrary ordering of LinkedHashSet.
*/
final Set<GlobalSymbol> referencingGlobalSymbols = new LinkedHashSet<>();

/** Instanceof references we may need to update with a guard after moving declarations. */
final Deque<InstanceofReference> instanceofReferencesToGuard = new ArrayDeque<>();
/** Used by OrderAndCombineGlobalSymbols to find reference cycles. */
Expand Down Expand Up @@ -422,7 +422,7 @@ private void addGuardToInstanceofReference(Node referenceNode) {
private static class DeclarationStatementGroup {

final GlobalSymbol declaredGlobalSymbol;
final Set<GlobalSymbol> referencedGlobalSymbols = new HashSet<>();
final Set<GlobalSymbol> referencedGlobalSymbols = new LinkedHashSet<>();

/** chunk containing the statements */
JSChunk currentChunk;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import com.google.javascript.rhino.Node;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import java.util.Map;
Expand All @@ -37,7 +36,7 @@
public final class CrossChunkReferenceCollector implements ScopedCallback, CompilerPass {

/** Maps global variable name to the corresponding {@link Var} object. */
private final Map<String, Var> varsByName = new HashMap<>();
private final Map<String, Var> varsByName = new LinkedHashMap<>();

/**
* Maps a given variable to a collection of references to that name. Note that Var objects are not
Expand Down
50 changes: 28 additions & 22 deletions src/com/google/javascript/jscomp/DotFormatter.java
Original file line number Diff line number Diff line change
Expand Up @@ -28,18 +28,19 @@
import com.google.javascript.rhino.jstype.JSType;
import java.io.IOException;
import java.util.Arrays;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
import org.jspecify.nullness.Nullable;

/**
* <p>DotFormatter prints out a dot file of the Abstract Syntax Tree.
* For a detailed description of the dot format and visualization tool refer
* to <a href="http://www.graphviz.org">Graphviz</a>.</p>
* <p>Typical usage of this class</p>
* <code>System.out.println(new DotFormatter().toDot(<i>node</i>));</code>
* <p>This class is <b>not</b> thread safe and should not be used without proper
* external synchronization.</p>
* DotFormatter prints out a dot file of the Abstract Syntax Tree. For a detailed description of the
* dot format and visualization tool refer to <a href="http://www.graphviz.org">Graphviz</a>.
*
* <p>Typical usage of this class <code>logfile.write(new DotFormatter().toDot(<i>node</i>));
* </code>
*
* <p>This class is <b>not</b> thread safe and should not be used without proper external
* synchronization.
*/
public final class DotFormatter {
private static final String INDENT = " ";
Expand All @@ -48,7 +49,7 @@ public final class DotFormatter {
private static final int MAX_LABEL_NAME_LENGTH = 10;

// stores the current assignment of node to keys
private final HashMap<Node, Integer> assignments = new HashMap<>();
private final LinkedHashMap<Node, Integer> assignments = new LinkedHashMap<>();

// key count in order to assign a unique key to each node
private int keyCount = 0;
Expand All @@ -67,8 +68,9 @@ private DotFormatter() {
this.printAnnotations = false;
}

private DotFormatter(Node n, ControlFlowGraph<Node> cfg,
Appendable builder, boolean printAnnotations) throws IOException {
private DotFormatter(
Node n, ControlFlowGraph<Node> cfg, Appendable builder, boolean printAnnotations)
throws IOException {
this.cfg = cfg;
this.builder = builder;
this.printAnnotations = printAnnotations;
Expand All @@ -80,10 +82,11 @@ private DotFormatter(Node n, ControlFlowGraph<Node> cfg,

/**
* Converts an AST to dot representation.
*
* @param n the root of the AST described in the dot formatted string
* @return the dot representation of the AST
*/
public static String toDot(Node n) throws IOException {
public static String toDot(Node n) throws IOException {
return toDot(n, null);
}

Expand Down Expand Up @@ -166,18 +169,17 @@ public static String toDot(GraphvizGraph graph) {

/**
* Converts an AST to dot representation and appends it to the given buffer.
*
* @param n the root of the AST described in the dot formatted string
* @param inCFG Control Flow Graph.
* @param builder A place to dump the graph.
*/
static void appendDot(Node n, ControlFlowGraph<Node> inCFG,
Appendable builder) throws IOException {
static void appendDot(Node n, ControlFlowGraph<Node> inCFG, Appendable builder)
throws IOException {
DotFormatter unused = new DotFormatter(n, inCFG, builder, false);
}

/**
* Creates a DotFormatter purely for testing DotFormatter's internal methods.
*/
/** Creates a DotFormatter purely for testing DotFormatter's internal methods. */
static DotFormatter newInstanceForTesting() {
return new DotFormatter();
}
Expand All @@ -187,8 +189,7 @@ private void traverseNodes(Node parent) throws IOException {
int keyParent = key(parent);

// edges
for (Node child = parent.getFirstChild(); child != null;
child = child.getNext()) {
for (Node child = parent.getFirstChild(); child != null; child = child.getNext()) {
int keyChild = key(child);
builder.append(INDENT);
builder.append(formatNodeName(keyParent));
Expand Down Expand Up @@ -216,9 +217,14 @@ private void traverseNodes(Node parent) throws IOException {
}

edgeList[i] =
formatNodeName(keyParent) + ARROW + toNode + " [label=\"" + edge.getValue() + "\", "
+ "fontcolor=\"red\", "
+ "weight=0.01, color=\"red\"];\n";
formatNodeName(keyParent)
+ ARROW
+ toNode
+ " [label=\""
+ edge.getValue()
+ "\", "
+ "fontcolor=\"red\", "
+ "weight=0.01, color=\"red\"];\n";
}

Arrays.sort(edgeList);
Expand Down
4 changes: 2 additions & 2 deletions src/com/google/javascript/jscomp/GenerateExports.java
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import com.google.javascript.rhino.Node;
import com.google.javascript.rhino.jstype.JSType;
import com.google.javascript.rhino.jstype.JSTypeNative;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.Map;
import java.util.Set;
import org.jspecify.nullness.Nullable;
Expand All @@ -42,7 +42,7 @@ public class GenerateExports implements CompilerPass {

private final boolean allowNonGlobalExports;

private final Set<String> exportedVariables = new HashSet<>();
private final Set<String> exportedVariables = new LinkedHashSet<>();

static final DiagnosticType MISSING_EXPORT_CONVENTION =
DiagnosticType.error(
Expand Down
43 changes: 16 additions & 27 deletions src/com/google/javascript/jscomp/IdMappingUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,38 +23,29 @@
import com.google.common.collect.HashBiMap;
import com.google.common.collect.ImmutableMap;
import com.google.javascript.jscomp.base.format.SimpleFormat;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.Map;

/**
* A utility class for generating and parsing id mappings held by {@link ReplaceIdGenerators}.
*/
/** A utility class for generating and parsing id mappings held by {@link ReplaceIdGenerators}. */
public final class IdMappingUtil {

@VisibleForTesting
static final char NEW_LINE = '\n';
@VisibleForTesting static final char NEW_LINE = '\n';

private static final Splitter LINE_SPLITTER = Splitter.on(NEW_LINE).omitEmptyStrings();

// Prevent instantiation.
private IdMappingUtil() {}

/**
* @return The serialize map of generators and their ids and their
* replacements.
* @return The serialize map of generators and their ids and their replacements.
*/
static String generateSerializedIdMappings(Map<String, Map<String, String>> idGeneratorMaps) {
StringBuilder sb = new StringBuilder();
for (Map.Entry<String, Map<String, String>> replacements : idGeneratorMaps.entrySet()) {
if (!replacements.getValue().isEmpty()) {
sb.append('[')
.append(replacements.getKey())
.append(']')
.append(NEW_LINE)
.append(NEW_LINE);
sb.append('[').append(replacements.getKey()).append(']').append(NEW_LINE).append(NEW_LINE);

for (Map.Entry<String, String> replacement :
replacements.getValue().entrySet()) {
for (Map.Entry<String, String> replacement : replacements.getValue().entrySet()) {
sb.append(replacement.getKey())
.append(':')
.append(replacement.getValue())
Expand All @@ -69,27 +60,23 @@ static String generateSerializedIdMappings(Map<String, Map<String, String>> idGe
/**
* The expected format looks like this:
*
* <p>[generatorName1]
* someId1:someFile:theLine:theColumn
* ...
* <p>[generatorName1] someId1:someFile:theLine:theColumn ...
*
* <p>[[generatorName2]
* someId2:someFile:theLine:theColumn]
* ...
* <p>[[generatorName2] someId2:someFile:theLine:theColumn] ...
*
* <p>The returned data is grouped by generator name (the map key). The inner map provides
* mappings from id to content (file, line and column info). In a glimpse, the structure is
* {@code Map<generator name, BiMap<id, value>>}.
* mappings from id to content (file, line and column info). In a glimpse, the structure is {@code
* Map<generator name, BiMap<id, value>>}.
*
* <p>@throws IllegalArgumentException malformed input where there it 1) has duplicate generator
* name, or 2) the line has no ':' for id and its content.
* name, or 2) the line has no ':' for id and its content.
*/
public static Map<String, BiMap<String, String>> parseSerializedIdMappings(String idMappings) {
if (Strings.isNullOrEmpty(idMappings)) {
return ImmutableMap.of();
}

Map<String, BiMap<String, String>> resultMap = new HashMap<>();
Map<String, BiMap<String, String>> resultMap = new LinkedHashMap<>();
BiMap<String, String> currentSectionMap = null;

int lineIndex = 0;
Expand All @@ -106,7 +93,8 @@ public static Map<String, BiMap<String, String>> parseSerializedIdMappings(Strin
resultMap.put(currentSection, currentSectionMap);
} else {
throw new IllegalArgumentException(
SimpleFormat.format("Cannot parse id map: %s\n Line: $s, lineIndex: %s",
SimpleFormat.format(
"Cannot parse id map: %s\n Line: $s, lineIndex: %s",
idMappings, line, lineIndex));
}
} else {
Expand All @@ -117,7 +105,8 @@ public static Map<String, BiMap<String, String>> parseSerializedIdMappings(Strin
currentSectionMap.put(name, location);
} else {
throw new IllegalArgumentException(
SimpleFormat.format("Cannot parse id map: %s\n Line: $s, lineIndex: %s",
SimpleFormat.format(
"Cannot parse id map: %s\n Line: $s, lineIndex: %s",
idMappings, line, lineIndex));
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/com/google/javascript/jscomp/InlineSimpleMethods.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
import com.google.javascript.rhino.IR;
import com.google.javascript.rhino.JSDocInfo;
import com.google.javascript.rhino.Node;
import java.util.HashSet;
import java.util.LinkedHashSet;
import java.util.Set;
import org.jspecify.nullness.Nullable;

Expand Down Expand Up @@ -61,7 +61,7 @@ class InlineSimpleMethods implements CompilerPass {
// - non-method properties
// - methods with @noinline
// - methods with multiple, non-equivalent definitions
private final Set<String> nonInlineableProperties = new HashSet<>();
private final Set<String> nonInlineableProperties = new LinkedHashSet<>();

// Use a linked map here to keep the output deterministic. Otherwise,
// the choice of method bodies is random when multiple identical definitions
Expand Down
10 changes: 5 additions & 5 deletions src/com/google/javascript/jscomp/Normalize.java
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@
import com.google.javascript.rhino.JSDocInfo;
import com.google.javascript.rhino.Node;
import com.google.javascript.rhino.Token;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.Map;
import java.util.Set;
import org.jspecify.nullness.Nullable;
Expand Down Expand Up @@ -167,7 +167,7 @@ public void process(Node externs, Node root) {
public void visit(NodeTraversal t, Node n, Node parent) {
// Note: Constant properties annotations are not propagated.
if (!n.isName() || n.getString().isEmpty()) {
return;
return;
}

// Find the JSDocInfo for a top-level variable
Expand Down Expand Up @@ -214,7 +214,7 @@ public void process(Node externs, Node root) {
NodeTraversal.traverseRoots(compiler, this, externs, root);
}

private final Map<String, Boolean> constantMap = new HashMap<>();
private final Map<String, Boolean> constantMap = new LinkedHashMap<>();

@Override
public void visit(NodeTraversal t, Node n, Node parent) {
Expand Down Expand Up @@ -683,7 +683,7 @@ private void removeDuplicateDeclarations(Node externs, Node root) {
private final class DuplicateDeclarationHandler
implements SyntacticScopeCreator.RedeclarationHandler {

private final Set<Var> hasOkDuplicateDeclaration = new HashSet<>();
private final Set<Var> hasOkDuplicateDeclaration = new LinkedHashSet<>();

/** Remove duplicate VAR declarations discovered during scope creation. */
@Override
Expand Down
Loading

0 comments on commit 35345da

Please sign in to comment.