Simplify BuildFileAST and move BUILD file validation checks.
After this change, it will be easy to access the Environment from the Validation checks. This is needed to access StarlarkSemantics, and it is required for computing the scope of variables.
RELNOTES: None.
PiperOrigin-RevId: 243256778
diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
index c1127b4..62cd394 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java
@@ -72,6 +72,7 @@
import com.google.devtools.build.lib.syntax.SyntaxTreeVisitor;
import com.google.devtools.build.lib.syntax.Type;
import com.google.devtools.build.lib.syntax.Type.ConversionException;
+import com.google.devtools.build.lib.syntax.ValidationEnvironment;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.Path;
@@ -1769,6 +1770,10 @@
}
}
+ if (!ValidationEnvironment.checkBuildSyntax(buildFileAST.getStatements(), eventHandler)) {
+ pkgBuilder.setContainsErrors();
+ }
+
// TODO(bazel-team): (2009) the invariant "if errors are reported, mark the package
// as containing errors" is strewn all over this class. Refactor to use an
// event sensor--and see if we can simplify the calling code in
diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
index bc72aaf..adb4983 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceFactory.java
@@ -45,6 +45,7 @@
import com.google.devtools.build.lib.syntax.SkylarkUtils;
import com.google.devtools.build.lib.syntax.SkylarkUtils.Phase;
import com.google.devtools.build.lib.syntax.StarlarkSemantics;
+import com.google.devtools.build.lib.syntax.ValidationEnvironment;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.RootedPath;
@@ -204,7 +205,9 @@
throw new IllegalStateException(e);
}
}
- if (!ast.exec(workspaceEnv, localReporter)) {
+
+ if (!ValidationEnvironment.checkBuildSyntax(ast.getStatements(), localReporter)
+ || !ast.exec(workspaceEnv, localReporter)) {
localReporter.handle(Event.error("Error evaluating WORKSPACE file"));
}
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 0da3c30..5f28614 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
@@ -295,8 +295,7 @@
EventHandler eventHandler) {
Parser.ParseResult result = Parser.parseFile(input, eventHandler);
return create(
- preludeStatements, result, /* contentHashCode= */ null, repositoryMapping, eventHandler)
- .validateBuildFile(eventHandler);
+ preludeStatements, result, /* contentHashCode= */ null, repositoryMapping, eventHandler);
}
/**
@@ -311,24 +310,22 @@
EventHandler eventHandler) {
Parser.ParseResult result = Parser.parseFile(input, eventHandler);
return create(
- preludeStatements,
- result,
- /* contentHashCode= */ null,
- repositoryMapping,
- eventHandler,
- true)
- .validateBuildFile(eventHandler);
+ preludeStatements,
+ result,
+ /* contentHashCode= */ null,
+ repositoryMapping,
+ eventHandler,
+ true);
}
public static BuildFileAST parseBuildFile(ParserInputSource input, EventHandler eventHandler) {
Parser.ParseResult result = Parser.parseFile(input, eventHandler);
return create(
- /* preludeStatements= */ ImmutableList.<Statement>of(),
- result,
- /* contentHashCode= */ null,
- /* repositoryMapping= */ ImmutableMap.of(),
- eventHandler)
- .validateBuildFile(eventHandler);
+ /* preludeStatements= */ ImmutableList.<Statement>of(),
+ result,
+ /* contentHashCode= */ null,
+ /* repositoryMapping= */ ImmutableMap.of(),
+ eventHandler);
}
public static BuildFileAST parseSkylarkFile(
@@ -389,19 +386,6 @@
return new BuildFileAST(statements, true, contentHashCode, getLocation(), comments, imports);
}
- /**
- * Run static checks for a BUILD file.
- *
- * @return a new AST (or the same), with the containsErrors flag updated.
- */
- public BuildFileAST validateBuildFile(EventHandler eventHandler) {
- boolean valid = ValidationEnvironment.checkBuildSyntax(statements, eventHandler);
- if (valid || containsErrors) {
- return this;
- }
- return new BuildFileAST(statements, true, contentHashCode, getLocation(), comments, imports);
- }
-
public static BuildFileAST parseString(EventHandler eventHandler, String... content) {
String str = Joiner.on("\n").join(content);
ParserInputSource input = ParserInputSource.create(str, PathFragment.EMPTY_FRAGMENT);
@@ -414,10 +398,6 @@
eventHandler);
}
- public static BuildFileAST parseBuildString(EventHandler eventHandler, String... content) {
- return parseString(eventHandler, content).validateBuildFile(eventHandler);
- }
-
/**
* Parse the specified build file, without building the AST.
*
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
index 6ee8f87..df0847c 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
@@ -678,4 +678,34 @@
+ "replace(old, new, maxsplit = None) of 'string'",
"'banana'.replace('a', 'o', 3, old='a')");
}
+
+ @Test
+ public void testDefInBuild() throws Exception {
+ new BuildTest()
+ .testIfErrorContains(
+ "function definitions are not allowed in BUILD files", "def func(): pass");
+ }
+
+ @Test
+ public void testForStatementForbiddenInBuild() throws Exception {
+ new BuildTest().testIfErrorContains("for loops are not allowed", "for _ in []: pass");
+ }
+
+ @Test
+ public void testIfStatementForbiddenInBuild() throws Exception {
+ new BuildTest()
+ .testIfErrorContains("if statements are not allowed in BUILD files", "if False: pass");
+ }
+
+ @Test
+ public void testKwargsForbiddenInBuild() throws Exception {
+ new BuildTest()
+ .testIfErrorContains("**kwargs arguments are not allowed in BUILD files", "func(**dict)");
+ }
+
+ @Test
+ public void testArgsForbiddenInBuild() throws Exception {
+ new BuildTest()
+ .testIfErrorContains("*args arguments are not allowed in BUILD files", "func(*array)");
+ }
}
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 d2c6e44..fdb855e 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
@@ -40,7 +40,7 @@
public class ParserTest extends EvaluationTestCase {
private BuildFileAST parseFileWithComments(String... input) {
- return BuildFileAST.parseBuildString(getEventHandler(), input);
+ return BuildFileAST.parseString(getEventHandler(), input);
}
/** Parses build code (not Skylark) */
@@ -1452,41 +1452,6 @@
}
@Test
- public void testDefInBuild() throws Exception {
- setFailFast(false);
- parseFile("def func(): pass");
- assertContainsError("function definitions are not allowed in BUILD files");
- }
-
- @Test
- public void testForStatementForbiddenInBuild() throws Exception {
- setFailFast(false);
- parseFile("for _ in []: pass");
- assertContainsError("for statements are not allowed in BUILD files");
- }
-
- @Test
- public void testIfStatementForbiddenInBuild() throws Exception {
- setFailFast(false);
- parseFile("if False: pass");
- assertContainsError("if statements are not allowed in BUILD files");
- }
-
- @Test
- public void testKwargsForbiddenInBuild() throws Exception {
- setFailFast(false);
- parseFile("func(**dict)");
- assertContainsError("**kwargs arguments are not allowed in BUILD files");
- }
-
- @Test
- public void testArgsForbiddenInBuild() throws Exception {
- setFailFast(false);
- parseFile("func(*array)");
- assertContainsError("*args arguments are not allowed in BUILD files");
- }
-
- @Test
public void testArgumentAfterKwargs() throws Exception {
setFailFast(false);
parseFileForSkylark(
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 be52ea8..88bc26e 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
@@ -38,6 +38,7 @@
import com.google.devtools.build.lib.syntax.SkylarkUtils;
import com.google.devtools.build.lib.syntax.SkylarkUtils.Phase;
import com.google.devtools.build.lib.syntax.Statement;
+import com.google.devtools.build.lib.syntax.ValidationEnvironment;
import com.google.devtools.build.lib.testutil.TestConstants;
import com.google.devtools.build.lib.testutil.TestMode;
import java.util.LinkedList;
@@ -199,7 +200,9 @@
if (testMode == TestMode.SKYLARK) {
return BuildFileAST.eval(env, input);
}
- return BuildFileAST.parseBuildString(env.getEventHandler(), input).eval(env);
+ BuildFileAST ast = BuildFileAST.parseString(env.getEventHandler(), input);
+ ValidationEnvironment.checkBuildSyntax(ast.getStatements(), env.getEventHandler());
+ return ast.eval(env);
}
public void checkEvalError(String msg, String... input) throws Exception {
