Refactor AST APIs
Added public visibility to some constructors/accessors, and made child LValue nodes explicitly constructed by the caller rather than hidden inside constructors. This makes it easier to treat nodes as uniform dumb values. Also added some helpers.
RELNOTES: None
PiperOrigin-RevId: 158761415
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ASTNode.java b/src/main/java/com/google/devtools/build/lib/syntax/ASTNode.java
index 84d859f..d2e22d9 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ASTNode.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ASTNode.java
@@ -21,6 +21,10 @@
/**
* Root class for nodes in the Abstract Syntax Tree of the Build language.
+ *
+ * The standard {@link Object#equals} and {@link Object#hashCode} methods are not supported. This is
+ * because their implementation would require traversing the entire tree in the worst case, and we
+ * don't want this kind of cost to occur implicitly.
*/
public abstract class ASTNode implements Serializable {
@@ -78,7 +82,7 @@
@Override
public int hashCode() {
- throw new UnsupportedOperationException(); // avoid nondeterminism
+ throw new UnsupportedOperationException();
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/AssignmentStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/AssignmentStatement.java
index f37843b..e41e78b 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/AssignmentStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/AssignmentStatement.java
@@ -27,8 +27,8 @@
/**
* Constructs an assignment: "lvalue := value".
*/
- AssignmentStatement(Expression lvalue, Expression expression) {
- this.lvalue = new LValue(lvalue);
+ public AssignmentStatement(LValue lvalue, Expression expression) {
+ this.lvalue = lvalue;
this.expression = expression;
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/AugmentedAssignmentStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/AugmentedAssignmentStatement.java
index d869aba..5b910ce 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/AugmentedAssignmentStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/AugmentedAssignmentStatement.java
@@ -24,9 +24,9 @@
private final Expression expression;
/** Constructs an augmented assignment: "lvalue ::= value". */
- AugmentedAssignmentStatement(Operator operator, Expression lhs, Expression expression) {
+ public AugmentedAssignmentStatement(Operator operator, LValue lvalue, Expression expression) {
this.operator = operator;
- this.lvalue = new LValue(lhs);
+ this.lvalue = lvalue;
this.expression = expression;
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java b/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java
index 44f1c15..0101117 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BuildFileAST.java
@@ -33,6 +33,9 @@
/**
* Abstract syntax node for an entire BUILD file.
*/
+// TODO(bazel-team): Consider breaking this up into two classes: One that extends ASTNode and does
+// not include import info; and one that wraps that object with additional import info but that
+// does not itself extend ASTNode. This would help keep the AST minimalistic.
public class BuildFileAST extends ASTNode {
private final ImmutableList<Statement> stmts;
@@ -292,22 +295,20 @@
*
* <p>This method should not be used in Bazel code, since it doesn't validate that the imports are
* syntactically valid.
- *
- * @throws IOException if the file cannot not be read.
*/
public static BuildFileAST parseSkylarkFileWithoutImports(
- ParserInputSource input, EventHandler eventHandler) throws IOException {
+ ParserInputSource input, EventHandler eventHandler) {
ParseResult result = Parser.parseFileForSkylark(input, eventHandler);
return new BuildFileAST(
ImmutableList.<Statement>builder()
.addAll(ImmutableList.<Statement>of())
.addAll(result.statements)
.build(),
- result.containsErrors, /*contentHashCode=*/
- null,
+ result.containsErrors,
+ /*contentHashCode=*/null,
result.location,
- ImmutableList.copyOf(result.comments), /*imports=*/
- null);
+ ImmutableList.copyOf(result.comments),
+ /*imports=*/null);
}
/**
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FlowStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/FlowStatement.java
index b7c8d57..63ce6cb 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FlowStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FlowStatement.java
@@ -41,7 +41,7 @@
this.ex = new FlowException(kind);
}
- Kind getKind() {
+ public Kind getKind() {
return kind;
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
index 797782a..34d0c30 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ForStatement.java
@@ -30,8 +30,8 @@
/**
* Constructs a for loop statement.
*/
- ForStatement(Expression variable, Expression collection, List<Statement> block) {
- this.variable = new LValue(Preconditions.checkNotNull(variable));
+ public ForStatement(LValue variable, Expression collection, List<Statement> block) {
+ this.variable = Preconditions.checkNotNull(variable);
this.collection = Preconditions.checkNotNull(collection);
this.block = ImmutableList.copyOf(block);
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java
index 3951b2b..e6ef1ba 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FunctionDefStatement.java
@@ -32,9 +32,9 @@
FunctionSignature.WithValues<Expression, Expression> signature,
Iterable<Statement> statements) {
this.ident = ident;
+ this.parameters = ImmutableList.copyOf(parameters);
this.signature = signature;
this.statements = ImmutableList.copyOf(statements);
- this.parameters = ImmutableList.copyOf(parameters);
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FunctionSignature.java b/src/main/java/com/google/devtools/build/lib/syntax/FunctionSignature.java
index 2903f8e..401b4a3 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FunctionSignature.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FunctionSignature.java
@@ -223,31 +223,39 @@
/** The underlying signature with parameter shape and names */
public abstract FunctionSignature getSignature();
- /** The default values (if any) as a List of one per optional parameter.
- * We might have preferred ImmutableList, but we care about
- * supporting null's for some BuiltinFunction's, and we don't spit on speed.
+ /**
+ * The default values (if any) as an unmodifiable List of one per optional parameter. May
+ * contain nulls.
*/
@Nullable public abstract List<V> getDefaultValues();
- /** The parameter types (if specified) as a List of one per parameter, including * and **.
- * We might have preferred ImmutableList, but we care about supporting null's
- * so we can take shortcut for untyped values.
+ /**
+ * The parameter types (if specified) as an unmodifiable List of one per parameter, including *
+ * and **. May contain nulls.
*/
@Nullable public abstract List<T> getTypes();
- /**
- * Create a signature with (default and type) values.
- * If you supply mutable List's, we trust that you won't modify them afterwards.
- */
+ /** Create a signature with (default and type) values. */
public static <V, T> WithValues<V, T> create(FunctionSignature signature,
@Nullable List<V> defaultValues, @Nullable List<T> types) {
Shape shape = signature.getShape();
- Preconditions.checkArgument(defaultValues == null
- || defaultValues.size() == shape.getOptionals());
- Preconditions.checkArgument(types == null
- || types.size() == shape.getArguments());
- return new AutoValue_FunctionSignature_WithValues<>(signature, defaultValues, types);
+ List<V> convertedDefaultValues = null;
+ if (defaultValues != null) {
+ Preconditions.checkArgument(defaultValues.size() == shape.getOptionals());
+ List<V> copiedDefaultValues = new ArrayList<>();
+ copiedDefaultValues.addAll(defaultValues);
+ convertedDefaultValues = Collections.unmodifiableList(copiedDefaultValues);
+ }
+ List<T> convertedTypes = null;
+ if (types != null) {
+ Preconditions.checkArgument(types.size() == shape.getArguments());
+ List<T> copiedTypes = new ArrayList<>();
+ copiedTypes.addAll(types);
+ convertedTypes = Collections.unmodifiableList(copiedTypes);
+ }
+ return new AutoValue_FunctionSignature_WithValues<>(
+ signature, convertedDefaultValues, convertedTypes);
}
public static <V, T> WithValues<V, T> create(FunctionSignature signature,
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
index 4dce228..ada928b 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Identifier.java
@@ -26,7 +26,11 @@
// into array reference with a constant index. Variable lookups are currently a speed bottleneck,
// as previously measured in an experiment.
/**
- * Syntax node for an identifier.
+ * Syntax node for an identifier.
+ *
+ * Unlike most {@link ASTNode} subclasses, this one supports {@link Object#equals} and {@link
+ * Object#hashCode} (but note that these methods ignore location information). They are needed
+ * because {@code Identifier}s are stored in maps when constructing {@link LoadStatement}.
*/
public final class Identifier extends Expression {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java
index 697923f..b6adc60 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/IfStatement.java
@@ -25,7 +25,7 @@
/**
* Syntax node for an [el]if statement.
*/
- static final class ConditionalStatements extends Statement {
+ public static final class ConditionalStatements extends Statement {
private final Expression condition;
private final ImmutableList<Statement> stmts;
@@ -52,11 +52,11 @@
visitor.visit(this);
}
- Expression getCondition() {
+ public Expression getCondition() {
return condition;
}
- ImmutableList<Statement> getStmts() {
+ public ImmutableList<Statement> getStmts() {
return stmts;
}
@@ -74,7 +74,7 @@
* Constructs a if-elif-else statement. The else part is mandatory, but the list may be empty.
* ThenBlocks has to have at least one element.
*/
- IfStatement(List<ConditionalStatements> thenBlocks, List<Statement> elseBlock) {
+ public IfStatement(List<ConditionalStatements> thenBlocks, List<Statement> elseBlock) {
Preconditions.checkArgument(!thenBlocks.isEmpty());
this.thenBlocks = ImmutableList.copyOf(thenBlocks);
this.elseBlock = ImmutableList.copyOf(elseBlock);
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 b098ead..e13074c 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
@@ -23,7 +23,7 @@
*/
public final class LoadStatement extends Statement {
- private final ImmutableMap<Identifier, String> symbols;
+ private final ImmutableMap<Identifier, String> symbolMap;
private final ImmutableList<Identifier> cachedSymbols; // to save time
private final StringLiteral imp;
@@ -34,10 +34,14 @@
* 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.
*/
- LoadStatement(StringLiteral imp, Map<Identifier, String> symbols) {
+ public LoadStatement(StringLiteral imp, Map<Identifier, String> symbolMap) {
this.imp = imp;
- this.symbols = ImmutableMap.copyOf(symbols);
- this.cachedSymbols = ImmutableList.copyOf(symbols.keySet());
+ this.symbolMap = ImmutableMap.copyOf(symbolMap);
+ this.cachedSymbols = ImmutableList.copyOf(symbolMap.keySet());
+ }
+
+ public ImmutableMap<Identifier, String> getSymbolMap() {
+ return symbolMap;
}
public ImmutableList<Identifier> getSymbols() {
@@ -56,7 +60,7 @@
@Override
void doExec(Environment env) throws EvalException, InterruptedException {
- for (Map.Entry<Identifier, String> entry : symbols.entrySet()) {
+ for (Map.Entry<Identifier, String> entry : symbolMap.entrySet()) {
try {
Identifier name = entry.getKey();
Identifier declared = new Identifier(entry.getValue());
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 1cb224d..763b233 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
@@ -212,26 +212,17 @@
}
/**
- * Entry-point to parser that parses a build file with comments. All errors encountered during
- * parsing are reported via "reporter".
+ * Entry-point for parsing a file with comments.
+ *
+ * @param input the input to parse
+ * @param eventHandler a reporter for parsing errors
+ * @param parsingMode if set to {@link ParsingMode#BUILD}, restricts the parser to just the
+ * features present in the Build language
*/
- public static ParseResult parseFile(ParserInputSource input, EventHandler eventHandler) {
+ public static ParseResult parseFile(
+ ParserInputSource input, EventHandler eventHandler, ParsingMode parsingMode) {
Lexer lexer = new Lexer(input, eventHandler);
- Parser parser = new Parser(lexer, eventHandler, BUILD);
- List<Statement> statements = parser.parseFileInput();
- return new ParseResult(statements, parser.comments, locationFromStatements(lexer, statements),
- parser.errorsCount > 0 || lexer.containsErrors());
- }
-
- /**
- * Entry-point to parser that parses a build file with comments. All errors encountered during
- * parsing are reported via "reporter". Enable Skylark extensions that are not part of the core
- * BUILD language.
- */
- public static ParseResult parseFileForSkylark(
- ParserInputSource input, EventHandler eventHandler) {
- Lexer lexer = new Lexer(input, eventHandler);
- Parser parser = new Parser(lexer, eventHandler, SKYLARK);
+ Parser parser = new Parser(lexer, eventHandler, parsingMode);
List<Statement> statements = parser.parseFileInput();
return new ParseResult(
statements,
@@ -240,15 +231,30 @@
parser.errorsCount > 0 || lexer.containsErrors());
}
+ /** Convenience method for {@code parseFile} with the Build language. */
+ public static ParseResult parseFile(ParserInputSource input, EventHandler eventHandler) {
+ return parseFile(input, eventHandler, BUILD);
+ }
+
+ /** Convenience method for {@code parseFile} with Skylark. */
+ public static ParseResult parseFileForSkylark(
+ ParserInputSource input, EventHandler eventHandler) {
+ return parseFile(input, eventHandler, SKYLARK);
+ }
+
/**
- * Entry-point to parser that parses an expression. All errors encountered
- * during parsing are reported via "reporter". The expression may be followed
- * by newline tokens.
+ * Entry-point for parsing an expression. The expression may be followed by newline tokens.
+ *
+ * @param input the input to parse
+ * @param eventHandler a reporter for parsing errors
+ * @param parsingMode if set to {@link ParsingMode#BUILD}, restricts the parser to just the
+ * features present in the Build language
*/
@VisibleForTesting
- public static Expression parseExpression(ParserInputSource input, EventHandler eventHandler) {
+ public static Expression parseExpression(
+ ParserInputSource input, EventHandler eventHandler, ParsingMode parsingMode) {
Lexer lexer = new Lexer(input, eventHandler);
- Parser parser = new Parser(lexer, eventHandler, null);
+ Parser parser = new Parser(lexer, eventHandler, parsingMode);
Expression result = parser.parseExpression();
while (parser.token.kind == TokenKind.NEWLINE) {
parser.nextToken();
@@ -257,6 +263,19 @@
return result;
}
+ /** Convenience method for {@code parseExpression} with the Build language. */
+ @VisibleForTesting
+ public static Expression parseExpression(ParserInputSource input, EventHandler eventHandler) {
+ return parseExpression(input, eventHandler, BUILD);
+ }
+
+ /** Convenience method for {@code parseExpression} with Skylark. */
+ @VisibleForTesting
+ public static Expression parseExpressionForSkylark(
+ ParserInputSource input, EventHandler eventHandler) {
+ return parseExpression(input, eventHandler, SKYLARK);
+ }
+
private void reportError(Location location, String message) {
errorsCount++;
// Limit the number of reported errors to avoid spamming output.
@@ -1200,14 +1219,16 @@
nextToken();
Expression rvalue = parseExpression();
return setLocation(
- new AssignmentStatement(/*lvalue=*/ expression, /*expression=*/ rvalue), start, rvalue);
+ new AssignmentStatement(new LValue(expression), rvalue),
+ start, rvalue);
} else if (augmentedAssignmentMethods.containsKey(token.kind)) {
Operator operator = augmentedAssignmentMethods.get(token.kind);
nextToken();
Expression operand = parseExpression();
int end = operand.getLocation().getEndOffset();
return setLocation(
- new AugmentedAssignmentStatement(operator, expression, operand), start, end);
+ new AugmentedAssignmentStatement(operator, new LValue(expression), operand),
+ start, end);
} else {
return setLocation(new ExpressionStatement(expression), start, expression);
}
@@ -1254,7 +1275,7 @@
enterLoop();
try {
List<Statement> block = parseSuite();
- Statement stmt = new ForStatement(loopVar, collection, block);
+ Statement stmt = new ForStatement(new LValue(loopVar), collection, block);
list.add(setLocation(stmt, start, token.right));
} finally {
exitLoop();
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
index 0976982..45889a4 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
@@ -56,7 +56,7 @@
throw new ReturnException(returnExpression.getLocation(), returnExpression.eval(env));
}
- Expression getReturnExpression() {
+ public Expression getReturnExpression() {
return returnExpression;
}
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkImports.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkImports.java
index 9d28ee8..b2ddcc0 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkImports.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkImports.java
@@ -24,11 +24,7 @@
/**
* Factory class for creating appropriate instances of {@link SkylarkImports}.
*/
-public class SkylarkImports {
-
- private SkylarkImports() {
- throw new IllegalStateException("This class should not be instantiated");
- }
+public abstract class SkylarkImports {
// Default implementation class for SkylarkImport.
private abstract static class SkylarkImportImpl implements SkylarkImport {
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkImportTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkImportsTest.java
similarity index 97%
rename from src/test/java/com/google/devtools/build/lib/syntax/SkylarkImportTest.java
rename to src/test/java/com/google/devtools/build/lib/syntax/SkylarkImportsTest.java
index fa9dfbf..7aa1474 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkImportTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkImportsTest.java
@@ -30,7 +30,7 @@
* Tests {@link SkylarkImports}.
*/
@RunWith(JUnit4.class)
-public class SkylarkImportTest {
+public class SkylarkImportsTest {
@Rule
public ExpectedException thrown = ExpectedException.none();
@@ -39,17 +39,17 @@
SkylarkImport importForLabel = SkylarkImports.create(labelString);
assertThat(importForLabel.hasAbsolutePath()).named("hasAbsolutePath()").isFalse();
- assertThat(importForLabel.getImportString()).named("getIMportString()").isEqualTo(labelString);
+ assertThat(importForLabel.getImportString()).named("getImportString()").isEqualTo(labelString);
Label irrelevantContainingFile = Label.parseAbsoluteUnchecked("//another/path:BUILD");
assertThat(importForLabel.getLabel(irrelevantContainingFile)).named("getLabel()")
.isEqualTo(Label.parseAbsoluteUnchecked(expectedLabelString));
-
+
assertThat(importForLabel.asPathFragment()).named("asPathFragment()")
.isEqualTo(PathFragment.create(expectedPathString));
thrown.expect(IllegalStateException.class);
- importForLabel.getAbsolutePath();
+ importForLabel.getAbsolutePath();
}
@Test
@@ -147,7 +147,7 @@
Label containingLabel = Label.parseAbsolute(containingLabelString);
assertThat(importForPath.getLabel(containingLabel)).named("getLabel()")
.isEqualTo(Label.parseAbsolute(expectedLabelString));
-
+
assertThat(importForPath.asPathFragment()).named("asPathFragment()")
.isEqualTo(PathFragment.create(expectedPathString));
@@ -174,7 +174,7 @@
private void invalidImportTest(String importString, String expectedMsgPrefix) throws Exception {
thrown.expect(SkylarkImportSyntaxException.class);
thrown.expectMessage(startsWith(expectedMsgPrefix));
- SkylarkImports.create(importString);
+ SkylarkImports.create(importString);
}
@Test
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java b/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
index ce5e916..9ae284d 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/util/EvaluationTestCase.java
@@ -139,10 +139,17 @@
return env;
}
+ protected BuildFileAST parseBuildFileASTWithoutValidation(String... input) {
+ return BuildFileAST.parseSkylarkString(getEventHandler(), input);
+ }
+
+ protected BuildFileAST parseBuildFileAST(String... input) {
+ BuildFileAST ast = parseBuildFileASTWithoutValidation(input);
+ return ast.validate(env, getEventHandler());
+ }
+
protected List<Statement> parseFile(String... input) {
- BuildFileAST ast = BuildFileAST.parseSkylarkString(getEventHandler(), input);
- ast = ast.validate(env, getEventHandler());
- return ast.getStatements();
+ return parseBuildFileAST(input).getStatements();
}
/** Parses an Expression from string without a supporting file */
@@ -152,6 +159,13 @@
ParserInputSource.create(Joiner.on("\n").join(input), null), getEventHandler());
}
+ /** Same as {@link #parseExpression} but supports Skylark constructs. */
+ @VisibleForTesting
+ public Expression parseExpressionForSkylark(String... input) {
+ return Parser.parseExpressionForSkylark(
+ ParserInputSource.create(Joiner.on("\n").join(input), null), getEventHandler());
+ }
+
public EvaluationTestCase update(String varname, Object value) throws Exception {
env.update(varname, value);
return this;
