Repository navigation
Propagate errors from typing.ClassVar attribute lookup (#1221) - #1223
Conversation
Siyet
left a comment
Siyet
left a comment
There was a problem hiding this comment.
Could you remove the claim that regular builds silently dropped the lookup error? On regular CPython 3.12.13, the original test already raises AttributeError before this change, and the property case also preserves the original RuntimeError. The fix prevents the assertion failure by returning immediately when the lookup fails.
|
Thanks, you're right. I dropped that sentence from the description. On a regular build the original error still surfaces; the fix is about the assertion failure on debug builds, where returning right away when the lookup fails avoids hitting assert(!PyErr_Occurred()). |
|
Thx, could you also remove that claim from |
|
Done, removed it from the changelog entry too (ccc3a24). |
| a: typing.ClassVar | ||
| """ | ||
|
|
||
| source = source.replace("a: typing.ClassVar", f"a: {annotation}") |
There was a problem hiding this comment.
Can we instead use f string to format source directly? It does not seem safe now.
Per review: interpolate the parameterized annotation directly into the module template instead of running str.replace over it. Signed-off-by: Charan Rathore <[email protected]>
|
Thanks for the review, both fair points. The f-string: agreed, done in 1bdc3a1. The template now interpolates The |
Closes #1221
The #1097 fix looks up
ClassVaron whatevertypingnames in the modulenamespace, but if that attribute lookup itself raises, the error return is
missed:
structmeta_is_classvarcompares the NULL result againsttyping.ClassVar, reports "not a ClassVar", and returns 0 with theexception still pending. On CPython builds with assertions enabled this
trips
assert(!PyErr_Occurred())intype_new_initand terminates theinterpreter.
Return -1 when the lookup fails, matching the error handling already used
for the
__origin__lookup just below. The original AttributeError from#1096 still propagates exactly as before.
Tests: the existing
test_wrong_classvarregression is parametrized to alsocover the subscripted form
typing.ClassVar[int], and a new test covers atypingstand-in whoseClassVarattribute raises an arbitrary error, sothe propagation path itself is pinned rather than only the #1096 case.
Verified on a locally built CPython 3.13.7
--with-pydebug: the unpatchedcode stops the interpreter in
test_wrong_classvar(exit 134); with thepatch the full unit suite passes (6298 passed, 313 skipped). Focused tests
also pass on a regular 3.13 build, which is what CI runs.
Changelog entry included under Unreleased.
Tested on Python 3.13 only (debug and regular builds). CI covers 3.10 to 3.15; the change is a two-line error check in version-independent C.