GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-83151: Make closure work on pdb by gaogaotiantian · Pull Request #111094 · python/cpython

Conversation

@gaogaotiantian

@gaogaotiantian gaogaotiantian commented Oct 20, 2023 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Closure on pdb has been an issue for years, and is often considered "to difficult to fix". PEP709 alleviated the issue by inlining all list, dict and set comprehensions, but the fundamental issue is still there - you can easily reproduce the issue with lambda functions ((lambda: x)()) or generators (any(x for x in lst if x != z)). Now that pdb supports multi-line statement, you can even create your own function scope. Neither of them work in current pdb.

The related (open) issues I can find are #83151, #80052, #76987, #70260 and #65360.

This PR fixes all the issues mentioned above, while keep (almost?) all the current correct behaviors.

Boiler alert, black magic involved.

The fundamental reason for the issue above, is that when python compiles code that generates nested functions, the inner function has no idea about the context. For example:

def f():
    z = 1
    any([x for x in range(2) if x != z])

The code above works fine because python compiles any([x for x in range(2) if x != z]) with the awareness that z is a local variable of f, so closure should be used. However, in pdb, if you execute/compile any([x for x in range(2) if x != z]), there's no way for Python to know that z is an outer local variable and should be passed as a cell variable.

So, the solution is - trick the compiler.

What we can do, is to force compiler to compile the code we need in a closure-awareness environment. Take the code above as an example, instead of compiling any([x for x in range(2) if x != z]), we compile the following code:

def outer():
    z = None
    def __pdb_scope():
        nonlocal z
        any([x for x in range(2) if x != z])

Then we get the code object of outer.__pdb_scope, and that is the code object we actually want - it considers z as a free variable. Then, we pass the value of z with closure argument of exec:

exec(code, globals, locals, closure=(types.CellType(1),))

The solution is not done, as we still need to write the local variables back (locals won't be changed as all the variables are considered freevar). Also, it's possible for the code itself to create new variables and we want that in locals as well (k=1 for example). So, we just write all the local variables in the scope back by adding some new code at the end of the code.

To keep the original behavior as close as possible, if any exception is raised in the new method, we fallback to the original simple exec solution (it might be a valid exception, but we will always have the same exception as before).

Why not ...

  • just combine globals() and locals() as the global namespace?
    • result of globals() would be wrong
    • result of global x; print(x) would be wrong if x appears in both global and local namespace (if local is preferred, same issue if global is preferred).
    • impossible to do writeback correctly
  • use the new method only?
    • the new method will give different exception on some cases (UnboundLocalError vs NameError)
    • the new method does not work well with global x; print(x) as x would be defined as both global and nonlocal (it will fallback to the original method and work)

This is not the perfect solution, but it's pretty close. It passed all the existing tests and fixed all the cases in the issues mentioned above. I have not came up with an example that does not work with this solution (I would guess there will be some dark corners).

Yes, I know this is complicated, and it kind of depends on the implementation of CPython, but I still believe we should do this, because this issue has been repeatedly raised by users of pdb for probably > 10 years.

  • Issue: [PDB] NameError in list comprehension in PDB #83151

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
gaogaotiantian added 2 commits October 19, 2023 18:53
@gaogaotiantian
Make closure work on pdb
720a584
@gaogaotiantian
Always update local variables
8232457
@gaogaotiantian
gaogaotiantian requested review from brandtbucher, carljm, gvanrossum and iritkatriel October 20, 2023 03:49
@bedevere-app bedevere-app Bot mentioned this pull request Oct 20, 2023
[PDB] NameError in list comprehension in PDB #83151
Closed
@bedevere-app bedevere-app Bot added the awaiting review label Oct 20, 2023
@blurb-it
📜🤖 Added by blurb_it.
8df6d2b
@gvanrossum

gvanrossum commented Oct 20, 2023

Copy link
Copy Markdown
Member

Great work! I'm not sure I have the time (or even, at this point, the context) to review this. Maybe someone else can do this more justice?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gaogaotiantian

gaogaotiantian commented Oct 20, 2023

Copy link
Copy Markdown
Member Author

Great work! I'm not sure I have the time (or even, at this point, the context) to review this. Maybe someone else can do this more justice?

Oh I invited you because you reopened #65360 (comment) and this fix involves some black magic of Python. But no worries I'm sure I can find great reviewers for feedbacks of the PR (a couple invited already)!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

brandtbucher
brandtbucher reviewed May 4, 2024
View reviewed changes

@brandtbucher brandtbucher left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Been playing around with this locally for a while, and it seems like it works really well! Nice job.

Here's a first round of review. This is a tricky task, but I think the current solution may be overcomplicating it in places:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@brandtbucher brandtbucher self-assigned this May 4, 2024
brandtbucher
brandtbucher reviewed May 4, 2024
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2023-10-20-03-50-17.gh-issue-83151.bcsD40.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

brandtbucher
brandtbucher reviewed May 4, 2024
View reviewed changes
Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@gaogaotiantian

gaogaotiantian commented May 4, 2024

Copy link
Copy Markdown
Member Author

I just realized this might have some conflict with PEP 667 which will probably be merged today. I will rework the code. Also taking the notes from your reviews. I will also hopefully finish this tomorrow.

brandtbucher reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gaogaotiantian added 2 commits May 4, 2024 15:23
@gaogaotiantian
Merge branch 'main' into pdb-closure
9c68aef
@gaogaotiantian
Update the code for pep 667
fe5a6f6
@gaogaotiantian
gaogaotiantian requested a review from markshannon as a code owner May 5, 2024 00:55
@gaogaotiantian
Remove blank line
cc21873
@gaogaotiantian

gaogaotiantian commented May 5, 2024

Copy link
Copy Markdown
Member Author

I had to make quite a bit of changes to make this work with PEP 667 - locals() is not write-through anymore for function scope. Neither can it do pop(). So a couple of things were changed:

  1. locals is copied first and the writeback only happens at the end of the function.
  2. Whether there is nested function is checked first, if not, use the normal exec. This actually solves plenty of interesting issues like locals() (which will print the extra helper variables without the protection).
  3. Some of the reviews are applied.
  4. We can't use locals() to pass around data anymore, so everything we need is now stored in a dict __pdb_eval__.

This relies on the fix from #118583 so please do not merge it yet. I applied the change from that issue just to make the tests pass. I'll merge it in this PR when that one is merged in main.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gaogaotiantian gaogaotiantian added the DO-NOT-MERGE label May 5, 2024
@gaogaotiantian
Merge branch 'main' into pdb-closure
d37b5a7
@gaogaotiantian gaogaotiantian removed the DO-NOT-MERGE label May 5, 2024
@gaogaotiantian
gaogaotiantian removed request for carljm, gvanrossum, iritkatriel and markshannon May 5, 2024 03:47
gaogaotiantian added 2 commits May 4, 2024 20:56
@gaogaotiantian
Update 2023-10-20-03-50-17.gh-issue-83151.bcsD40.rst
cd46a05
@gaogaotiantian
Merge branch 'main' into pdb-closure
d04bc5d
@gaogaotiantian
Fix test for breakpoint change
aa91238
brandtbucher
brandtbucher reviewed May 6, 2024
View reviewed changes

@brandtbucher brandtbucher left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how this is starting to look. Some more thoughts:

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

gaogaotiantian and others added 2 commits May 5, 2024 21:12
@gaogaotiantian @brandtbucher
Apply suggestions from code review
7650aa9
Co-authored-by: Brandt Bucher <[email protected]>
@gaogaotiantian
Fix some typos in review and add exception check
3c44f88
@gaogaotiantian

gaogaotiantian commented May 6, 2024

Copy link
Copy Markdown
Member Author

We need to put the other exec in the try .. except block as well. The newly added test case is for that. Basically if we have a global variable x and we do

global x; (lambda: x)()

We should be able to get the value. But compiling this with our instrumentation would be:

nonlocal x
global x; (lambda: x)()

and returns an error that's confusing to users - *** SyntaxError: name 'g' is nonlocal and global

That's why we need to fall back to normal.

brandtbucher reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

brandtbucher
brandtbucher reviewed May 6, 2024
View reviewed changes
Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

brandtbucher
brandtbucher reviewed May 6, 2024
View reviewed changes
Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

brandtbucher
brandtbucher reviewed May 6, 2024
View reviewed changes
Comment thread Lib/pdb.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@gaogaotiantian @brandtbucher
Apply suggestions from code review
18e7a16
Co-authored-by: Brandt Bucher <[email protected]>
brandtbucher
brandtbucher approved these changes May 6, 2024
View reviewed changes

@brandtbucher brandtbucher left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this seems correct! Thanks for tackling such a longstanding and hairy issue, and for taking the time to walk me through the fix. :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels May 6, 2024
@brandtbucher
brandtbucher merged commit e5353d4 into python:main May 6, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label May 6, 2024
@gaogaotiantian
gaogaotiantian deleted the pdb-closure branch May 6, 2024 19:41
@gaogaotiantian

gaogaotiantian commented May 6, 2024

Copy link
Copy Markdown
Member Author

I'm glad this finally got fixed :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced May 6, 2024
Pdb NameError in generator param and list comprehension #80052
Closed
locally imported modules are unaccessible in lambdas in pdb #76987
Closed
pdb fails to access variables closed over #70260
Closed
list comprehensions don't see local variables in pdb in python3 #65360
Closed
@erlend-aasland erlend-aasland mentioned this pull request May 6, 2024
bpo-21161: pdb: default: handle list comprehensions #15194
Closed
1 of 3 tasks
SonicField pushed a commit to SonicField/cpython that referenced this pull request May 8, 2024
@gaogaotiantian @SonicField
pythonGH-83151: Add closure support to pdb (pythonGH-111094)
85a5202
@gaogaotiantian gaogaotiantian mentioned this pull request Aug 19, 2024
PDB missing function arguments in list comprehension #123141
Closed
@limwz01 limwz01 mentioned this pull request Nov 6, 2024
Global variables undefined in interactive use of embedded ipython shell ipython/ipython#62
Open
@SamB SamB mentioned this pull request Jan 19, 2025
[LICENSING ISSUE]: This is a non-compliant derivative of CPython code pdbpp/pdbpp#529
Open
@krassowski krassowski mentioned this pull request Jun 30, 2025
Backport Pdb closure evaluation fix (list comprehension, generators) from Python 3.13 ipython/ipython#14933
Merged
2 tasks done
krassowski added a commit to ipython/ipython that referenced this pull request Jul 1, 2025
@krassowski
Backport Pdb closure evaluation fix (list comprehension, generators) …
2197228
…from Python 3.13 (#14933)

- [x] add a failing test case
- [x] backport python/cpython#111094 into
IPython codebase
- fixes gotcha/ipdb#256
- solves the pdb-related comments on
#62


![image](https://github.com/user-attachments/assets/c1a33ad0-d8a5-44f2-84d3-73f708918747)


### Context

IPython already includes bits of Pdb code from Python itself, as
documented in:


https://github.com/ipython/ipython/blob/1e9a2e8b5a584ffe1973318fdabf2e39395ec367/IPython/core/debugger.py#L92-L110

I am taking a more cautious approach and separating the backport to a
new file to make it easy to delete in the (near) future (1 year from
now).
@krassowski krassowski mentioned this pull request Jul 4, 2025
Include Pdb._exec_in_closure() as code-path in exec() itself, or make it a public utility otherwise? #136280
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brandtbucher brandtbucher brandtbucher approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@brandtbucher brandtbucher

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@gaogaotiantian @gvanrossum @brandtbucher
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.