bazel syntax: break dependence on EventHandler
This change removes nearly all uses of EventHandler in the interpreter.
(The remaining uses are for events created by the Starlark 'print' statement.
A follow-up change will create a dedicated StarlarkThread.PrintHandler.)
All parse functions now report errors in one of two ways:
1) by throwing a SyntaxError exception containing a list of events.
This approach is used by "all-or-nothing" parse operations related
to expressions. Clients may catch the exception, extract the events,
and replay them on their handler should they choose. Example:
try {
Expression expr = Expression.parse(input)
...
} catch (SyntaxError ex) {
Event.replayEventsOn(handler, ex.errors());
}
2) by recording a list of scan/parse errors in StarlarkFile.errors.
The result of parsing a file is both a syntax tree (possibly incomplete),
and a list of errors. If parsing is followed by validation,
the validator appends its errors to this list. (Validation is now
capable of reporting multiple errors per file. ValidationException is gone.)
StarlarkFile file = StarlarkFile.parse(input)
if (!file.ok()) {
Event.replayEventsOn(handler, file.errors());
return;
}
Or:
StarlarkFile file = StarlarkFile.parse(input)
if (!file.ok()) {
throw new SyntaxError(file.errors());
}
Details:
- error helpers for Identifiers have moved from Eval to ValidationEnvironment
and no longer depend on EvalException.
- The EvalException.incompleteAST concept is irrelevant now that
clients do not proceed to evaluation if the parser or validation
step produced errors.
- StarlarkFile: exec + execTopLevelStatement have moved to EvalUtils, and will
change further.
- replayLexerEvents is gone from the API. The validator (which has access
to a StarlarkSemantics) can optionally copy the string escape events
into the main error bucket, or ignore them.
- StarlarkFile: eliminate parseWithoutImports
- Parser: delete dead code parseStatement{,s}
- Lexer: delete dead code stringAtLine
Numerous fixes were required in the tests, because they are such a muddle
of different phases (scan, parse, validate, evaluate)
and modes (BUILD vs .bzl vs core Starlark).
RELNOTES: Multiple Starlark validation errors are reported in a single pass.
PiperOrigin-RevId: 272205103
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
index 19c021a..5907fd1 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
@@ -46,8 +46,10 @@
import org.junit.runners.JUnit4;
/** Tests of Starlark evaluation. */
+// This test uses 'extends' to make a copy of EvaluationTest whose
+// mode is overridden to SKYLARK, changing various environmental parameters.
@RunWith(JUnit4.class)
-public class SkylarkEvaluationTest extends EvaluationTest {
+public final class SkylarkEvaluationTest extends EvaluationTest {
@Before
public final void setup() throws Exception {
@@ -974,15 +976,25 @@
flowStatementAfterLoop("continue");
}
+ // TODO(adonovan): move this and all tests that use it to Validation tests.
+ private void assertValidationError(String expectedError, final String... lines) throws Exception {
+ SyntaxError error = assertThrows(SyntaxError.class, () -> eval(lines));
+ assertThat(error).hasMessageThat().contains(expectedError);
+ }
+
private void flowStatementInsideFunction(String statement) throws Exception {
- checkEvalErrorContains(statement + " statement must be inside a for loop",
+ assertValidationError(
+ statement + " statement must be inside a for loop",
+ //
"def foo():",
" " + statement,
"x = foo()");
}
- private void flowStatementAfterLoop(String statement) throws Exception {
- checkEvalErrorContains(statement + " statement must be inside a for loop",
+ private void flowStatementAfterLoop(String statement) throws Exception {
+ assertValidationError(
+ statement + " statement must be inside a for loop",
+ //
"def foo2():",
" for i in range(0, 3):",
" pass",
@@ -991,6 +1003,11 @@
}
@Test
+ public void testStructMembersAreImmutable() throws Exception {
+ assertValidationError("cannot assign to 's.x'", "s = struct(x = 'a')", "s.x = 'b'\n");
+ }
+
+ @Test
public void testNoneAssignment() throws Exception {
new SkylarkTest()
.setUp("def foo(x=None):", " x = 1", " x = None", " return 2", "s = foo()")
@@ -1578,17 +1595,21 @@
@Test
public void testInvalidAugmentedAssignment_ListExpression() throws Exception {
- new SkylarkTest().testIfErrorContains(
+ assertValidationError(
"cannot perform augmented assignment on a list or tuple expression",
+ //
"def f(a, b):",
" [a, b] += []",
"f(1, 2)");
}
+
@Test
public void testInvalidAugmentedAssignment_NotAnLValue() throws Exception {
- newTest().testIfErrorContains(
- "cannot assign to 'x + 1'", "x + 1 += 2");
+ assertValidationError(
+ "cannot assign to 'x + 1'",
+ //
+ "x + 1 += 2");
}
@Test
@@ -1841,12 +1862,13 @@
}
@Test
+ // TODO(adonovan): move to Validation tests.
public void testTypo() throws Exception {
- new SkylarkTest()
- .testIfErrorContains(
- "name 'my_variable' is not defined (did you mean 'myVariable'?)",
- "myVariable = 2",
- "x = my_variable + 1");
+ assertValidationError(
+ "name 'my_variable' is not defined (did you mean 'myVariable'?)",
+ //
+ "myVariable = 2",
+ "x = my_variable + 1");
}
@Test
@@ -2223,6 +2245,7 @@
}
@Test
+ // TODO(adonovan): move to Validation tests.
public void testExperimentalFlagGuardedValue() throws Exception {
// This test uses an arbitrary experimental flag to verify this functionality. If this
// experimental flag were to go away, this test may be updated to use any experimental flag.
@@ -2234,6 +2257,7 @@
"GlobalSymbol is experimental and thus unavailable with the current "
+ "flags. It may be enabled by setting --experimental_build_setting_api";
+
new SkylarkTest(ImmutableMap.of("GlobalSymbol", val), "--experimental_build_setting_api=true")
.setUp("var = GlobalSymbol")
.testLookup("var", "foo");
