Skip to content

gh-130660: Revert sys.ps1 and sys.ps2 after code.interact #130661

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

Merged
merged 5 commits into from
Feb 28, 2025

Conversation

gaogaotiantian
Copy link
Member

@gaogaotiantian gaogaotiantian commented Feb 28, 2025

I'd consider this as a bug fix as the current behavior does not match what we documented. However, it's a behavior that has been in the code base for a while (in a relatively dark corner), so I'm still open to suggestions.

@gaogaotiantian
Copy link
Member Author

I'm fully aware that this is a 28-year old behavior - which I still think should be fixed. But maybe the original author @gvanrossum can share some thoughts about it?

@gvanrossum
Copy link
Member

I don't particularly care whether we fix this or not -- if someone actually checks the presence of sys.ps1 to determine whether we're interactive they are already on thin ice. (The REPL itself checks sys.stdin.isatty() AFAICT.

I have one suggestion: Instead of trying to set _ps1 to the value of sys.ps1 or None if that isn't set, why not keep a bool that records directly whether it was set? That would avoid the problem that if I set sys.ps1 = None (which seems legit, at least it works just like setting it to any other non-string value), the current patch mistakes this for "not set" and deletes it.

There's also logic in the new REPL to reset sys.ps1 to the default (">>> ") if it is not set at the point where it's trying to read a new line. Might be worth replicating that in code.InteractiveConsole.interact().

(Another bug in the new REPL seems to be that if I set sys.ps1 to something that raises an exception in __str__(), this crashes the interpreter. In 3.12 and before this just didn't print a prompt. Maybe this could be treated the same as ps1 not existing?)

@gaogaotiantian
Copy link
Member Author

I have one suggestion: Instead of trying to set _ps1 to the value of sys.ps1 or None if that isn't set, why not keep a bool that records directly whether it was set?

Yeah I can do that.

There's also logic in the new REPL to reset sys.ps1 to the default (">>> ") if it is not set at the point where it's trying to read a new line. Might be worth replicating that in code.InteractiveConsole.interact().

Do you mean that if the user changes sys.ps1 during input? I can work on that as well but do you think it should be a separate PR?

The reason I looked at this is that pdb recently added a check for whether the user is in interactive mode. Basically if the user starts the interactive interpreter and do breakpoint(), then q, we don't want to raise a SystemExit, in order to keep the old behavior on terminals like REPL or ipython. I don't think sys.stdin.isatty() works in this case - that only checks if the stdin is atty, but it's totally possible that the stdin is atty and the user is not interactive mode.

@gvanrossum
Copy link
Member

You're right that checking isatty() is wrong -- it will be true when running python file.py for example. My bad.

But are there docs that suggest checking sys.ps1 is the right thing, other than inferring it from the docs for sys.ps1?

Next we'll see users setting sys.ps1 just to affect the debugger... That would be unfortunate too.

Note that IDLE has a shell that has its own ps1 and ps2, which are initialized from sys.ps1 etc., but if you start it directly from the unix shell (instead of from the Python REPL), sys.ps1 is not set. In that case, even though it is clearly an interactive shell, sys.ps1 never gets set. I suppose this should be filed as an IDLE bug. (Additionally, I cannot actually start an IDLE shell in a way that does inherit sys.ps1 -- it always uses the defaults. But that may be my lack of IDLE-fu.)

@gaogaotiantian
Copy link
Member Author

Well, pdb has its own interactive interface and it does not set sys.ps1 either - that's how it can check whether it's started from an interactive shell. Although we can argue that it's not the "interpreter" that's in interactive mode. It's a user-defined stdin based interface. Imaging a simple loop with input() - that's basically what pdb and idle is - not an interpreter.

I don't know if there's a documentation about how sys.ps1 should be used. I would love something more official that can sys.ps1 to tell us if we are using interactive interpreter. However, the documentation of sys.ps1 is an acceptable evidence to me that this can be used (relied on?).

@sergey-miryanov
Copy link
Contributor

(Another bug in the new REPL seems to be that if I set sys.ps1 to something that raises an exception in __str__(), this crashes the interpreter. In 3.12 and before this just didn't print a prompt. Maybe this could be treated the same as ps1 not existing?)

This should be fixed by #130503

Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

self.assertEqual(self.sysmod.ps1, '>>> ')
output = ''.join(''.join(call[1]) for call in self.stdout.method_calls)
self.assertIn('>>> ', output)
self.assertFalse(hasattr(self.sysmod, 'ps1'))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assertNotHasAttr

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it depends on whether we consider this a bug fix and backport it. assertNotHasAttr is new in 3.14 which would cause issues when we backport the test. If we only want to fix this in 3.14 (which is fine by me because I only need this in 3.14), then sure we can use the latest method.

@gvanrossum do you think we should backport this? Or just leave it be for the previous versions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These methods were backported as private helpers in test.support. You will need to add an import and a mix-in, but you will get clearer testing code with better error reporting.

@gvanrossum
Copy link
Member

(Another bug in the new REPL seems to be that if I set sys.ps1 to something that raises an exception in __str__(), this crashes the interpreter. In 3.12 and before this just didn't print a prompt. Maybe this could be treated the same as ps1 not existing?)

This should be fixed by #130503

No, I can still repro it. I will make a separate bug report.

Regarding IDLE, its shell window definitely is (intended to be) an interpreter (I should know because I started the project long ago). Anyway, the behavior of pdb there is a bit confusing -- pdb warns that exiting will end the process, but if I select Yes there, it just goes back to the IDLE shell with a fresh prompt. So you don't have to fix anything there.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't backport this. It's an utter edge case but it does change observable behavior.

@gaogaotiantian
Copy link
Member Author

Anyway, the behavior of pdb there is a bit confusing -- pdb warns that exiting will end the process, but if I select Yes there, it just goes back to the IDLE shell with a fresh prompt.

Ah, okay. So if IDLE sets sys.ps1, pdb will raise a bdb.BdbQuit exception which would probably be caught by IDLE (so no prompt there) - that's the intended behavior. If you have other thoughts about how quit command should behave in different scenarios, let me know.

@gaogaotiantian gaogaotiantian merged commit fdcbc29 into python:main Feb 28, 2025
39 checks passed
@gaogaotiantian gaogaotiantian deleted the restore-ps1-after-interact branch February 28, 2025 18:16
@gvanrossum
Copy link
Member

Anyway, the behavior of pdb there is a bit confusing -- pdb warns that exiting will end the process, but if I select Yes there, it just goes back to the IDLE shell with a fresh prompt.

Ah, okay. So if IDLE sets sys.ps1, pdb will raise a bdb.BdbQuit exception which would probably be caught by IDLE (so no prompt there) - that's the intended behavior. If you have other thoughts about how quit command should behave in different scenarios, let me know.

The "problem" is that IDLE doesn't set sys.ps1, but somehow handles SystemExit differently (by simply ignoring it). So pdb says it is going to kill the process, but it doesn't. I think we can live with that, I don't think pdb needs to have a special case for IDLE (which has a built-in visual debugger anyway).

@gaogaotiantian
Copy link
Member Author

Yeah that makes sense.

@terryjreedy
Copy link
Member

"if IDLE sets sys.ps1": IDLE never sets sys.ps1 in either the UI process or the default-mode user-code execution process. If the user sets it in the execution process, it can affect other code run by the user, such as pdb. But sys.ps1 in the UI process is not affected.

When we moved the primary >>> prompt to a 3-character wide sidebar and added secondary ... prompts, to make indentation look right in Shell, we hard-coded both prompts. IDLE currently reads sys.ps1, if it exists, only when initializing the shell window, and after initializing its ModifiedInterpreter subclass of code.InteractiveInterpreter. But the string seems to no longer matter. So setting it to whatever string and unsetting it when the interpreter is deleted should not affect IDLE.

(I opened a draft issue on IDLE Issues (view) to fix IDLE's failure when sys.ps1 is not a string. If IDLE continues to read it, it should treat it as information to be displayed separately from the >>> and ... prompts.)

In the REPL, indentation no longer looks right if a user changes the length of ps1 but not ps2. Does anyone know, briefly, what sorts of non-default prompts are used?

@terryjreedy
Copy link
Member

The sys.ps1/2 doc says "These are only defined if the interpreter is in interactive mode." This literally only prohibits setting in batch mode, which is the subject of this issue. Guido's comment #130661 (comment) suggests that it is also meant to require setting. And I agree that being consistent here with the REPL would be good. But it does not seem very easy. I added draft issue "IDLE: set execution process sys.ps1/2 when interactive" to the Run section of IDLE Issues.

@terryjreedy
Copy link
Member

About pdb not exiting: the IDLE process was designed, before me, to survive anything user code does to the execution process. A user who wants a clean subprocess can explicitly request one on the Shell menu (or use the restart hotkey). A restart is also done if the user process does somehow die. I agree that pdb should not try to force that.

@gvanrossum
Copy link
Member

@terryjreedy

"if IDLE sets sys.ps1": IDLE never sets sys.ps1 in either the UI process or the default-mode user-code execution process. If the user sets it in the execution process, it can affect other code run by the user, such as pdb. But sys.ps1 in the UI process is not affected.

Oh, so this means the UI process cannot see changes to sys.ps1 in the run process. Fair enough.

When we moved the primary >>> prompt to a 3-character wide sidebar and added secondary ... prompts, to make indentation look right in Shell, we hard-coded both prompts. IDLE currently reads sys.ps1, if it exists, only when initializing the shell window, and after initializing its ModifiedInterpreter subclass of code.InteractiveInterpreter. But the string seems to no longer matter. So setting it to whatever string and unsetting it when the interpreter is deleted should not affect IDLE.

So you read sys.ps1 in the UI but it ends up not being used... Searching the code I found where it was read but didn't realize it wasn't actually used because of the change to move the shell prompt to a separate UI column (that was a good call BTW, solves a lot of problems).

(I opened a draft issue on IDLE Issues (view) to fix IDLE's failure when sys.ps1 is not a string. If IDLE continues to read it, it should treat it as information to be displayed separately from the >>> and ... prompts.)

What would happen if you just removed the code that reads it?

In the REPL, indentation no longer looks right if a user changes the length of ps1 but not ps2. Does anyone know, briefly, what sorts of non-default prompts are used?

Honestly I have no idea. I imagine many folks of the hacking persuasion have ended up coding something in there that shows dynamic data, like people put in bash prompts all the time (bash even has a mini-language for that purpose). In Python you can just do it by writing an object with a dynamic __str__ method. I would imagine that the current date/time and the current directory might be handy. The indentation issue can be solved by adding "\n>>> " to the end.

The sys.ps1/2 doc says "These are only defined if the interpreter is in interactive mode." This literally only prohibits setting in batch mode, which is the subject of this issue. Guido's comment #130661 (comment) suggests that it is also meant to require setting.

I think from that sentence you can also infer that they are always defined in interactive mode -- it doesn't say "These may only be defined if ...", it says "These are only defined if ...".

And I agree that being consistent here with the REPL would be good. But it does not seem very easy. I added draft issue "IDLE: set execution process sys.ps1/2 when interactive" to the Run section of IDLE Issues.

Yeah, now that I'm reminded of the separation between UI and run process, I agree it might not be so easy. Perhaps the part of the run process that talks to the UI process could just set sys.ps1 = ">>> "; sys.ps2 = "... " once when it starts up (since from that point on IIUC it only executes code sent to it by the UI/shell window). Other uses of sys.ps1 might still reflect whatever it's set to, e.g.

>>> sys.ps1 = "~~> "
>>> code.interact()
    Python 3.14...
    Type "help", ...
    (InteractiveConsole)
    ~~> ^D
>>> 

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants