From 3954e54c2435fbd7841cee93c8c7f3b7d3bdbdb7 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Tue, 14 Jan 2025 14:49:48 +0100 Subject: [PATCH 1/2] Fix Starlark debugger crash on duplicate name in locals --- .../starlark/java/eval/StarlarkThread.java | 2 +- .../eval/StarlarkThreadDebuggingTest.java | 69 +++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/src/main/java/net/starlark/java/eval/StarlarkThread.java b/src/main/java/net/starlark/java/eval/StarlarkThread.java index b073a8bd672fdf..66bfe737b8d0f3 100644 --- a/src/main/java/net/starlark/java/eval/StarlarkThread.java +++ b/src/main/java/net/starlark/java/eval/StarlarkThread.java @@ -195,7 +195,7 @@ public ImmutableMap getLocals() { } } } - return env.buildOrThrow(); + return env.buildKeepingLast(); } @Override diff --git a/src/test/java/net/starlark/java/eval/StarlarkThreadDebuggingTest.java b/src/test/java/net/starlark/java/eval/StarlarkThreadDebuggingTest.java index 7668f279abd0e5..a2cf9f0598cbdf 100644 --- a/src/test/java/net/starlark/java/eval/StarlarkThreadDebuggingTest.java +++ b/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 ""; + } + }; + + // 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 stack = (ImmutableList) 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( + """ + @ main.star:5:2 local={} + g @ main.star:4:4 local={a=[0, 1, 2, 3, 4, 5], y=5, z=6, x=5} + f @ builtin:12 local={} + """); + } } From ccb92bd07e4adf88dd49b60d3bd71688f37cca51 Mon Sep 17 00:00:00 2001 From: Fabian Meumertzheim Date: Wed, 15 Jan 2025 10:08:51 +0100 Subject: [PATCH 2/2] Show a conflict warning --- .../java/net/starlark/java/eval/Debug.java | 24 +++++++++++++++++++ .../starlark/java/eval/StarlarkThread.java | 21 +++++++++++++--- .../eval/StarlarkThreadDebuggingTest.java | 2 +- 3 files changed, 43 insertions(+), 4 deletions(-) diff --git a/src/main/java/net/starlark/java/eval/Debug.java b/src/main/java/net/starlark/java/eval/Debug.java index e820b1469a6361..4b05a4349b04fb 100644 --- a/src/main/java/net/starlark/java/eval/Debug.java +++ b/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 { /** diff --git a/src/main/java/net/starlark/java/eval/StarlarkThread.java b/src/main/java/net/starlark/java/eval/StarlarkThread.java index 66bfe737b8d0f3..f2083bf56d596d 100644 --- a/src/main/java/net/starlark/java/eval/StarlarkThread.java +++ b/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,8 +184,15 @@ public Location getLocation() { @Override public ImmutableMap 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 env = ImmutableMap.builder(); + LinkedHashMap env = new LinkedHashMap<>(); if (fn instanceof StarlarkFunction) { for (int i = 0; i < locals.length; i++) { Object local = locals[i]; @@ -191,11 +200,17 @@ public ImmutableMap getLocals() { 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.buildKeepingLast(); + return ImmutableMap.copyOf(env); } @Override diff --git a/src/test/java/net/starlark/java/eval/StarlarkThreadDebuggingTest.java b/src/test/java/net/starlark/java/eval/StarlarkThreadDebuggingTest.java index a2cf9f0598cbdf..025ec044b130a3 100644 --- a/src/test/java/net/starlark/java/eval/StarlarkThreadDebuggingTest.java +++ b/src/test/java/net/starlark/java/eval/StarlarkThreadDebuggingTest.java @@ -315,7 +315,7 @@ def g(a, y, z): .isEqualTo( """ @ main.star:5:2 local={} - g @ main.star:4:4 local={a=[0, 1, 2, 3, 4, 5], y=5, z=6, x=5} + g @ main.star:4:4 local={a=[0, 1, 2, 3, 4, 5], y=5, z=6, x=} f @ builtin:12 local={} """); }