Skip to content

Conversation

@cyyever
Copy link
Contributor

@cyyever cyyever commented Dec 9, 2025

Description

PyObject_HasAttrString can return -1 and be treated as having attr.

Signed-off-by: cyy <cyyever@outlook.com>
@cyyever cyyever changed the title Fix PyObject_HasAttrString return value Fix PyObject_HasAttrString checking Dec 9, 2025
rwgk added 2 commits December 12, 2025 21:58
PyObject_HasAttrString may return -1 to signal an error and set a
Python exception. The previous logic only checked for "!= 0", which
meant that the error path was treated the same as "attribute exists",
causing two problems: misreporting the presence of __notes__ and
leaving a spurious exception pending.

The earlier PR tightened the condition to "== 1" so that only a
successful lookup marks the error string as [WITH __notes__], but it
still left the -1 case unhandled. In the context of
error_fetch_and_normalize, we are already dealing with an active
exception and only want to best-effort detect whether normalization
attached any __notes__. If the attribute probe itself fails, we do not
want that secondary failure to affect later C-API calls or the error
we ultimately report.

This change stores the PyObject_HasAttrString return value, treats
"== 1" as "has __notes__", and explicitly calls PyErr_Clear() when
it returns -1. That way, we avoid leaking a secondary error while
still preserving the original exception information and hinting
[WITH __notes__] only when we can determine it reliably.
@rwgk
Copy link
Collaborator

rwgk commented Dec 13, 2025

I pushed a commit, so you can look at it. I'll remove the [skip ci] later and force push when our CI isn't so busy.

I believe when I worked on the code originally, I looked at the PyObject_HasAttrString CPython sources to fully understand how it works. I'm not sure anymore about the details, but possibly I decided the return value can only be 0 or 1, i.e. the simple if condition is all we need. However, this PR surely makes the code safer and more obvious and future proof. I think it's a good change.

Copy link
Contributor Author

@cyyever cyyever left a comment

Choose a reason for hiding this comment

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

LGTM

@rwgk
Copy link
Collaborator

rwgk commented Dec 13, 2025

The commits here were merged into PR #5929, so that we can test both changes in the same CI run.

@rwgk rwgk closed this Dec 13, 2025
@cyyever cyyever deleted the fix2 branch December 13, 2025 11:07
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.

2 participants