-
-
Notifications
You must be signed in to change notification settings - Fork 33.6k
gh-136057: Allow step and next to step over for loops #136160
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-136057: Allow step and next to step over for loops #136160
Conversation
Lib/bdb.py
Outdated
| if self.stop_here(frame) or self.break_here(frame): | ||
| # GH-136057 | ||
| # For line events, besides whether we should stop at the frame, we | ||
| # also need to check if it's the same line as we issue the command. |
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.
I don't think this is very clear: "it's the same line as we issue the command". I think you mean "the same frame and line at which the break command was issued by the user".
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.
It's not the break command, it's either next or step. But yes I'm open to all suggestions to make this clearer.
Lib/bdb.py
Outdated
| self.stoplineno = stoplineno | ||
| # startframe/startlineno is the frame/line number when the user does | ||
| # step or next. We don't want to stop at the same line for those commands. | ||
| self.startframe = startframe |
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 we could pick a more informative name, something like "command frame", because it's ambiguous here what exactly started at that frame.
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.
Yeah we can change it, but there's actually a caveat here - it's not really the command frame. You can do up to an upper frame and do next/step - the behavior is different. next will next to the line at the upper frame, so the frame we need is the "command frame". step however, will stop at the innermost frame, no matter which frame you are inspecting - that's just how step works.
So it's more like a "do not stop here again if it's a line event" frame/lineno. Do you think there's better name to describe it?
|
@iritkatriel do you think the current name and comments are better? |
Lib/bdb.py
Outdated
| if self.stop_here(frame) or self.break_here(frame): | ||
| # GH-136057 | ||
| # For line events, we don't want to stop at the same line where | ||
| # we issue the previous next/step command. |
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.
If we stop, it's not necessarily due to a next/step, right? Could it also be a breakpoint or until, etc?
So maybe the comment can say:
If we are stopping due to a next/step command, we don't want to stop on the same line on which
the command was issued.
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.
So I'm thinking. If we are currently at line 10, and the user does a until 10, should we stop at the current line? And if we stopped by a breakpoint, then we do continue, should we stop at the same line? Those are kind of undefined behaviors, unlike step and next which explicitly said that the debugger should stop at the "next" line.
Co-authored-by: Irit Katriel <[email protected]>
|
Hi @iritkatriel , want to circle back to this. I made a small change to initialize the attributes - just merged a PR for pdb so I think might as well do it for at least the newly added attributes for bdb. |
|
|
||
| def test_pdb_issue_gh_136057(): | ||
| """See GH-136057 | ||
| "step" and "next" commands should be able to get over list comprehensions |
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.
Shouldn't it be that next steps over the list comp but step executes every iteration?
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 according to our documentation
Execute the current line, stop at the first possible occasion (either in a function that is called or on the next line in the current function).
I mean we can have different interpretation of the phrase "first possible occasion". Technically we can do this at bytecode level - but that's probably not we really want. I think we should stick to the explanation in the parenthesis.
|
Thanks @gaogaotiantian for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14. |
…-136160) (cherry picked from commit 8be3b2f) Co-authored-by: Tian Gao <[email protected]>
|
Sorry, @gaogaotiantian, I could not cleanly backport this to |
|
GH-141640 is a backport of this pull request to the 3.14 branch. |
|
GH-141641 is a backport of this pull request to the 3.13 branch. |
Due to historical reasons,
sys.settraceissues a line event forforloops even if the jump target is on the same line:We don't want to touch this behavior, because it's been like this for like forever.
However, it's not desirable to
pdb. When the user doesnextorstepcommand, they want to step over this line and reach the next line.Our documentation also clearly states such behavior:
So I consider the current behavior as a bug, which we should fix.
A thought might be - why don't we check this in
stop_here? The reason is becausedispatch_exceptionalso usesstop_hereand we should triggeruser_exceptioneven if it's on the same line.An extra check in
dispatch_lineis the cleanest way I can think of to make this happen - only line events should be dealt with. We don't need to worry about the extra reference to a frame because the eventualcontinue(or anything other thanstepornext) will clear this reference.Let me know if anyone has a better idea of how to implement this.