FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-106046: Improve error message from `os.fspath` if `__fspath__` is set to `None` by AlexWaygood · Pull Request #106082 · python/cpython · GitHub

/ cpython Public

gh-106046: Improve error message from os.fspath if __fspath__ is set to None - #106082

Merged
AlexWaygood merged 5 commits into
python:mainfrom
AlexWaygood:fspath
Jun 25, 2023
Merged

gh-106046: Improve error message from os.fspath if __fspath__ is set to None#106082
AlexWaygood merged 5 commits into
python:mainfrom
AlexWaygood:fspath

Conversation

AlexWaygood commented Jun 25, 2023
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

Comment thread Lib/test/test_os.py Outdated

barneygale left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks so much for looking at this :)

Comment thread Modules/posixmodule.c

func = _PyObject_LookupSpecial(o, &_Py_ID(__fspath__));
if (NULL == func) {
if ((NULL == func) || (func == Py_None)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 write if (func = NULL), and it doesn't do what you want (it always sets func to NULL). The Yoda condition prevents this, because if (NULL = func) is probably a syntax error. However, Yoda conditions aren't that useful these days as compilers will warn about if (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 where Py_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 putting Py_XDECREF(func) inside the conditional block (the X meaning that func may be NULL).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
  • The NULL == func here is a "Yoda condition". People write that because C lets you write if (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!

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 == :)

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3.13 bugs and security fixes type-feature A feature request or enhancement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve error message from os.fspath if __fspath__ is set to None

4 participants


Back | FazBrowse Home | New Git URL