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

The SystemError description is misleading #129407

Open
ZeroIntensity opened this issue Jan 28, 2025 · 9 comments
Open

The SystemError description is misleading #129407

ZeroIntensity opened this issue Jan 28, 2025 · 9 comments
Labels
3.12 bugs and security fixes 3.13 bugs and security fixes 3.14 new features, bugs and security fixes docs Documentation in the Doc dir

Comments

@ZeroIntensity
Copy link
Member

ZeroIntensity commented Jan 28, 2025

Documentation

The docs for SystemError currently say:

Raised when the interpreter finds an internal error, but the situation does not look so serious to cause it to abandon all hope. The associated value is a string indicating what went wrong (in low-level terms).

You should report this to the author or maintainer of your Python interpreter. Be sure to report the version of the Python interpreter (sys.version; it is also printed at the start of an interactive Python session), the exact error message (the exception’s associated value) and if possible the source of the program that triggered the error.

A SystemError isn't always an interpreter problem, at least in CPython's case. A SystemError can be caused relatively easily through misuse of the C API, so I don't think it's a good idea to point people to the issue tracker if they ever see a SystemError.

Linked PRs

@ZeroIntensity ZeroIntensity added 3.12 bugs and security fixes 3.13 bugs and security fixes 3.14 new features, bugs and security fixes docs Documentation in the Doc dir labels Jan 28, 2025
@ericvsmith
Copy link
Member

I don't think it's worth changing this. We don't get a ton of reports of this nature, and when we do, we just say "you're using it wrong".

@ZeroIntensity
Copy link
Member Author

But my point is that we shouldn't get any incorrect reports derived from the SystemError documentation--what's the harm in changing it?

@markshannon
Copy link
Member

I don't think the docs are the problem, and we shouldn't change them.
The problem, IMO, is that SystemError is raised when it shouldn't be.

One example is when a builtin function sets the exception and returns a value, or returns NULL and doesn't set an exception.

@ZeroIntensity @hoodmane
Do you have other examples?

If the problem is extensions, not the VM, raising SystemError then we could, in theory, prevent that. But I doubt it would be worth it.

@markshannon
Copy link
Member

Eurgh. There are loads of places we raise SystemError and probably shouldn't 🙁

Maybe we should add a new VMError class for actual "system errors"?

@hoodmane
Copy link
Contributor

Yeah the most common cases are due to extension functions violating the calling conventions. There's also many public C APIs will raise a system error if their argument is a null pointer or otherwise their arguments fail some precondition.

I agree it would be better to fix the interpreter to distinguish between "the interpreter messed up" and "package code messed up". But that's a much larger project than this minor documentation adjustment.

Given that currently SystemErrors most frequently do result from mistakes in downstream code, I think updating the documentation to reflect this is an improvement over doing nothing and a lot less work than changing the behavior.

@markshannon
Copy link
Member

Maybe it is not quite as bad as it first appears. A lot of the uses of SystemError seem to be in tests.

We could leave SystemError be and add a new ApiMisuseException?
Some places where ApiMisuseException could replace SystemError:

  • Incorrectly setting the exception when returning from a builtin function, as described above
  • PyBytes_FromStringAndSize, PyUnicode_FromStringAndSize, etc. with a negative size
  • PyFile_WriteString with NULL file
  • PyUnicode_New with an invalid character
  • PyArg_Parse variants with invalid formats/arguments

@markshannon
Copy link
Member

Given that currently SystemErrors most frequently do result from mistakes in downstream code, I think updating the documentation to reflect this is an improvement over doing nothing and a lot less work than changing the behavior.

It may well be less work, but I don't think it is the right thing to do long term.
We could change the docs temporarily, and change them back once we've added once we've added ApiMisuseException?

@markshannon
Copy link
Member

@vstinner
Copy link
Member

See also #95799.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes 3.13 bugs and security fixes 3.14 new features, bugs and security fixes docs Documentation in the Doc dir
Projects
Status: Todo
Development

No branches or pull requests

5 participants