bpo-44554: refactor pdb targets (and internal tweaks) by jaraco · Pull Request #26992 · python/cpython · GitHub

Refactor module/script handling to share an interface (check method).
Import functools and adjust tests for the new line number for find_function.
Use cached_property for details.
Add blurb.

https://bugs.python.org/issue44554

Automerge-Triggered-By: GH:jaraco

This line changes the meaning of mainpyfile mid-function. It's because of this behavior that ScriptTarget contains an 'orig' property (to keep both representations), even though the "real path" form is the preferred one except during error checking.

This changes the behavior of pdb, right?

This PR extracts the refactoring only, but not the fix for bpo-44461, as discussed in iritkatriel#16.

I would be careful with it - pdb invokes code in several different way (from command line, from the pdb prompt, from Pdb subclasses that various debuggers implement).

One place this refactoring could go awry is if there were other callers of Pdb._runmodule, because this code changes the parameter type for that method. By my estimation, that method was only ever called from main().

Please do have a look at this approach as it focuses on the refactoring and ignores the concerns for now in fixing the reported issue.

This changes the behavior of pdb, right?

I do not believe this changes the behavior except for a couple subtle differences. I'll point those out in the diff.

A refactor changes the code to improve its structure without changing its behavior. I don't think you can call this a refactor.

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

Previously, the opts would be iterated over just once. In this new code, they are iterated over three times. Any performance degradation is negligible and outweighed by the value of disentangling the concerns.

In the previous implementation, assignment of sys.argv[:] would happen before the sys.path change. In the new code, the sys.path change happens during the ScriptTarget.check. I contend these two operations are independent and can happen in any order.

Please do have a look at this approach as it focuses on the refactoring and ignores the concerns for now in fixing the reported issue.

This PR doesn't really have anything to do with the reported issue. It should be a new BPO.

Previously, the existence check was done on mainpyfile before it was mutated to a "real path". Now the existence check is the real path. If compatibility is needed, this line could be changed to:

Suggested change

      
                    if not os.path.exists(self):
          
                    if not os.path.exists(self.orig):

But in my opinion, the existence check on either form is equivalent, and the form presented is more readable and distracts less from the primary intention for self.orig (the error message).

It's not equivalent for anonymous pipes created by process substitution (see GH-142315).

A refactor changes the code to improve its structure without changing its behavior. I don't think you can call this a refactor.

Perhaps I'm using the term too loosely. I believe the changes do not change the behavior or interfaces for the intended use-cases.

This PR doesn't really have anything to do with the reported issue. It should be a new BPO.

Respectfully, I disagree. The whole point of this refactor is to enable an earlier check for a runpy module target, symmetric to the syntax check on a script target, a technique which has been shown to fix the issue (thanks to your analysis and test).

A refactor changes the code to improve its structure without changing its behavior. I don't think you can call this a refactor.

Perhaps I'm using the term too loosely. I believe the changes do not change the behavior or interfaces for the intended use-cases.

I think you are using it too loosely. A refactor improves the code without changing its behavior. As I said, pdb has quite a few different use cases. I don't know it well enough to tell which changes are minor and which will break someone's debugger. Any behavior change needs to be associated with a bpo and news item that states clearly what the change is, not sneaked into a refactor.

I'm also not sure about the motivation here. We have a fix for the reported bug.

I think you are using it too loosely. A refactor improves the code without changing its behavior.

I've updated the blurb and PR title to reflect that it's more than a refactor. But I do genuinely expect this change to result in no significant behavior change. Can you point to a behavior change that you think is more than a refactor?

I'm also not sure about the motivation here. We have a fix for the reported bug.

The motivation comes from my observation here that when the target is a script, there's a separate check for its existence (and similarly later a separate trap for bad syntax) that doesn't go through the Pdb interactive module. In my opinion, passing an invalid module to -m is equivalent to passing a non-existent script and should be handled similarly. I would not expect the interactive prompt to appear if a non-executable module is indicated as a runpy target. In other words, I don't expect Pdb to break into runpy code any more than I would expect it to break into CPython code if a script didn't exist or had a syntax error.

As I analyzed the issue, following your analysis and initial patch, I felt uneasy with adding the .botframe property. I asked myself, why is the botframe property needed for only this use case? And as you found, it's because the Bdb.run is never called. That got me thinking - the Pdb interactive prompt probably shouldn't be invoked at all if .run is never reached. So I asked myself, why is .run never reached in this case, and it's because runpy is raising an exception loading the module and checking it for runability. And that's what led me to the consideration that the module should be checked for runability early, and a failure to pass runability shouldn't trigger the "Unhandled Exception" logic and interactive prompt.

We have a fix for the reported bug.

