Fix Starlark debugger crash on duplicate name in locals

src/main/java/net/starlark/java/eval/Debug.java

@@ -19,6 +19,7 @@
 import java.util.concurrent.atomic.AtomicReference;
 import java.util.function.Predicate;
 import javax.annotation.Nullable;
+import net.starlark.java.annot.StarlarkBuiltin;
 import net.starlark.java.syntax.Location;
 
 /** Debugger API. */
@@ -37,6 +38,29 @@ public interface Debugger {
     void close();
   }
 
+  /** A distinguished value that carries a message. */
+  @StarlarkBuiltin(
+      name = "debugger_message",
+      doc = "A distinguished value that carries a message.",
+      documented = false)
+  public static final class DebuggerMessage implements StarlarkValue {
+    private final String message;
+
+    public DebuggerMessage(String message) {
+      this.message = message;
+    }
+
+    @Override
+    public String toString() {
+      return "<" + message + ">";
+    }
+
+    @Override
+    public void repr(Printer printer) {
+      printer.append("<").append(message).append(">");
+    }
+  }
+
   /** A Starlark value that can expose additional information to a debugger. */
   public interface ValueWithDebugAttributes extends StarlarkValue {
     /**

src/main/java/net/starlark/java/eval/StarlarkThread.java

@@ -19,7 +19,9 @@
 import com.google.common.collect.ImmutableMap;
 import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.LinkedHashMap;
 import java.util.Map;
+import java.util.Objects;
 import java.util.concurrent.atomic.AtomicInteger;
 import javax.annotation.Nullable;
 import javax.annotation.concurrent.Immutable;
@@ -182,20 +184,33 @@ public Location getLocation() {
 
     @Override
     public ImmutableMap<String, Object> getLocals() {
+      // TODO: List comprehensions introduce new locals that can shadow outer locals. Find a way to
+      //  accurately represent them and their values in a situation such as:
+      //
+      //  def foo(x):
+      //    x += [[j(x) for x in i(x)] + h(x) for x in f(x) if g(x)]
+      //    return k(x)
+      //
       // TODO(adonovan): provide a more efficient API.
-      ImmutableMap.Builder<String, Object> env = ImmutableMap.builder();
+      LinkedHashMap<String, Object> env = new LinkedHashMap<>();
       if (fn instanceof StarlarkFunction) {
         for (int i = 0; i < locals.length; i++) {
           Object local = locals[i];
           if (local instanceof StarlarkFunction.Cell) {
             local = ((StarlarkFunction.Cell) local).x;
           }
           if (local != null) {
-            env.put(((StarlarkFunction) fn).rfn.getLocals().get(i).getName(), local);
+            env.merge(
+                ((StarlarkFunction) fn).rfn.getLocals().get(i).getName(),
+                local,
+                (oldValue, newValue) ->
+                    Objects.equals(oldValue, newValue)
+                        ? oldValue
+                        : new Debug.DebuggerMessage("different values in scope"));
           }
         }
       }
-      return env.buildOrThrow();
+      return ImmutableMap.copyOf(env);
     }
 
     @Override

src/test/java/net/starlark/java/eval/StarlarkThreadDebuggingTest.java

@@ -250,4 +250,73 @@ public void testEvaluateExpressionOnVariableInScope() throws Exception {
     assertThat(Starlark.execFile(ParserInput.fromLines("a"), FileOptions.DEFAULT, module, thread))
         .isEqualTo(StarlarkInt.of(1));
   }
+
+  @Test
+  public void testListComprehensionVariable() throws Exception {
+    // f is a built-in that captures the stack using the Debugger API.
+    Object[] result = {null, null};
+    StarlarkCallable f =
+        new StarlarkCallable() {
+          @Override
+          public String getName() {
+            return "f";
+          }
+
+          @Override
+          public Object fastcall(StarlarkThread thread, Object[] positional, Object[] named) {
+            result[0] = Debug.getCallStack(thread);
+            result[1] = thread.getCallStack();
+            return Starlark.NONE;
+          }
+
+          @Override
+          public Location getLocation() {
+            return Location.fromFileLineColumn("builtin", 12, 0);
+          }
+
+          @Override
+          public String toString() {
+            return "<debug function>";
+          }
+        };
+
+    // Set up global environment.
+    Module module =
+        Module.withPredeclared(StarlarkSemantics.DEFAULT, ImmutableMap.of("a", 1, "b", 2, "f", f));
+
+    // Execute a small file that calls f.
+    // shadows global a
+    ParserInput input =
+        ParserInput.fromString(
+            """
+                def g(a, y, z):
+                  a = [x for x in range(3)]
+                  a = [x for x in range(6)]
+                  f()
+                g(4, 5, 6)""",
+            "main.star");
+    Starlark.execFile(input, FileOptions.DEFAULT, module, newThread());
+
+    @SuppressWarnings("unchecked")
+    ImmutableList<Debug.Frame> stack = (ImmutableList<Debug.Frame>) result[0];
+
+    // Check the stack captured by f.
+    // We compare printed string forms, as it gives more informative assertion failures.
+    StringBuilder buf = new StringBuilder();
+    for (Debug.Frame fr : stack) {
+      buf.append(
+          String.format(
+              "%s @ %s local=%s\n", fr.getFunction().getName(), fr.getLocation(), fr.getLocals()));
+    }
+    // location is paren of g(4, 5, 6) call:
+    // location is paren of "f()" call:
+    // location is "current PC" in f.
+    assertThat(buf.toString())
+        .isEqualTo(
+            """
+                <toplevel> @ main.star:5:2 local={}
+                g @ main.star:4:4 local={a=[0, 1, 2, 3, 4, 5], y=5, z=6, x=<different values in scope>}
+                f @ builtin:12 local={}
+                """);
+  }
 }