Fix Starlark debugger crash on duplicate name in locals #24919
Conversation
fmeum
requested review from
hvadehra
and removed request for
tetromino and
brandjon
github-actions
bot
added
the
awaiting-review
There was a problem hiding this comment.
This is the obvious but wrong solution.
Consider
def foo(x):
x += [[j(x) for x in i(x)] + h(x) for x in f(x) if g(x)]
return k(x)Now consider which value of which x should the debugger see
- when f(x) is called
- when g(x) is called
- when h(x) is called
- when i(x) is called
- when j(x) is called
- when k(x) is called
We saw the same bug reported internally and I have been scratching my head at it for a few days - it's very possible that to fix, we would need to revise how we represent the frame in debugger protos.
iancha1992
added
area-java-Starlark_API
|
Thanks for the instructive example, I didn't grasp the full complexity of this issue.
I would argue that the current state (the debugger crashes) is pretty bad. What do you think of handling conflicting locals by communicating a dedicated
I checked what PyCharm's debugger is doing in this case. It emits additional |
A new frame is probably the right way to present this. Unfortunately, while this is easy in cpython (where I believe the intepreter physically pushes new frames for comprehensions), it's not easy in starlark (to save memory, the starlark resolver puts comprehension's variables into the enclosing function's frame). |
|
I updated the PR to reflect what I proposed as an incremental improvement above. |
|
Filed #24931 for the long-term fix |
|
I think I see a better way of solving the crash: emit only the comprehension variable currently visible and hide the shadowed ones. Let me check if that's feasible. |
|
I think my proposed fix will work: https://bazel-review.googlesource.com/c/bazel/+/269374 |
The Starlark interpreter, for optimization reasons, stores the bindings of comprehension variables in their enclosing function's locals array. This means we cannot naively transform the locals array into a name -> value map for debugger output: there may be multiple initialized variables having the same name. And since we have been building this map using buildOrThrow(), attempting to debug a function with a comprehension variable having the same name as a parameter or other local variable would crash Bazel. The immediate fix would be to track comprehension bindings' lexical scope (by saving a pointer to the comprehension AST node; note that a comprehension's scope has a non-trivial shape - it has a "hole" punched out for the first `for` clause), so we can ignore comprehension variables outside their comprehension, and emit only the innermost one when inside a comprehension (relying on the fact that the resolver places inner comprehensions' variable bindings after the outer ones). However, while this fixes the crash and the most obvious correctness problem, it does not allow the user of the debugger to examine the value of the shadowed variables. The proper fix - to be implemented later - would be to push a new debugger frame when debugging inside a comprehension. See #24931 and discussion with @fmeum in #24919 PiperOrigin-RevId: 715908968 Change-Id: I3066e89f2e92d0fd0e483851a767b7c7d70ab555
No description provided.