Starlark AST: use a list of bindings instead of a map in load statements

- The map was not useful: all we want to do is to iterate on the bindings
- Using Identifier as a key is not well defined (we shouldn't compare them and they shouldn't be hashable in the first place)
- Using Identifier instead of String for the original name allows us to preserve the location in the AST

RELNOTES: None.
PiperOrigin-RevId: 220860272

src/main/java/com/google/devtools/build/lib/syntax/Eval.java

@@ -17,7 +17,6 @@
 import com.google.common.collect.ImmutableList;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Map;
 import java.util.function.Function;
 
 /**
@@ -145,10 +144,10 @@ void execIf(IfStatement node) throws EvalException, InterruptedException {
   }
 
   void execLoad(LoadStatement node) throws EvalException, InterruptedException {
-    for (Map.Entry<Identifier, String> entry : node.getSymbolMap().entrySet()) {
+    for (LoadStatement.Binding binding : node.getBindings()) {
       try {
-        Identifier name = entry.getKey();
-        Identifier declared = Identifier.of(entry.getValue());
+        Identifier name = binding.getLocalName();
+        Identifier declared = binding.getOriginalName();
 
         if (declared.isPrivate()) {
           throw new EvalException(

src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java

@@ -14,34 +14,53 @@
 package com.google.devtools.build.lib.syntax;
 
 import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableMap;
 import java.io.IOException;
-import java.util.Map;
+import java.io.Serializable;
+import java.util.List;
 
 /** Syntax node for an import statement. */
 public final class LoadStatement extends Statement {
 
-  private final ImmutableMap<Identifier, String> symbolMap;
+  /**
+   * Binding represents a binding in a load statement. load("...", local = "orig")
+   *
+   * <p>If there's no alias, a single Identifier can be used for both local and orig.
+   */
+  public static final class Binding implements Serializable {
+    private final Identifier local;
+    private final Identifier orig;
+
+    public Identifier getLocalName() {
+      return local;
+    }
+
+    public Identifier getOriginalName() {
+      return orig;
+    }
+
+    public Binding(Identifier localName, Identifier originalName) {
+      this.local = localName;
+      this.orig = originalName;
+    }
+  }
+
+  private final ImmutableList<Binding> bindings;
   private final StringLiteral imp;
 
   /**
    * Constructs an import statement.
    *
-   * <p>{@code symbols} maps a symbol to the original name under which it was defined in
-   * the bzl file that should be loaded. If aliasing is used, the value differs from its key's
-   * {@code symbol.getName()}. Otherwise, both values are identical.
+   * <p>{@code symbols} maps a symbol to the original name under which it was defined in the bzl
+   * file that should be loaded. If aliasing is used, the value differs from its key's {@code
+   * symbol.getName()}. Otherwise, both values are identical.
    */
-  public LoadStatement(StringLiteral imp, Map<Identifier, String> symbolMap) {
+  public LoadStatement(StringLiteral imp, List<Binding> bindings) {
     this.imp = imp;
-    this.symbolMap = ImmutableMap.copyOf(symbolMap);
-  }
-
-  public ImmutableMap<Identifier, String> getSymbolMap() {
-    return symbolMap;
+    this.bindings = ImmutableList.copyOf(bindings);
   }
 
-  public ImmutableList<Identifier> getSymbols() {
-    return symbolMap.keySet().asList();
+  public ImmutableList<Binding> getBindings() {
+    return bindings;
   }
 
   public StringLiteral getImport() {
@@ -53,15 +72,16 @@ public void prettyPrint(Appendable buffer, int indentLevel) throws IOException {
     printIndent(buffer, indentLevel);
     buffer.append("load(");
     imp.prettyPrint(buffer);
-    for (Identifier symbol : symbolMap.keySet()) {
+    for (Binding binding : bindings) {
       buffer.append(", ");
-      String origName = symbolMap.get(symbol);
-      if (origName.equals(symbol.getName())) {
+      Identifier local = binding.getLocalName();
+      String origName = binding.getOriginalName().getName();
+      if (origName.equals(local.getName())) {
         buffer.append('"');
-        symbol.prettyPrint(buffer);
+        local.prettyPrint(buffer);
         buffer.append('"');
       } else {
-        symbol.prettyPrint(buffer);
+        local.prettyPrint(buffer);
         buffer.append("=\"");
         buffer.append(origName);
         buffer.append('"');

src/main/java/com/google/devtools/build/lib/syntax/Parser.java

@@ -30,9 +30,10 @@
 import com.google.devtools.build.lib.syntax.IfStatement.ConditionalStatements;
 import java.util.ArrayList;
 import java.util.EnumSet;
-import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import javax.annotation.Nullable;
 
 /**
@@ -1039,19 +1040,22 @@ private void parseLoad(List<Statement> list) {
     }
     expect(TokenKind.COMMA);
 
-    Map<Identifier, String> symbols = new HashMap<>();
-    parseLoadSymbol(symbols); // At least one symbol is required
+    ImmutableList.Builder<LoadStatement.Binding> bindings = ImmutableList.builder();
+    // previousSymbols is used to detect duplicate symbols in the same statement.
+    Set<String> previousSymbols = new HashSet<>();
+
+    parseLoadSymbol(bindings, previousSymbols); // At least one symbol is required
 
     while (token.kind != TokenKind.RPAREN && token.kind != TokenKind.EOF) {
       expect(TokenKind.COMMA);
       if (token.kind == TokenKind.RPAREN) {
         break;
       }
 
-      parseLoadSymbol(symbols);
+      parseLoadSymbol(bindings, previousSymbols);
     }
 
-    LoadStatement stmt = new LoadStatement(importString, symbols);
+    LoadStatement stmt = new LoadStatement(importString, bindings.build());
     list.add(setLocation(stmt, start, token.right));
     expect(TokenKind.RPAREN);
     expectAndRecover(TokenKind.NEWLINE);
@@ -1060,39 +1064,41 @@ private void parseLoad(List<Statement> list) {
   /**
    * Parses the next symbol argument of a load statement and puts it into the output map.
    *
-   * <p> The symbol is either "name" (STRING) or name = "declared" (IDENTIFIER EQUALS STRING).
-   * If no alias is used, "name" and "declared" will be identical. "Declared" refers to the
-   * original name in the Bazel file that should be loaded, while "name" will be the key of the
-   * entry in the map.
+   * <p>The symbol is either "name" (STRING) or name = "declared" (IDENTIFIER EQUALS STRING). If no
+   * alias is used, "name" and "declared" will be identical. "Declared" refers to the original name
+   * in the Bazel file that should be loaded, while "name" will be the key of the entry in the map.
    */
-  private void parseLoadSymbol(Map<Identifier, String> symbols) {
+  private void parseLoadSymbol(
+      ImmutableList.Builder<LoadStatement.Binding> symbols, Set<String> previousSymbols) {
     if (token.kind != TokenKind.STRING && token.kind != TokenKind.IDENTIFIER) {
       syntaxError("expected either a literal string or an identifier");
       return;
     }
 
     String name = (String) token.value;
-    Identifier identifier = Identifier.of(name);
-    if (symbols.containsKey(identifier)) {
-      syntaxError(
-          String.format("Identifier '%s' is used more than once", identifier.getName()));
+    Identifier local = setLocation(Identifier.of(name), token.left, token.right);
+
+    if (previousSymbols.contains(local.getName())) {
+      syntaxError(String.format("Identifier '%s' is used more than once", local.getName()));
     }
-    setLocation(identifier, token.left, token.right);
+    previousSymbols.add(local.getName());
 
-    String declared;
+    Identifier original;
     if (token.kind == TokenKind.STRING) {
-      declared = name;
+      // load(..., "name")
+      original = local;
     } else {
+      // load(..., local = "orig")
       expect(TokenKind.IDENTIFIER);
       expect(TokenKind.EQUALS);
       if (token.kind != TokenKind.STRING) {
         syntaxError("expected string");
         return;
       }
-      declared = token.value.toString();
+      original = setLocation(Identifier.of((String) token.value), token.left, token.right);
     }
     nextToken();
-    symbols.put(identifier, declared);
+    symbols.add(new LoadStatement.Binding(local, original));
   }
 
   private void parseTopLevelStatement(List<Statement> list) {

src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java

@@ -92,7 +92,9 @@ public void visit(ForStatement node) {
   }
 
   public void visit(LoadStatement node) {
-    visitAll(node.getSymbols());
+    for (LoadStatement.Binding binding : node.getBindings()) {
+      visit(binding.getLocalName());
+    }
   }
 
   public void visit(ListLiteral node) {

src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java

@@ -134,8 +134,8 @@ private void collectDefinitions(Statement stmt) {
         declare(fctName.getName(), fctName.getLocation());
         break;
       case LOAD:
-        for (Identifier id : ((LoadStatement) stmt).getSymbols()) {
-          declare(id.getName(), id.getLocation());
+        for (LoadStatement.Binding binding : ((LoadStatement) stmt).getBindings()) {
+          declare(binding.getLocalName().getName(), binding.getLocalName().getLocation());
         }
         break;
       case CONDITIONAL:

src/test/java/com/google/devtools/build/lib/syntax/ASTPrettyPrintTest.java

@@ -19,7 +19,7 @@
 import static com.google.devtools.build.lib.syntax.Parser.ParsingLevel.TOP_LEVEL;
 
 import com.google.common.base.Joiner;
-import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.syntax.util.EvaluationTestCase;
 import java.io.IOException;
 import org.junit.Test;
@@ -376,7 +376,9 @@ public void loadStatement() {
     ASTNode loadStatement =
         new LoadStatement(
             new StringLiteral("foo.bzl"),
-            ImmutableMap.of(Identifier.of("a"), "A", Identifier.of("B"), "B"));
+            ImmutableList.of(
+                new LoadStatement.Binding(Identifier.of("a"), Identifier.of("A")),
+                new LoadStatement.Binding(Identifier.of("B"), Identifier.of("B"))));
     assertIndentedPrettyMatches(
         loadStatement,
         "  load(\"foo.bzl\", a=\"A\", \"B\")\n");

src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java

@@ -1221,8 +1221,8 @@ public void testLoadOneSymbol() throws Exception {
     List<Statement> statements = parseFileForSkylark("load('//foo/bar:file.bzl', 'fun_test')\n");
     LoadStatement stmt = (LoadStatement) statements.get(0);
     assertThat(stmt.getImport().getValue()).isEqualTo("//foo/bar:file.bzl");
-    assertThat(stmt.getSymbols()).hasSize(1);
-    Identifier sym = stmt.getSymbols().get(0);
+    assertThat(stmt.getBindings()).hasSize(1);
+    Identifier sym = stmt.getBindings().get(0).getLocalName();
     int startOffset = sym.getLocation().getStartOffset();
     int endOffset = sym.getLocation().getEndOffset();
     assertThat(startOffset).named("getStartOffset()").isEqualTo(27);
@@ -1234,15 +1234,15 @@ public void testLoadOneSymbolWithTrailingComma() throws Exception {
     List<Statement> statements = parseFileForSkylark("load('//foo/bar:file.bzl', 'fun_test',)\n");
     LoadStatement stmt = (LoadStatement) statements.get(0);
     assertThat(stmt.getImport().getValue()).isEqualTo("//foo/bar:file.bzl");
-    assertThat(stmt.getSymbols()).hasSize(1);
+    assertThat(stmt.getBindings()).hasSize(1);
   }
 
   @Test
   public void testLoadMultipleSymbols() throws Exception {
     List<Statement> statements = parseFileForSkylark("load(':file.bzl', 'foo', 'bar')\n");
     LoadStatement stmt = (LoadStatement) statements.get(0);
     assertThat(stmt.getImport().getValue()).isEqualTo(":file.bzl");
-    assertThat(stmt.getSymbols()).hasSize(2);
+    assertThat(stmt.getBindings()).hasSize(2);
   }
 
   @Test
@@ -1278,10 +1278,10 @@ public void testLoadAlias() throws Exception {
     List<Statement> statements =
         parseFileForSkylark("load('//foo/bar:file.bzl', my_alias = 'lawl')\n");
     LoadStatement stmt = (LoadStatement) statements.get(0);
-    ImmutableList<Identifier> actualSymbols = stmt.getSymbols();
+    ImmutableList<LoadStatement.Binding> actualSymbols = stmt.getBindings();
 
     assertThat(actualSymbols).hasSize(1);
-    Identifier sym = actualSymbols.get(0);
+    Identifier sym = actualSymbols.get(0).getLocalName();
     assertThat(sym.getName()).isEqualTo("my_alias");
     int startOffset = sym.getLocation().getStartOffset();
     int endOffset = sym.getLocation().getEndOffset();
@@ -1299,14 +1299,14 @@ private void runLoadAliasTestForSymbols(String loadSymbolString, String... expec
     List<Statement> statements =
         parseFileForSkylark(String.format("load('//foo/bar:file.bzl', %s)\n", loadSymbolString));
     LoadStatement stmt = (LoadStatement) statements.get(0);
-    ImmutableList<Identifier> actualSymbols = stmt.getSymbols();
+    ImmutableList<LoadStatement.Binding> actualSymbols = stmt.getBindings();
 
     assertThat(actualSymbols).hasSize(expectedSymbols.length);
 
     List<String> actualSymbolNames = new LinkedList<>();
 
-    for (Identifier identifier : actualSymbols) {
-      actualSymbolNames.add(identifier.getName());
+    for (LoadStatement.Binding binding : actualSymbols) {
+      actualSymbolNames.add(binding.getLocalName().getName());
     }
 
     assertThat(actualSymbolNames).containsExactly((Object[]) expectedSymbols);

src/tools/skylark/java/com/google/devtools/skylark/skylint/AstVisitorWithNameResolution.java

@@ -31,7 +31,6 @@
 import com.google.devtools.build.lib.syntax.Statement;
 import com.google.devtools.build.lib.syntax.SyntaxTreeVisitor;
 import java.util.Collection;
-import java.util.Map;
 
 /**
  * AST visitor that keeps track of which symbols are in scope.
@@ -76,11 +75,10 @@ public void visit(BuildFileAST node) {
 
   @Override
   public void visit(LoadStatement node) {
-    Map<Identifier, String> symbolMap = node.getSymbolMap();
-    for (Map.Entry<Identifier, String> entry : symbolMap.entrySet()) {
-      String name = entry.getKey().getName();
-      env.addImported(name, entry.getKey());
-      declare(name, entry.getKey());
+    for (LoadStatement.Binding binding : node.getBindings()) {
+      String name = binding.getLocalName().getName();
+      env.addImported(name, binding.getLocalName());
+      declare(name, binding.getLocalName());
     }
   }
 

src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecationChecker.java

@@ -128,9 +128,9 @@ public Map<String, DeprecatedSymbol> loadDependency(
         LoadStatement stmt,
         Path loadedPath,
         Map<String, DeprecatedSymbol> loadedFileInfo) {
-      for (Map.Entry<Identifier, String> entry : stmt.getSymbolMap().entrySet()) {
-        String originalName = entry.getValue();
-        String alias = entry.getKey().getName();
+      for (LoadStatement.Binding binding : stmt.getBindings()) {
+        String originalName = binding.getOriginalName().getName();
+        String alias = binding.getLocalName().getName();
         DeprecatedSymbol originalDeprecation = loadedFileInfo.get(originalName);
         if (originalDeprecation != null) {
           currentFileInfo.put(alias, originalDeprecation);