-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
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`
barneygale
left a comment
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 :)
|
|
||
| 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 == funchere 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_IsNonefunction (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_Nonemight 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_Nonechecks 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 (theXmeaning thatfuncmay 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 == funchere 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_IsNonefunction (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_Nonemight 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.fspathif__fspath__is set toNone#106046