Skylark debugger: Fix NPE listing frames.

Environment$Continuation#caller is most definitely nullable in practice.

I suspect it's a bug in skylark itself, but for now, just properly mark
it nullable and handle it in the debugger.

Stack trace from NPE:

Caused by: java.lang.NullPointerException
	at com.google.devtools.build.lib.syntax.Environment.listFrames(Environment.java:1197)
	at com.google.devtools.build.lib.syntax.Environment.listFrames(Environment.java:81)
	at com.google.devtools.build.lib.skylarkdebug.server.ThreadHandler.listFrames(ThreadHandler.java:236)
	at com.google.devtools.build.lib.skylarkdebug.server.ThreadHandler.getThreadProto(ThreadHandler.java:345)
	at com.google.devtools.build.lib.skylarkdebug.server.ThreadHandler.pauseCurrentThread(ThreadHandler.java:289)
	at com.google.devtools.build.lib.skylarkdebug.server.ThreadHandler.pauseIfNecessary(ThreadHandler.java:203)
	at com.google.devtools.build.lib.skylarkdebug.server.SkylarkDebugServer.pauseIfNecessary(SkylarkDebugServer.java:158)
	at com.google.devtools.build.lib.skylarkdebug.server.SkylarkDebugServer$DebugAwareEval.exec(SkylarkDebugServer.java:262)
	at com.google.devtools.build.lib.syntax.UserDefinedFunction.call(UserDefinedFunction.java:91)
	at com.google.devtools.build.lib.syntax.BaseFunction.callWithArgArray(BaseFunction.java:462)
	at com.google.devtools.build.lib.syntax.BaseFunction.call(BaseFunction.java:440)
	at com.google.devtools.build.lib.analysis.skylark.SkylarkRuleConfiguredTargetUtil.lambda$buildRule$1(SkylarkRuleConfiguredTargetUtil.java:105)
	at com.google.devtools.build.lib.skylarkdebug.server.SkylarkDebugServer.runWithDebugging(SkylarkDebugServer.java:142)
	at com.google.devtools.build.lib.syntax.DebugServerUtils.runWithDebuggingIfEnabled(DebugServerUtils.java:70)
	at com.google.devtools.build.lib.analysis.skylark.SkylarkRuleConfiguredTargetUtil.buildRule(SkylarkRuleConfiguredTargetUtil.java:100)

PiperOrigin-RevId: 200229036

5 files changed, 23 insertions, 16 deletions

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 f81185f20a..3d74f951af 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
@@ -213,7 +213,8 @@ message Frame {
   // The scopes that contain value bindings accessible in this frame.
   repeated Scope scope = 2;
 
-  // The source location where the frame is currently paused.
+  // The source location where the frame is currently paused. May not be set in
+  // some situations.
   Location location = 3;
 }
 
diff --git a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebugEventHelper.java b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebugEventHelper.java
index 0254484062..a389569bc2 100644
--- a/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebugEventHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/skylarkdebug/server/DebugEventHelper.java
@@ -159,11 +159,14 @@ final class DebugEventHelper {
   }
 
   static SkylarkDebuggingProtos.Frame getFrameProto(DebugFrame frame) {
-    return SkylarkDebuggingProtos.Frame.newBuilder()
-        .setFunctionName(frame.functionName())
-        .setLocation(getLocationProto(frame.location()))
-        .addAllScope(getScopes(frame))
-        .build();
+    SkylarkDebuggingProtos.Frame.Builder builder =
+        SkylarkDebuggingProtos.Frame.newBuilder()
+            .setFunctionName(frame.functionName())
+            .addAllScope(getScopes(frame));
+    if (frame.location() != null) {
+      builder.setLocation(getLocationProto(frame.location()));
+    }
+    return builder.build();
   }
 
   private static ImmutableList<Scope> getScopes(DebugFrame frame) {
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinCallable.java b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinCallable.java
index 27bf364385..9a0a56b592 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinCallable.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinCallable.java
@@ -93,8 +93,7 @@ public class BuiltinCallable extends BaseFunction {
 
   @Override
   @Nullable
-  public Object call(Object[] args,
-      FuncallExpression ast, Environment env)
+  public Object call(Object[] args, @Nullable FuncallExpression ast, Environment env)
       throws EvalException, InterruptedException {
     Preconditions.checkNotNull(env);
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java
index 03d24bd0a6..469df1e3e7 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java
@@ -138,8 +138,7 @@ public class BuiltinFunction extends BaseFunction {
 
   @Override
   @Nullable
-  public Object call(Object[] args,
-      FuncallExpression ast, Environment env)
+  public Object call(Object[] args, @Nullable FuncallExpression ast, Environment env)
       throws EvalException, InterruptedException {
     Preconditions.checkNotNull(env);
 
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 6f84188b4b..5b22906d71 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
@@ -463,7 +463,7 @@ public final class Environment implements Freezable, Debuggable {
     final BaseFunction function;
 
     /** The {@link FuncallExpression} to which this Continuation will return. */
-    final FuncallExpression caller;
+    @Nullable final FuncallExpression caller;
 
     /** The next Continuation after this Continuation. */
     @Nullable final Continuation continuation;
@@ -478,12 +478,12 @@ public final class Environment implements Freezable, Debuggable {
     @Nullable final LinkedHashSet<String> knownGlobalVariables;
 
     Continuation(
-        Continuation continuation,
+        @Nullable Continuation continuation,
         BaseFunction function,
-        FuncallExpression caller,
+        @Nullable FuncallExpression caller,
         LexicalFrame lexicalFrame,
         GlobalFrame globalFrame,
-        LinkedHashSet<String> knownGlobalVariables) {
+        @Nullable LinkedHashSet<String> knownGlobalVariables) {
       this.continuation = continuation;
       this.function = function;
       this.caller = caller;
@@ -719,13 +719,17 @@ public final class Environment implements Freezable, Debuggable {
 
   /**
    * Enters a scope by saving state to a new Continuation
+   *
    * @param function the function whose scope to enter
    * @param lexical the lexical frame to use
    * @param caller the source AST node for the caller
    * @param globals the global Frame that this function closes over from its definition Environment
    */
   void enterScope(
-      BaseFunction function, LexicalFrame lexical, FuncallExpression caller, GlobalFrame globals) {
+      BaseFunction function,
+      LexicalFrame lexical,
+      @Nullable FuncallExpression caller,
+      GlobalFrame globals) {
     continuation =
         new Continuation(
             continuation, function, caller, lexicalFrame, globalFrame, knownGlobalVariables);
@@ -1194,7 +1198,8 @@ public final class Environment implements Freezable, Debuggable {
               .build());
 
       currentFrame = currentContinuation.lexicalFrame;
-      currentLocation = currentContinuation.caller.getLocation();
+      currentLocation =
+          currentContinuation.caller != null ? currentContinuation.caller.getLocation() : null;
       currentContinuation = currentContinuation.continuation;
     }