I believe that by first applying this refactor, then applying the test and fix in iritkatriel#16, you get a superior fix, avoiding adding a .botframe property to Bdb where it was previously not needed and avoiding the need to duplicately check for _user_requested_quit (although I suspect that change may also be independently beneficial). Additionally, with this refactor, the fix for the issue becomes a few lines in a single location (implementing ModuleTarget.check) instead of altering behavior in other classes and states.

Can you point to a behavior change that you think is more than a refactor?

Since it handles bad modules differently (we agree on that right?) it's already more than a refactor.

That may be a minor change, it might be a good change, I don't know. I need to spend some time reviewing it. I've moved on to work on something else between last week and now. I don't have time at the moment to convince myself that your change is safe. I'll have a look when I have time, unless someone else will before me.

Since it handles bad modules differently (we agree on that right?)

Sorry this wasn't clear. In this change, ModuleTarget.check is empty, matching the existing behavior. Bad modules are not handled. This change does not fix bpo-44461, but only refactors the code such that there is a designated place for such a check to exist. I would expect to merge this change prior to the fix for bpo-44461.

But as I think about it, perhaps the concerns this change addresses could be enumerated in its own bug.

I don't have time at the moment

No problem. I understand. Thanks for taking the time to review as much as you have already and for keeping me apprised of your schedule so I'm not left feeling abandoned.

…nction.

Thanks for making the requested changes!

@iritkatriel: please review the changes made to this pull request.

…n properties of the Target object.

In fe8aea5, I've pushed an additional refactor that consolidates _run(:ModuleTarget) and _run(:ScriptTarget) by implementing the differential behaviors on each target. This approach dramatically simplifies the implementation of _run but also clarifies the purpose of each target.

This change has the additional benefit of restoring the association of the comments with their function. Prior to this change, the two block comments in _run_script were disassociated from their corresponding implementation in _run_module.

This version is indeed clearer. Can spec() and details() be _spec() and _details()?

…Union in the module.

This version is indeed clearer. Can spec() and details() be _spec() and _details()?

Done in c91ba80.

LGTM

@jaraco: Status check is done, and it's a success ❌ .

@jaraco: Status check is done, and it's a success ✅ .

Sorry, I can't merge this PR. Reason: 3 of 5 required status checks are expected..

@jaraco: Status check is done, and it's a success ✅ .

