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
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java b/src/main/java/com/google/devtools/build/lib/syntax/Eval.java index d7e30dd..42e798e 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Eval.java +++ b/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 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(
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java index 9134075..cb2a7e7 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/LoadStatement.java +++ b/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); + this.bindings = ImmutableList.copyOf(bindings); } - public ImmutableMap<Identifier, String> getSymbolMap() { - return symbolMap; - } - - public ImmutableList<Identifier> getSymbols() { - return symbolMap.keySet().asList(); + public ImmutableList<Binding> getBindings() { + return bindings; } public StringLiteral getImport() { @@ -53,15 +72,16 @@ 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('"');
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Parser.java b/src/main/java/com/google/devtools/build/lib/syntax/Parser.java index 3524e84..dea8f33 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/Parser.java +++ b/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,8 +1040,11 @@ } 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); @@ -1048,10 +1052,10 @@ 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 @@ /** * 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())); - } - setLocation(identifier, token.left, token.right); + Identifier local = setLocation(Identifier.of(name), token.left, token.right); - String declared; + if (previousSymbols.contains(local.getName())) { + syntaxError(String.format("Identifier '%s' is used more than once", local.getName())); + } + previousSymbols.add(local.getName()); + + 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) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java b/src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java index 9845a8e..117bbf0 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/SyntaxTreeVisitor.java
@@ -92,7 +92,9 @@ } public void visit(LoadStatement node) { - visitAll(node.getSymbols()); + for (LoadStatement.Binding binding : node.getBindings()) { + visit(binding.getLocalName()); + } } public void visit(ListLiteral node) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java index 0ffadaa..e71670d 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/ValidationEnvironment.java
@@ -134,8 +134,8 @@ 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: