-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-106046: Improve error message from os.fspath
if __fspath__
is set to None
#106082
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
Conversation
…_` is set to `None`
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
…ests fail elsewhere
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.
Thanks so much for looking at this :)
@@ -1197,7 +1197,7 @@ path_converter(PyObject *o, void *p) | |||
PyObject *func, *res; | |||
|
|||
func = _PyObject_LookupSpecial(o, &_Py_ID(__fspath__)); | |||
if (NULL == func) { | |||
if ((NULL == func) || (func == Py_None)) { |
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.
A few thoughts, no change requested:
- The parentheses are technically redundant, but it's OK to keep them for extra clarity.
- The
NULL == func
here is a "Yoda condition". People write that because C lets you writeif (func = NULL)
, and it doesn't do what you want (it always sets func to NULL). The Yoda condition prevents this, becauseif (NULL = func)
is probably a syntax error. However, Yoda conditions aren't that useful these days as compilers will warn aboutif (func = NULL)
. Arguably you're introducing an inconsistency here as one of the two conditions on this line is Yoda and the other isn't, but I think that's fine. - There is a
Py_IsNone
function (https://docs.python.org/3/c-api/structures.html#c.Py_IsNone) that could be used instead of== Py_None
. I think the motivation is to allow alternate C APIs wherePy_None
might not be a singleton. I don't know if there is any push to use this function in CPython itself, though I see a few calls to it. However, there are already several== Py_None
checks in this file, so it seems fine to add a few more. - I already mentioned this on Discord, but before 3.12 this would have introduced a refleak, as the branch never decrefs
func
. However, now Py_None is immortal and we no longer have to worry about refcounting for it. Otherwise, I'd have suggested puttingPy_XDECREF(func)
inside the conditional block (theX
meaning thatfunc
may be NULL).
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.
- The
NULL == func
here is a "Yoda condition". People write that because C lets you writeif (func = NULL)
, and it doesn't do what you want (it always sets func to NULL).
Thanks for explaining this! I wondered what the rationale was here for writing it like this!
- There is a
Py_IsNone
function (https://docs.python.org/3/c-api/structures.html#c.Py_IsNone) that could be used instead of== Py_None
. I think the motivation is to allow alternate C APIs wherePy_None
might not be a singleton.
Huh. I checked the API docs for Py_None
before filing this PR, and they didn't mention this function — they recommended using ==
: https://docs.python.org/3/c-api/none.html. I guess I'll leave this as it is for now, since as you say, there are a bunch of other places in this file that use ==
:)
Closes #106046
os.fspath
if__fspath__
is set toNone
#106046