From 14c0f40fbbc313524055e514ba07be3a604af77b Mon Sep 17 00:00:00 2001 From: laurentlb Date: Fri, 9 Nov 2018 13:59:34 -0800 Subject: [PATCH] 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 --- .../devtools/build/lib/syntax/Eval.java | 7 +-- .../build/lib/syntax/LoadStatement.java | 58 +++++++++++++------ .../devtools/build/lib/syntax/Parser.java | 44 ++++++++------ .../build/lib/syntax/SyntaxTreeVisitor.java | 4 +- .../lib/syntax/ValidationEnvironment.java | 4 +- .../build/lib/syntax/ASTPrettyPrintTest.java | 6 +- .../devtools/build/lib/syntax/ParserTest.java | 18 +++--- .../skylint/AstVisitorWithNameResolution.java | 10 ++-- .../skylark/skylint/DeprecationChecker.java | 6 +- 9 files changed, 92 insertions(+), 65 deletions(-) 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 d7e30dd4ad20a8..42e798e93f2df5 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 execIf(IfStatement node) throws EvalException, InterruptedException { } void execLoad(LoadStatement node) throws EvalException, InterruptedException { - for (Map.Entry 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 9134075e0c0d91..cb2a7e7ddc57eb 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 symbolMap; + /** + * Binding represents a binding in a load statement. load("...", local = "orig") + * + *

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 bindings; private final StringLiteral imp; /** * Constructs an import statement. * - *

{@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. + *

{@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 symbolMap) { + public LoadStatement(StringLiteral imp, List bindings) { this.imp = imp; - this.symbolMap = ImmutableMap.copyOf(symbolMap); - } - - public ImmutableMap getSymbolMap() { - return symbolMap; + this.bindings = ImmutableList.copyOf(bindings); } - public ImmutableList getSymbols() { - return symbolMap.keySet().asList(); + public ImmutableList 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('"'); 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 3524e84d551508..dea8f33407d65e 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 @@ private void parseLoad(List list) { } expect(TokenKind.COMMA); - Map symbols = new HashMap<>(); - parseLoadSymbol(symbols); // At least one symbol is required + ImmutableList.Builder bindings = ImmutableList.builder(); + // previousSymbols is used to detect duplicate symbols in the same statement. + Set 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 @@ private void parseLoad(List list) { 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 list) { /** * Parses the next symbol argument of a load statement and puts it into the output map. * - *

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. + *

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 symbols) { + private void parseLoadSymbol( + ImmutableList.Builder symbols, Set 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 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 9845a8ecb1191a..117bbf05048bf0 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(ForStatement node) { } 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 0ffadaa114cc03..e71670d04b0403 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 @@ 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: diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ASTPrettyPrintTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ASTPrettyPrintTest.java index ab2f23f6e1417c..73107545a9e77a 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/ASTPrettyPrintTest.java +++ b/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"); diff --git a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java index 8adda27b2bba4d..78de4f957c930c 100644 --- a/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java +++ b/src/test/java/com/google/devtools/build/lib/syntax/ParserTest.java @@ -1221,8 +1221,8 @@ public void testLoadOneSymbol() throws Exception { List 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,7 +1234,7 @@ public void testLoadOneSymbolWithTrailingComma() throws Exception { List 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 @@ -1242,7 +1242,7 @@ public void testLoadMultipleSymbols() throws Exception { List 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 statements = parseFileForSkylark("load('//foo/bar:file.bzl', my_alias = 'lawl')\n"); LoadStatement stmt = (LoadStatement) statements.get(0); - ImmutableList actualSymbols = stmt.getSymbols(); + ImmutableList 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 statements = parseFileForSkylark(String.format("load('//foo/bar:file.bzl', %s)\n", loadSymbolString)); LoadStatement stmt = (LoadStatement) statements.get(0); - ImmutableList actualSymbols = stmt.getSymbols(); + ImmutableList actualSymbols = stmt.getBindings(); assertThat(actualSymbols).hasSize(expectedSymbols.length); List 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); diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/AstVisitorWithNameResolution.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/AstVisitorWithNameResolution.java index 5c09bc504a64d5..b39ba112c4ad2f 100644 --- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/AstVisitorWithNameResolution.java +++ b/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 symbolMap = node.getSymbolMap(); - for (Map.Entry 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()); } } diff --git a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecationChecker.java b/src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecationChecker.java index 893e48e7ca46a3..602374a989c48c 100644 --- a/src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecationChecker.java +++ b/src/tools/skylark/java/com/google/devtools/skylark/skylint/DeprecationChecker.java @@ -128,9 +128,9 @@ public Map loadDependency( LoadStatement stmt, Path loadedPath, Map loadedFileInfo) { - for (Map.Entry 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);