Skip to content

gh-133956 fix bug where ClassVar string annotation in @dataclass caused incorrect __init__ generation #134073

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

gh-133956 fix bug where ClassVar string annotation in @dataclass caused incorrect __init__ generation #134073

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
92e2f76
fix: string annotations ClassVar bug
dzherb May 15, 2025
bae38af
doc: add NEWS entry
dzherb May 15, 2025
6f5c210
chore: correct comment phrasing
dzherb May 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 27 additions & 15 deletions Lib/dataclasses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -753,21 +753,33 @@ def _is_type(annotation, cls, a_module, a_type, is_type_predicate):
# that's defined. It was judged not worth it.

match = _MODULE_IDENTIFIER_RE.match(annotation)
if match:
ns = None
module_name = match.group(1)
if not module_name:
# No module name, assume the class's module did
# "from dataclasses import InitVar".
ns = sys.modules.get(cls.__module__).__dict__
else:
# Look up module_name in the class's module.
module = sys.modules.get(cls.__module__)
if module and module.__dict__.get(module_name) is a_module:
ns = sys.modules.get(a_type.__module__).__dict__
if ns and is_type_predicate(ns.get(match.group(2)), a_module):
return True
return False
if not match:
return False

ns = None
module_name = match.group(1)
Copy link

Choose a reason for hiding this comment

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

There is an edge case that is not handled by this: foo.typing_filtered.ClassVar. The regex only accounts for one module level. While it is probably uncommon to import foo.typing_filtered without aliasing it, it is possible.

I don't want to change the world for a simple bug fix, but it seems like partitioning on . is more robust (and probably more performant) than using the current regex.

Copy link
Author

Choose a reason for hiding this comment

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

While I agree that splitting on a dot would be faster, there are existing tests that cover cases like dataclasses.InitVar.[int] and dataclasses.InitVar+. If we want to support multiple module levels, we should extend the current regex pattern rather than replace it.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's worth creating a new issue to track more levels of module nesting. And I think it's a mistake to test for dataclasses.InitVar.[int] and other invalid strings.

Copy link

Choose a reason for hiding this comment

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

This bug was actually caught by __.typx.ClassVar in the real world. I typically stuff all of my common imports, including typing_extensions as typx, into an internals subpackage (__) so that I can do from . import __. Reduces module namespace pollution and lets me avoid setting __all__ to define module interfaces. But, I understand that my practice may not be common.

That said, any fix to multi-level traversal here or another PR is going to affect the code that is actually being fixed. Imo, it would make more sense to fix both together holistically (to save developer effort), if there is any interest in actually fixing the multi-level traversal.

type_name = match.group(2)

if not module_name:
# No module name, assume the class's module did
# "from dataclasses import InitVar".
ns = sys.modules.get(cls.__module__).__dict__
else:
# Look up module_name in the class's module.
cls_module = sys.modules.get(cls.__module__)
if not cls_module:
return False
Comment on lines +770 to +771
Copy link

Choose a reason for hiding this comment

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

It is interesting that we're paranoid about the dataclass not having its module imported on only this branch and not the branch above. Not suggesting a change, but a comment could be useful if you know why we only care in the else branch and not the if not module_name branch.

Copy link
Author

Choose a reason for hiding this comment

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

I was following the checks that were already present in the function. In the else branch, there was a check for if module:

module = sys.modules.get(cls.__module__)
if module and module.__dict__.get(module_name) is a_module:
      ns = sys.modules.get(a_type.__module__).__dict__

I’d actually consider removing that check — it does seem unnecessary. At least, I can’t think of a case where cls.__module__ would be missing from sys.modules. And the tests are still passing.

Copy link

Choose a reason for hiding this comment

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

Yes, I saw the original code. My suggestion would be to hoist module = sys.modules.get(cls.__module__) out of the if..else. If you want to preserve the rather paranoid sanity check alongside the hoisted assignment, you could. Something like:

module = sys.modules.get(cls.__module__)
if module:  # not sure that we need to be this paranoid 
    ns = module.__dict__
if module_name:
...


a_type_module = cls_module.__dict__.get(module_name)
if (
isinstance(a_type_module, types.ModuleType)
# Handle cases when a_type is not defined in
# the referenced module, e.g. 'dataclasses.ClassVar[int]'
Copy link

Choose a reason for hiding this comment

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

InitVar lives in dataclasses, ClassVar lives in typing. Using an example of dataclasses.ClassVar is likely to cause confusion.

and a_type_module.__dict__.get(type_name) is a_type
):
ns = sys.modules.get(a_type.__module__).__dict__
Comment on lines +774 to +780
Copy link

Choose a reason for hiding this comment

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

I think this could be replaced with:

