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:
