Handle evaluation of statements, not just expressions.
Also handle statements in conditional breakpoints.
This is more consistent with other common debuggers (e.g. java, python).
Calls Parser#parseStatement with local parsing level, so some statement types aren't handled (e.g. load statements), which is broadly consistent with other debuggers.
Assignment, augmented assignment, and return statements return a non-None value,
and simple expression statements still return the result of evaluating the expression.
TAG_CHANGE_OK=This proto has never yet been used
TYPE_CHANGE_OK=This proto has never yet been used
PiperOrigin-RevId: 202135678
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/proto/skylark_debugging.proto b/src/main/java/com/google/devtools/build/lib/skylarkdebug/proto/skylark_debugging.proto
index a1066c3..cc0de17 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkdebug/proto/skylark_debugging.proto
+++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/proto/skylark_debugging.proto
@@ -66,14 +66,14 @@
Stepping stepping = 2;
}
-// A request to evaluate a Skylark expression in a thread's current environment.
+// A request to evaluate a Skylark statement in a thread's current environment.
message EvaluateRequest {
// The identifier of the thread in whose execution context the expression
// should be evaluated.
int64 thread_id = 1;
- // The Skylark expression to evaluate.
- string expression = 2;
+ // The Skylark statement to evaluate.
+ string statement = 2;
}
// A request to list the stack frames of a thread.
@@ -141,7 +141,7 @@
// The response to an EvaluateRequest.
message EvaluateResponse {
- // The result of evaluating an expression.
+ // The result of evaluating a statement.
Value result = 1;
}
@@ -185,7 +185,8 @@
}
// An optional condition for the breakpoint. When present, the breakpoint will
// be triggered iff both the primary condition holds and this expression
- // evaluates to True.
+ // evaluates to True. It is unspecified how many times this expression will be
+ // evaluated, so it should be free of side-effects.
string expression = 2;
}
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServer.java b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServer.java
index 93e833d..433f4ef 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServer.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServer.java
@@ -201,7 +201,7 @@
long sequenceNumber, SkylarkDebuggingProtos.EvaluateRequest request)
throws DebugRequestException {
return DebugEventHelper.evaluateResponse(
- sequenceNumber, threadHandler.evaluate(request.getThreadId(), request.getExpression()));
+ sequenceNumber, threadHandler.evaluate(request.getThreadId(), request.getStatement()));
}
/** Handles a {@code ContinueExecutionRequest} and returns its response. */
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadHandler.java b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadHandler.java
index 585d581..6f92949 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadHandler.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/ThreadHandler.java
@@ -235,7 +235,7 @@
}
}
- SkylarkDebuggingProtos.Value evaluate(long threadId, String expression)
+ SkylarkDebuggingProtos.Value evaluate(long threadId, String statement)
throws DebugRequestException {
Debuggable debuggable;
synchronized (this) {
@@ -250,7 +250,7 @@
// accessed in response to a client request, and requests are handled serially
// TODO(bazel-team): support asynchronous replies, and use finer-grained locks
try {
- Object result = doEvaluate(debuggable, expression);
+ Object result = doEvaluate(debuggable, statement);
return DebuggerSerialization.getValueProto("Evaluation result", result);
} catch (EvalException | InterruptedException e) {
throw new DebugRequestException(e.getMessage());
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Debuggable.java b/src/main/java/com/google/devtools/build/lib/syntax/Debuggable.java
index 4540f55..ee871e6 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Debuggable.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Debuggable.java
@@ -22,8 +22,8 @@
/** A context in which debugging can occur. Implemented by Skylark environments. */
public interface Debuggable {
- /** Evaluates a Skylark expression in the adapter's environment. */
- Object evaluate(String expression) throws EvalException, InterruptedException;
+ /** Evaluates a Skylark statement in the adapter's environment. */
+ Object evaluate(String statement) throws EvalException, InterruptedException;
/**
* Returns the stack frames corresponding of the context's current (paused) state.
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
index 5b22906..d5b0d6c 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Environment.java
@@ -29,6 +29,7 @@
import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
import com.google.devtools.build.lib.syntax.Mutability.Freezable;
import com.google.devtools.build.lib.syntax.Mutability.MutabilityException;
+import com.google.devtools.build.lib.syntax.Parser.ParsingLevel;
import com.google.devtools.build.lib.util.Fingerprint;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.SpellChecker;
@@ -1169,15 +1170,31 @@
}
@Override
- public Object evaluate(String expression) throws EvalException, InterruptedException {
- ParserInputSource inputSource =
- ParserInputSource.create(expression, PathFragment.create("<debug eval>"));
+ public Object evaluate(String contents) throws EvalException, InterruptedException {
+ ParserInputSource input =
+ ParserInputSource.create(contents, PathFragment.create("<debug eval>"));
EvalEventHandler eventHandler = new EvalEventHandler();
- Expression expr = Parser.parseExpression(inputSource, eventHandler);
+ Statement statement = Parser.parseStatement(input, eventHandler, ParsingLevel.LOCAL_LEVEL);
if (!eventHandler.messages.isEmpty()) {
- throw new EvalException(expr.getLocation(), eventHandler.messages.get(0));
+ throw new EvalException(statement.getLocation(), eventHandler.messages.get(0));
}
- return expr.eval(this);
+ // TODO(bazel-team): move statement handling code to Eval
+ // deal with the most common case first
+ if (statement.kind() == Statement.Kind.EXPRESSION) {
+ return ((ExpressionStatement) statement).getExpression().doEval(this);
+ }
+ // all other statement types are executed directly
+ Eval.fromEnvironment(this).exec(statement);
+ switch (statement.kind()) {
+ case ASSIGNMENT:
+ case AUGMENTED_ASSIGNMENT:
+ return ((AssignmentStatement) statement).getLValue().getExpression().doEval(this);
+ case RETURN:
+ Expression expr = ((ReturnStatement) statement).getReturnExpression();
+ return expr != null ? expr.doEval(this) : Runtime.NONE;
+ default:
+ return Runtime.NONE;
+ }
}
@Override
diff --git a/src/test/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServerTest.java b/src/test/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServerTest.java
index 064f666..32e5698 100644
--- a/src/test/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylarkdebug/server/SkylarkDebugServerTest.java
@@ -41,6 +41,7 @@
import com.google.devtools.build.lib.syntax.Environment;
import com.google.devtools.build.lib.syntax.Mutability;
import com.google.devtools.build.lib.syntax.ParserInputSource;
+import com.google.devtools.build.lib.syntax.Runtime;
import com.google.devtools.build.lib.syntax.SkylarkList;
import com.google.devtools.build.lib.testutil.Scratch;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
@@ -378,7 +379,35 @@
}
@Test
- public void testEvaluateRequest() throws Exception {
+ public void testEvaluateRequestWithExpression() throws Exception {
+ sendStartDebuggingRequest();
+ BuildFileAST buildFile = parseBuildFile("/a/build/file/BUILD", "x = [1,2,3]", "y = [2,3,4]");
+ Environment env = newEnvironment();
+
+ Location breakpoint =
+ Location.newBuilder().setLineNumber(2).setPath("/a/build/file/BUILD").build();
+ setBreakpoints(ImmutableList.of(breakpoint));
+
+ Thread evaluationThread = execInWorkerThread(buildFile, env);
+ long threadId = evaluationThread.getId();
+
+ // wait for breakpoint to be hit
+ client.waitForEvent(DebugEvent::hasThreadPaused, Duration.ofSeconds(5));
+
+ DebugEvent response =
+ client.sendRequestAndWaitForResponse(
+ DebugRequest.newBuilder()
+ .setSequenceNumber(123)
+ .setEvaluate(
+ EvaluateRequest.newBuilder().setThreadId(threadId).setStatement("x[1]").build())
+ .build());
+ assertThat(response.hasEvaluate()).isTrue();
+ assertThat(response.getEvaluate().getResult())
+ .isEqualTo(DebuggerSerialization.getValueProto("Evaluation result", 2));
+ }
+
+ @Test
+ public void testEvaluateRequestWithAssignmentStatement() throws Exception {
sendStartDebuggingRequest();
BuildFileAST buildFile = parseBuildFile("/a/build/file/BUILD", "x = [1,2,3]", "y = [2,3,4]");
Environment env = newEnvironment();
@@ -400,12 +429,55 @@
.setEvaluate(
EvaluateRequest.newBuilder()
.setThreadId(threadId)
- .setExpression("x[1]")
+ .setStatement("x = [5,6]")
.build())
.build());
- assertThat(response.hasEvaluate()).isTrue();
assertThat(response.getEvaluate().getResult())
- .isEqualTo(DebuggerSerialization.getValueProto("Evaluation result", 2));
+ .isEqualTo(
+ DebuggerSerialization.getValueProto(
+ "Evaluation result", SkylarkList.createImmutable(ImmutableList.of(5, 6))));
+
+ ListFramesResponse frames = listFrames(threadId);
+ assertThat(frames.getFrame(0).getScope(0).getBindingList())
+ .contains(
+ DebuggerSerialization.getValueProto(
+ "x", SkylarkList.createImmutable(ImmutableList.of(5, 6))));
+ }
+
+ @Test
+ public void testEvaluateRequestWithExpressionStatementMutatingState() throws Exception {
+ sendStartDebuggingRequest();
+ BuildFileAST buildFile = parseBuildFile("/a/build/file/BUILD", "x = [1,2,3]", "y = [2,3,4]");
+ Environment env = newEnvironment();
+
+ Location breakpoint =
+ Location.newBuilder().setLineNumber(2).setPath("/a/build/file/BUILD").build();
+ setBreakpoints(ImmutableList.of(breakpoint));
+
+ Thread evaluationThread = execInWorkerThread(buildFile, env);
+ long threadId = evaluationThread.getId();
+
+ // wait for breakpoint to be hit
+ client.waitForEvent(DebugEvent::hasThreadPaused, Duration.ofSeconds(5));
+
+ DebugEvent response =
+ client.sendRequestAndWaitForResponse(
+ DebugRequest.newBuilder()
+ .setSequenceNumber(123)
+ .setEvaluate(
+ EvaluateRequest.newBuilder()
+ .setThreadId(threadId)
+ .setStatement("x.append(4)")
+ .build())
+ .build());
+ assertThat(response.getEvaluate().getResult())
+ .isEqualTo(DebuggerSerialization.getValueProto("Evaluation result", Runtime.NONE));
+
+ ListFramesResponse frames = listFrames(threadId);
+ assertThat(frames.getFrame(0).getScope(0).getBindingList())
+ .contains(
+ DebuggerSerialization.getValueProto(
+ "x", SkylarkList.createImmutable(ImmutableList.of(1, 2, 3, 4))));
}
@Test
@@ -429,10 +501,7 @@
DebugRequest.newBuilder()
.setSequenceNumber(123)
.setEvaluate(
- EvaluateRequest.newBuilder()
- .setThreadId(threadId)
- .setExpression("z[0]")
- .build())
+ EvaluateRequest.newBuilder().setThreadId(threadId).setStatement("z[0]").build())
.build());
assertThat(response.hasError()).isTrue();
assertThat(response.getError().getMessage()).isEqualTo("name 'z' is not defined");