return (
    isinstance(a_type_module, types.ModuleType)
    and is_type_predicate(a_type_module.__dict__.get(type_name), a_module))

Copy link
Author

Choose a reason for hiding this comment

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

It looks like we can indeed replace it. But if we go with this version, the a_type parameter will no longer be used in the function body.

Also, I was thinking — in the original version, this line:

sys.modules.get(a_type.__module__)

could probably be replaced with a_module? Or am I missing something? It seems like a_type might not have been necessary from the start, and we should probably remove it.

Copy link
Member

Choose a reason for hiding this comment

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

If you think the code can be simplified, please do so, then I'll review that version.


return ns and is_type_predicate(ns.get(type_name), a_module)


def _get_field(cls, a_name, a_type, default_kw_only):
Expand Down
10 changes: 7 additions & 3 deletions Lib/test/test_dataclasses/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4093,10 +4093,14 @@ def test_classvar_module_level_import(self):
from test.test_dataclasses import dataclass_module_1_str
from test.test_dataclasses import dataclass_module_2
from test.test_dataclasses import dataclass_module_2_str
from test.test_dataclasses import dataclass_module_3
from test.test_dataclasses import dataclass_module_3_str

for m in (dataclass_module_1, dataclass_module_1_str,
dataclass_module_2, dataclass_module_2_str,
):
for m in (
dataclass_module_1, dataclass_module_1_str,
dataclass_module_2, dataclass_module_2_str,
dataclass_module_3, dataclass_module_3_str,
):
with self.subTest(m=m):
# There's a difference in how the ClassVars are
# interpreted when using string annotations or
Expand Down
6 changes: 6 additions & 0 deletions Lib/test/test_dataclasses/_types_proxy.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# We need this to test a case when a type
# is imported via some other package,
# like ClassVar from typing_extensions instead of typing.
# https://github.com/python/cpython/issues/133956
from typing import ClassVar
from dataclasses import InitVar
32 changes: 32 additions & 0 deletions Lib/test/test_dataclasses/dataclass_module_3.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#from __future__ import annotations
USING_STRINGS = False

# dataclass_module_3.py and dataclass_module_3_str.py are identical
# except only the latter uses string annotations.

from dataclasses import dataclass
import test.test_dataclasses._types_proxy as tp

T_CV2 = tp.ClassVar[int]
T_CV3 = tp.ClassVar

T_IV2 = tp.InitVar[int]
T_IV3 = tp.InitVar

@dataclass
class CV:
T_CV4 = tp.ClassVar
cv0: tp.ClassVar[int] = 20
cv1: tp.ClassVar = 30
cv2: T_CV2
cv3: T_CV3
not_cv4: T_CV4 # When using string annotations, this field is not recognized as a ClassVar.

@dataclass
class IV:
T_IV4 = tp.InitVar
iv0: tp.InitVar[int]
iv1: tp.InitVar
iv2: T_IV2
iv3: T_IV3
not_iv4: T_IV4 # When using string annotations, this field is not recognized as an InitVar.
32 changes: 32 additions & 0 deletions Lib/test/test_dataclasses/dataclass_module_3_str.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
from __future__ import annotations
USING_STRINGS = True

# dataclass_module_3.py and dataclass_module_2_str.py are identical
# except only the latter uses string annotations.

from dataclasses import dataclass
import test.test_dataclasses._types_proxy as tp

T_CV2 = tp.ClassVar[int]
T_CV3 = tp.ClassVar

T_IV2 = tp.InitVar[int]
T_IV3 = tp.InitVar

@dataclass
class CV:
T_CV4 = tp.ClassVar
cv0: tp.ClassVar[int] = 20
cv1: tp.ClassVar = 30
cv2: T_CV2
cv3: T_CV3
not_cv4: T_CV4 # When using string annotations, this field is not recognized as a ClassVar.

@dataclass
class IV:
T_IV4 = tp.InitVar
iv0: tp.InitVar[int]
iv1: tp.InitVar
iv2: T_IV2
iv3: T_IV3
not_iv4: T_IV4 # When using string annotations, this field is not recognized as an InitVar.
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Library/2025-05-16-01-43-58.gh-issue-133956.5kWDYd.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix bug where ``ClassVar`` string annotation in :func:`@dataclass <dataclasses.dataclass>` caused incorrect __init__ generation
Copy link
Member

@ericvsmith ericvsmith May 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm just starting to look at this PR, and will have more to say later. For now: I'd prefer this to say something more specific, like <some specific description> of ClassVar not recognized as typing.ClassVar. I'd like this because I'm sure we won't be fixing all instances of using ClassVar as string annotations, and we should mention the exact one being fixed here. Also, I don't want to mention __init__, because that's not the only method affected by this bug.

Loading