gh-121450: Make inline breakpoints use the most recent pdb instance - #121451
gh-121450: Make inline breakpoints use the most recent pdb instance#121451gaogaotiantian merged 9 commits into
Conversation
Sorry, something went wrong.
| @@ -0,0 +1 @@ | |||
| Inline breakpoints (:func:`breakpoint` and :func:`pdb.set_trace()`) will use the most recent ``Pdb`` instance, instead of creating a new one. | |||
There was a problem hiding this comment.
| Inline breakpoints (:func:`breakpoint` and :func:`pdb.set_trace()`) will use the most recent ``Pdb`` instance, instead of creating a new one. | |
| Inline breakpoints (:func:`breakpoint` and :func:`pdb.set_trace()`) now reuse the same ``Pdb`` instance, instead of creating a new one each time. |
Sorry, something went wrong.
|
Are you sure this can't change behaviour in some cases, where the pdb instance now has a different state? For example, what if the user added breakpoints at the prompt between two breakpoint() calls? |
Sorry, something went wrong.
|
This will change the behavior, in favor of the expectation, in some cases, and that's the reason of the change. Adding breakpoints is not impacted because bdb uses a very weird pattern for holding breakpoints. All the breakpoints are stored as class members so every pdb instance can share all the breakpoints - I would guess originally it was designed this way simply to avoid multiple instance issues. The behavior changes when user is debugging between breakpoint() calls. Now breakpoint() will instantiate a new pdb instance, and lose all the instance specific data the user sets (for example, breakpoint commands, or display). So the user might set a command at a breakpoint like "print(a) when hitting this breakpoint". Then a breakpoint() hit, this command will be discarded forever. |
Sorry, something went wrong.
This could surprise someone, so best explain it in what's new. |
Sorry, something went wrong.
|
|
||
| def __new__(self, *args, **kwargs): | ||
| Pdb._last_pdb_instance = super().__new__(self) | ||
| return Pdb._last_pdb_instance |
There was a problem hiding this comment.
what if we're creating this instance while debugging? It would overwrite the instance from the hard coded breakpoint, and this one will be reused at the next hard coded breakpoint. This is not what's intended right?
Sorry, something went wrong.
There was a problem hiding this comment.
Maybe Pdb._last_pdb_instance should be set in set_trace().
Probably need a test for this scenario too.
Sorry, something went wrong.
There was a problem hiding this comment.
You mean in Pdb.set_trace()? That would make some sense, only enlist the instance when it explicitly takes over the tracing.
However, if the user somehow creates and uses a pdb instance before a breakpoint(), we really can't determine the purpose. It could be nested debugging (debug command in pdb which I think we should patch to make sure the _last_pdb_instance is not modified). Or the user could want a new instance, for example, in the test cases.
Sorry, something went wrong.
There was a problem hiding this comment.
Not sure I understand. If you set _last_pdb_instance in Pdb.set_trace() then you get clear semantics, and no other breakpoint interrupts with this. Is that not a good thing?
Sorry, something went wrong.
There was a problem hiding this comment.
No I agree that setting _last_pdb_instance in Pdb.set_trace() is a good approach. And I just realized the nested debugging (debug command in pdb) uses sys.call_tracing so it probably won't affect anything if we do it in Pdb.set_trace(). I'll do the change and see how it works out.
Sorry, something went wrong.
There was a problem hiding this comment.
And this change actually made the test change unnecessary so it's good. Do you think we need to test more scenarios? I think the case where users explicitly create a Pdb instance in the pdb prompt is really rare - what's the point?
Sorry, something went wrong.
|
LGTM. Still needs a what's new entry, I think. |
Sorry, something went wrong.
iritkatriel
left a comment
There was a problem hiding this comment.
LGTM, add a what's new entry and it's good to go.
Sorry, something went wrong.
With 3.14, breakpoints use the most recently created Pdb instance, in order to maintain history, display commands etc. See gh-121450, python/cpython#121451
With 3.14, breakpoints use the most recently created Pdb instance, in order to maintain history, display commands etc. See gh-121450, python/cpython#121451
With 3.14, breakpoints use the most recently created Pdb instance, in order to maintain history, display commands etc. See gh-121450: python/cpython#121451 Also includes misc fixes tests: - test_break_with_inner_set_trace - test_set_trace_remembers_previous_state - set_trace/set_trace_via_module - set_trace_with_incomplete_pdb - test_commands_* - test_nested_completer - test_integration - fix fixtures (set_trace, ...)
With 3.14, breakpoints use the most recently created Pdb instance, in order to maintain history, display commands etc. See gh-121450: python/cpython#121451 Also includes misc fixes tests: - test_break_with_inner_set_trace - test_set_trace_remembers_previous_state - set_trace/set_trace_via_module - set_trace_with_incomplete_pdb - test_commands_* - test_nested_completer - test_integration - fix fixtures (set_trace, ...)
With 3.14, breakpoints use the most recently created Pdb instance, in order to maintain history, display commands etc. See gh-121450: python/cpython#121451 Also includes misc fixes tests: - fix fixtures (set_trace, ...) - test_break_with_inner_set_trace - test_set_trace_remembers_previous_state - set_trace/set_trace_via_module - fix set_trace_in_completion - set_trace_with_incomplete_pdb - test_commands_* - test_nested_completer - test_integration - test_locals - completions tests
With 3.14, breakpoints use the most recently created Pdb instance, in order to maintain history, display commands etc. See gh-121450: python/cpython#121451 Also includes misc fixes tests: - fix fixtures (set_trace, ...) - test_break_with_inner_set_trace - test_set_trace_remembers_previous_state - set_trace/set_trace_via_module - fix set_trace_in_completion - set_trace_with_incomplete_pdb - test_commands_* - test_nested_completer - test_integration - test_locals - completions tests
pdb.set_trace() will use the most recent pdb instance created if there is one. A minor fix in bdb is introduced otherwise the debugger will stop in Bdb.reset(). A small fix in test_pdb is also required because we need to make sure the instance pdb.set_trace is using the patched instance.
The common debugger experience won't be much different to the users. We never explain the implementation details of pdb.set_trace in our documentation so I don't even see a conflict with existing documentation. Not sure if any new documentation is needed at this point.