Make accessing dunder-names on a CTrait object an AttributeError #1469
Conversation
There was a problem hiding this comment.
Mostly LGTM but with a few questions
References I used when reviewing this PR
- https://docs.python.org/3.6/c-api/unicode.html?highlight=pyunicode_ready#c.PyUnicode_READY (this is why
is_dunder_namewill set an exception on failure) - https://docs.python.org/3.6/c-api/unicode.html?highlight=pyunicode_ready#c.PyUnicode_GET_LENGTH
- https://docs.python.org/3.6/c-api/unicode.html?highlight=pyunicode_ready#c.PyUnicode_KIND (returns one of wchar, 1 byte, 2 byte and 4 byte)
- https://docs.python.org/3.6/c-api/unicode.html?highlight=pyunicode_ready#c.PyUnicode_DATA
- https://docs.python.org/3.6/c-api/unicode.html?highlight=pyunicode_ready#c.PyUnicode_READ
- https://docs.python.org/3.6/c-api/none.html?highlight=py_return_none#c.Py_RETURN_NONE (replaces
Py_INCREF(Py_None); return Py_None)
traits/ctraits.c
Outdated
| else if (is_dunder) { | ||
| /* Retry the attribute lookup so that we get the correct exception | ||
| message. */ | ||
| return PyObject_GenericGetAttr((PyObject *)obj, name); |
There was a problem hiding this comment.
IIUC, we're having to retry in this clause instead of simply removing the PyErr_Clear outside of the conditional statements because the other two clauses still need/want to use PyErr_Clear. Is that correct?
There was a problem hiding this comment.
Hmm. This is really just laziness. What I want to do here is re-raise the original AttributeError. I don't want to create my own exception message, since the one from the interpreter is likely to be better (and may even change with more recent versions of Python - there's a lot of work on error messages right now).
I can't simply remove the PyErr_Clear because is_dunder_name might set an exception, and it's an error to set another exception if there's already an exception set (or at least, I can't find any evidence that it's okay to do so, and there's CPython core code that goes out of its way to avoid this).
The right way to handle this without the retry is to do a PyErr_Fetch / PyErr_Restore pairing. I'll aim to update the PR to do that.
There was a problem hiding this comment.
and it's an error to set another exception if there's already an exception set (or at least, I can't find any evidence that it's okay to do so, and there's CPython core code that goes out of its way to avoid this).
Hmm, actually, looking into the source, this may be okay. Investigating ...
There was a problem hiding this comment.
From the source, all the exception setting mechanisms end up at _PyErr_SetObject, which, if an exception is already set, applies implicit exception chaining. I think that's good enough for this case. I've reworked the code accordingly.
There was a problem hiding this comment.
From the source, ...
I didn't mean to send you down a rabbit hole 😅
|
Still LGTM |
Sorry, @rahulporuri: I think the above message came just a couple of minutes before I updated to simplify the exception handling. Could you do another quick check? |
|
@rahulporuri Thank you! Merging. |
This PR changes attribute lookups on
CTraitobjects to raiseAttributeErrorfor names beginning and ending with a double underscore, and for which the normal attribute lookup rules don't return anything. Previously, those attribute lookups would returnNone, and this broke napoleon, which expected that a lookup of__qualname__on aCTraitobject would either raise or return a string.Attribute lookups for names that don't begin and end with a double underscore are unchanged.
No documentation update: it's not currently documented that arbitrary attribute accesses on a
CTraitobject returnNone, and I'm not sure that I really want it to be - it's not a "feature" of Traits that I'd want to encourage people to rely on.Fixes #1463.
Checklist
docs/source/traits_api_reference)docs/source/traits_user_manual)traits-stubs