GeezerVison

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.

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

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.

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.

Thanks for making the requested changes!

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

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()?

Done in c91ba80.

@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 ✅ .