Skip to content
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-108463: Make expressions/statements work as expected in pdb #108464

Merged
merged 7 commits into from Sep 4, 2023

Conversation

gaogaotiantian
Copy link
Member

@gaogaotiantian gaogaotiantian commented Aug 25, 2023

This PR makes expressions like

c.a
c['a']
n()
j=1
r"a"

work in pdb.

@gaogaotiantian
Copy link
Member Author

Hi @iritkatriel , this is a very straighforward feature (implementation wise) and the code change is trivial. It's only about whether we should do it or not. Could you give it a review when you have some time? Thanks!

Lib/pdb.py Outdated Show resolved Hide resolved
@iritkatriel
Copy link
Member

Hi @iritkatriel , this is a very straighforward feature (implementation wise) and the code change is trivial. It's only about whether we should do it or not. Could you give it a review when you have some time? Thanks!

Sorry only saw this now.

Can this break anything?

@gaogaotiantian
Copy link
Member Author

Sorry only saw this now.

Can this break anything?

I can't think of any meaningful case that this could break. This change only affects how "cmd" parse commands - basically what characters can be considered as the command. In pdb, we expect users to use space to separate commands and args. All the supported commands only contain letters.

So in real world, this will only make a difference when the user types a string that contains characters other than letters, numbers and underscores, for example, c['a']. In such cases, the user always almost want to evaluate the expression(or execute the statement), but forget that c is a command.

This feature basically changed the behavior when the user types such statements/expressions - it does not report "command invalid" anymore - it runs the statement/expression.

@iritkatriel
Copy link
Member

This needs to be documented quite prominently as a new feature because it changes behaviour quite visibly. Someone might be surprised that something now executes code when it was previously a NOP.

@gaogaotiantian
Copy link
Member Author

gaogaotiantian commented Sep 3, 2023

This needs to be documented quite prominently as a new feature because it changes behaviour quite visibly. Someone might be surprised that something now executes code when it was previously a NOP.

I think the behavior after this change actually is closer to what the current documentation claims.

Arguments to commands must be separated by whitespace (spaces or tabs)

This defines how pdb should treat commands and arguments, which is not what we are doing now. According to the current docs, if the user inputs c['a'], it should be treated as a "command" because there's no whitespace anywhere. However, the current behavior treats it as command c + argument ['a'], which is against the docs.

I don't think anyone would be surprised if they input c['a'] and pdb evaluates the expression. If anything, this change eliminates a lot of surprises. The user would be pretty surprised if they do c['a'] and pdb complaints that it does not recognize the argument ['a'].

We also claimed in the docs that

Commands that the debugger doesn’t recognize are assumed to be Python statements and are executed in the context of the program being debugged

which is exactly what this change tries to do - c['a'] is not a command the debugger recognizes and should be considered as a Python expression.

Why would the user expect different behavior when they do d['a'] (currently evaluates the expression) and c['a'] (currently reports an error)? That's not intuitive.

I honestly can not think of a single case where the user would feel surprised with this change, even if they are very used to the old pdb.

@iritkatriel
Copy link
Member

I'm not disputing the value of the change. But it's a change in behaviour which needs to be documented (a versionchanged entry, a what's new in 3.13 entry).

Maybe someone will be surprised by the 3.12 behaviour, or the change between 3.12 and 3.13. It should be clear from the docs that this is a deliberate change.

@gaogaotiantian
Copy link
Member Author

I'm not disputing the value of the change. But it's a change in behaviour which needs to be documented (a versionchanged entry, a what's new in 3.13 entry).

Maybe someone will be surprised by the 3.12 behaviour, or the change between 3.12 and 3.13. It should be clear from the docs that this is a deliberate change.

Okay, I'm a bit confused about what to document, as this is more like a "bug fix" to make pdb behave like it claimed.

Maybe

pdb will execute the Python statement even if the first piece of the statement (seperated by spaces) starts with a pdb command.

This is basically what this feature fixes. I'm not sure how to organize the explanation as it's doing what the docs say. Any suggestsions on the matter?

@iritkatriel
Copy link
Member

I think something like:

.. versionchanged 3.13

   Expressions whose prefix is a pdb command are now correctly identified and executed.

And something similar in what's new in 3.13.

@iritkatriel iritkatriel enabled auto-merge (squash) September 4, 2023 21:26
@iritkatriel iritkatriel added the stdlib Python modules in the Lib dir label Sep 4, 2023
@iritkatriel iritkatriel merged commit 6304d98 into python:main Sep 4, 2023
25 checks passed
@gaogaotiantian gaogaotiantian deleted the pdb-improve-commands branch September 4, 2023 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants