Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.3k
gh-121450: Make inline breakpoints use the most recent pdb instance#121451
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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| Inline breakpoints (:func:`breakpoint` and:func:`pdb.set_trace()`)will use themost recent``Pdb`` instance, instead of creating a new one. | |
| Inline breakpoints (:func:`breakpoint` and:func:`pdb.set_trace()`)now reuse thesame``Pdb`` instance, instead of creating a new one each time. |
iritkatriel commentedJul 8, 2024
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? |
gaogaotiantian commentedJul 10, 2024
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 The behavior changes when user is debugging between |
iritkatriel commentedJul 10, 2024
This could surprise someone, so best explain it in what's new. |
Lib/pdb.py Outdated
| def__new__(self,*args,**kwargs): | ||
| Pdb._last_pdb_instance=super().__new__(self) | ||
| returnPdb._last_pdb_instance |
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.
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?
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.
Maybe Pdb._last_pdb_instance should be set in set_trace().
Probably need a test for this scenario too.
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.
You mean inPdb.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 abreakpoint(), 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.
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.
Not sure I understand. If you set_last_pdb_instance inPdb.set_trace() then you get clear semantics, and no other breakpoint interrupts with this. Is that not a good thing?
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.
No I agree that setting_last_pdb_instance inPdb.set_trace() is a good approach. And I just realized the nested debugging (debug command in pdb) usessys.call_tracing so it probably won't affect anything if we do it inPdb.set_trace(). I'll do the change and see how it works out.
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.
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 aPdb instance in the pdb prompt is really rare - what's the point?
iritkatriel commentedJul 10, 2024
LGTM. Still needs a what's new entry, I think. |
iritkatriel left a comment
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.
LGTM, add a what's new entry and it's good to go.
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
Uh oh!
There was an error while loading.Please reload this page.
pdb.set_trace()will use the most recent pdb instance created if there is one. A minor fix inbdbis introduced otherwise the debugger will stop inBdb.reset(). A small fix intest_pdbis also required because we need to make sure the instancepdb.set_traceis 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_tracein our documentation so I don't even see a conflict with existing documentation. Not sure if any new documentation is needed at this point.