* origin/main: (1146 commits) bpo-42064: Finalise establishing sqlite3 global state (pythonGH-27155) bpo-44678: Separate error message for discontinuous padding in binascii.a2b_base64 strict mode (pythonGH-27249) correct spelling (pythonGH-27076) bpo-44524: Add missed __name__ and __qualname__ to typing module objects (python#27237) bpo-27513: email.utils.getaddresses() now handles Header objects (python#13797) Clean up comma usage in Doc/library/functions.rst (python#27083) bpo-42238: Fix small rst issue in NEWS.d/. (python#27238) bpo-41972: Tweak fastsearch.h string search algorithms (pythonGH-27091) bpo-44340: Add support for building with clang full/thin lto (pythonGH-27231) bpo-44661: Update property_descr_set to use vectorcall if possible. (pythonGH-27206) bpo-44645: Check for interrupts on any potentially backwards edge (pythonGH-27216) bpo-41546: make pprint (like print) not write to stdout when it is None (pythonGH-26810) bpo-44554: refactor pdb targets (and internal tweaks) (pythonGH-26992) bpo-43086: Add handling for out-of-spec data in a2b_base64 (pythonGH-24402) bpo-44561: Update hyperlinks in Doc/distributing/index.rst (python#27032) bpo-42355: symtable.get_namespace() now checks whether there are multiple or any namespaces found (pythonGH-23278) bpo-44654: Do not export the union type related symbols (pythonGH-27223) bpo-44633: Fix parameter substitution of the union type with wrong types. (pythonGH-27218) bpo-44654: Refactor and clean up the union type implementation (pythonGH-27196) bpo-20291: Fix MSVC warnings in getargs.c (pythonGH-27211) ...

the-knights-who-say-ni added the CLA signed label Jul 2, 2021

bedevere-bot added the awaiting core review label Jul 2, 2021

jaraco requested a review from iritkatriel July 2, 2021 13:26

jaraco commented Jul 2, 2021

View reviewed changes

jaraco added the 🤖 automerge label Jul 2, 2021

iritkatriel requested changes Jul 2, 2021

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting core review labels Jul 2, 2021

jaraco commented Jul 2, 2021

View reviewed changes

jaraco changed the title bpo-44461: refactor pdb targets bpo-44461: refactor pdb targets (and internal tweaks) Jul 2, 2021

jaraco added 6 commits July 2, 2021 14:16

Refactor module/script handling to share an interface (check method).

59b5906

Import functools and adjust tests for the new line number for find_fu…

4df7611

…nction.

Use cached_property for details.

86fbc39

Add blurb.

825e35b

Add comments about 'orig'

914ec41

Rely on singledispatch to dispatch the target.

b959e3e

bedevere-bot added awaiting change review and removed awaiting changes labels Jul 2, 2021

bedevere-bot requested a review from iritkatriel July 2, 2021 22:49

iritkatriel reviewed Jul 15, 2021

View reviewed changes

Comment thread Misc/NEWS.d/next/Library/2021-07-02-18-17-56.bpo-44554.aBUmJo.rst Outdated

Comment thread Lib/pdb.py

jaraco added 2 commits July 18, 2021 08:42

Simplify changelog per suggestion.

e21f085

Replace singledispatch _run with a unified implementation selecting o…

fe8aea5

…n properties of the Target object.

iritkatriel reviewed Jul 18, 2021

View reviewed changes

Suppress spurious errors in pyclbr when it encounters properties and …

0d8611c

…Union in the module.

jaraco mentioned this pull request Jul 18, 2021

Refactor module/script handling to share an interface. iritkatriel/cpython#16

Closed

Make _spec and _details private.

c91ba80

iritkatriel approved these changes Jul 18, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jul 18, 2021

miss-islington merged commit 2c20558 into main Jul 19, 2021

miss-islington deleted the bpo-44461-refactor-targets branch July 19, 2021 01:00

bedevere-bot removed the awaiting merge label Jul 19, 2021

hauntsaninja mentioned this pull request Apr 24, 2023

gh-103578: Fix pdb reading code with non-utf8 encoding #103581

Merged

gaogaotiantian mentioned this pull request Dec 1, 2023

Refactor pdb executable targets #112570

Merged

johnslavik mentioned this pull request Dec 7, 2025

gh-142315: Don't pass the "real path" of Pdb script target to system functions #142371

Merged

+                  commands = [optarg for opt, optarg in opts if opt in ['-c', '--command']]
-                  if not run_as_module:
-                      mainpyfile = os.path.realpath(mainpyfile)

-                  commands = []
-                  run_as_module = False
-                  for opt, optarg in opts:


		target.check()

		sys.argv[:] = args # Hide "pdb.py" and pdb options from argument list

+                      return res
+                  def check(self):
+                      if not os.path.exists(self):

	if not os.path.exists(self):
	if not os.path.exists(self.orig):

bpo-44554: refactor pdb targets (and internal tweaks) by jaraco · Pull Request #26992 · python/cpython · GitHub

Repository navigation

bpo-44554: refactor pdb targets (and internal tweaks) - #26992

bpo-44554: refactor pdb targets (and internal tweaks)#26992miss-islington merged 14 commits intomainpython/cpython:mainfrom bpo-44461-refactor-targetspython/cpython:bpo-44461-refactor-targetsCopy head branch name to clipboard

Conversation

jaraco commented Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaraco Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

iritkatriel commented Jul 2, 2021

Uh oh!

jaraco commented Jul 2, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaraco commented Jul 2, 2021

Uh oh!

iritkatriel left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jul 2, 2021

Uh oh!

jaraco Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

jaraco Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

iritkatriel commented Jul 2, 2021

Uh oh!

jaraco Jul 2, 2021

Choose a reason for hiding this comment

Uh oh!

johnslavik Dec 5, 2025

Choose a reason for hiding this comment

Uh oh!

jaraco commented Jul 2, 2021

Uh oh!

jaraco commented Jul 2, 2021

Uh oh!

iritkatriel commented Jul 2, 2021

Uh oh!

jaraco commented Jul 2, 2021

Uh oh!

jaraco commented Jul 2, 2021

Uh oh!

iritkatriel commented Jul 2, 2021

Uh oh!

jaraco commented Jul 2, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-bot commented Jul 2, 2021

Uh oh!

Uh oh!

Uh oh!

jaraco commented Jul 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iritkatriel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jaraco commented Jul 18, 2021

Uh oh!

iritkatriel left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Jul 18, 2021

Uh oh!

miss-islington commented Jul 19, 2021

Uh oh!

miss-islington commented Jul 19, 2021

Uh oh!

miss-islington commented Jul 19, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

bpo-44554: refactor pdb targets (and internal tweaks)#26992
miss-islington merged 14 commits into
mainfrom
bpo-44461-refactor-targets

jaraco commented Jul 2, 2021 •
edited
Loading

jaraco commented Jul 2, 2021 •
edited by bedevere-bot
Loading

jaraco commented Jul 2, 2021 •
edited by bedevere-bot
Loading

jaraco commented Jul 18, 2021 •
edited
Loading

iritkatriel left a comment •
edited
Loading