Skip to content

gh-59000: Fix pdb breakpoint resolution for class methods when module not imported #141949

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gaogaotiantian merged 7 commits into from
Dec 2, 2025
+20 −1
Merged

gh-59000: Fix pdb breakpoint resolution for class methods when module not imported #141949

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7844490
fix incorrectly pdb break
cocolato Nov 25, 2025
701050f
Merge branch 'main' into fix_pdb_break
cocolato Nov 26, 2025
7b8fe20
fix test
cocolato Nov 30, 2025
25c8819
fix test
cocolato Nov 30, 2025
df88534
fix test
cocolato Dec 1, 2025
b59d79a
fix test
cocolato Dec 1, 2025
53ac260
fix test
cocolato Dec 1, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion Lib/pdb.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1487,7 +1487,9 @@ def lineinfo(self, identifier):
f = self.lookupmodule(parts[0])
if f:
fname = f
item = parts[1]
item = parts[1]
else:
return failed
answer = find_function(item, self.canonic(fname))
return answer or failed

Expand Down
25 changes: 25 additions & 0 deletions Lib/test/test_pdb.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4587,6 +4587,31 @@ def bar():
]))
self.assertIn('break in bar', stdout)

def test_issue_59000(self):
Copy link
Copy Markdown
Member

@gaogaotiantian gaogaotiantian Nov 29, 2025

Choose a reason for hiding this comment

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

You are testing the wrong thing. You should test C.foo, not C.c_foo. So a good practice is to confirm that your test fails before your fix and passes after. Also from the other PR - do not test what you don't want to. Make the test minimal. C.c_foo is not important here. Let's simplify the test.

Copy link
Copy Markdown
Member Author

@cocolato cocolato Nov 30, 2025

Choose a reason for hiding this comment

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

You are testing the wrong thing. You should test C.foo, not C.c_foo. So a good practice is to confirm that your test fails before your fix and passes after. Also from the other PR - do not test what you don't want to. Make the test minimal. C.c_foo is not important here. Let's simplify the test.

Thank you for taking the time to review. I have made some changes.

Copy link
Copy Markdown
Member

@gaogaotiantian gaogaotiantian Nov 30, 2025

Choose a reason for hiding this comment

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

You are still testing too much. The purpose of a regression test is to test for the specific thing you fixed. When other developers read this test in the future, they should understand what is the bug back then. It's different than a feature test which needs to be comprehensive.

The only thing in this fix is that the user should not be able to set a breakpoint for C.foo if foo exists - that's the thing you should test for. All the other stuff are not related. Let's minimize the test case so it tests just this case. And by "minimize", I meant you can't delete anything anymore because every line is useful.

Copy link
Copy Markdown
Member Author

@cocolato cocolato Dec 1, 2025

Choose a reason for hiding this comment

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

Thanks again for your review! I've pared the test cases down to the minimum needed.

script = """
def foo():
pass

class C:
def c_foo(self):
pass
def foo(self):
pass

foo()
"""
commands = """
break foo
break C.foo
break C.c_foo
break 10
continue
break C.foo
quit
"""
stdout, stderr = self.run_pdb_script(script, commands)
self.assertIn("The specified object 'C.c_foo' is not a function", stdout)
Copy link

Copilot AI Nov 25, 2025

Choose a reason for hiding this comment

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

The test only verifies that C.c_foo fails with an error, but doesn't verify the main bug that was fixed: that C.foo no longer incorrectly resolves to the top-level foo function before the module is executed. Add assertions to verify that C.foo also fails with the 'not found' error before execution (around line 4605), and that after continue (line 4609), C.foo successfully creates a breakpoint at the correct line. For example: self.assertIn(\"The specified object 'C.foo' is not a function\", stdout) to verify the fix prevents the incorrect resolution.

Copilot uses AI. Check for mistakes.


class ChecklineTests(unittest.TestCase):
def setUp(self):
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2025-11-25-16-00-29.gh-issue-59000.YtOyJy.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix :mod:`pdb` breakpoint resolution for class methods when the module defining the class is not imported.
Loading