◐ Shell
reader mode source ↗
Skip to content

gh-98886: Fix issues with dataclass fields with special underscore names#102032

Merged
hauntsaninja merged 5 commits into
python:mainfrom
hauntsaninja:gh-98886
Mar 25, 2023
Merged

gh-98886: Fix issues with dataclass fields with special underscore names#102032
hauntsaninja merged 5 commits into
python:mainfrom
hauntsaninja:gh-98886

Conversation

@hauntsaninja

@hauntsaninja hauntsaninja commented Feb 18, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

This commit prefixes __dataclass to several things in the locals dict:

  • Names like _dflt_ (which cause trouble, see first test)
  • Names like _type_ (not known to be able to cause trouble)
  • _return_type (not known to able to cause trouble)
  • _HAS_DEFAULT_FACTORY (which causes trouble, see second test)

In addition, this removes MISSING from the locals dict. As far as I can tell, this wasn't needed even in the initial implementation of dataclasses.py (and tests on that version passed with it removed). This makes me wary :-)

This is basically a continuation of #96151, where fixing this was welcomed in #98143 (comment)

@hauntsaninja
pythongh-98886: Fix issues with fields with special underscore names …
8b906cb 
This commit prefixes `__dataclass` to several things in the locals dict:
- Names like _dflt_ (which cause trouble, see first test)
- Names like _type_ (not known to be able to cause trouble)
- _return_type (not known to able to cause trouble)
- _HAS_DEFAULT_FACTORY (which causes trouble, see second test)

In addition, this removes `MISSING` from the locals dict. As far as I
can tell, this wasn't needed even in the initial implementation of
dataclasses.py (and tests on that version passed with it removed).

This is basically a continuation of python#96151, where fixing this was
welcomed in python#98143 (comment)
@bedevere-bot bedevere-bot mentioned this pull request Feb 18, 2023
Closed
@hauntsaninja hauntsaninja changed the title gh-98886: Fix issues with fields with special underscore names Feb 18, 2023
@carljm

carljm commented Mar 8, 2023

Copy link
Copy Markdown
Member

The name you eventually settled on for builtins.object in #98143 is __dataclass_builtins_object__. This name uses both initial and trailing double-underscore. That seems ideal to me, since such names are explicitly reserved for Python's use, whereas names that are only prefixed with a double underscore are merely private (and mangled), but not reserved. And in fact today it is possible to make a dataclass with a double-underscore-prefixed attribute, and the basics at least seem to work fine (the field name is mangled, as expected.)

What do you think about changing all the names in this PR to have both leading and trailing underscores?

@hauntsaninja

hauntsaninja commented Mar 8, 2023

Copy link
Copy Markdown
Contributor Author

Thanks, fixed!

carljm
carljm approved these changes Mar 8, 2023
View reviewed changes

@carljm carljm left a comment

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.

Hide comment

LGTM. I agree the MISSING thing is a bit odd, but I examined the code for _init_fn and the functions it calls (_init_param and _field_init) and I can't see any way for MISSING to show up in the text of the init function to create. And clearly the tests pass without it.

@hauntsaninja hauntsaninja merged commit 718e866 into python:main Mar 25, 2023
@hauntsaninja hauntsaninja deleted the gh-98886 branch March 25, 2023 21:40
Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this pull request Mar 27, 2023
@hauntsaninja @Fidget-Spinner
pythongh-98886: Fix issues with dataclass fields with special undersc… …
8e4f648 
…ore names (python#102032)

This commit prefixes `__dataclass` to several things in the locals dict:
- Names like `_dflt_` (which cause trouble, see first test)
- Names like `_type_` (not known to be able to cause trouble)
- `_return_type` (not known to able to cause trouble)
- `_HAS_DEFAULT_FACTORY` (which causes trouble, see second test)

In addition, this removes `MISSING` from the locals dict. As far as I can tell, this wasn't needed even in the initial implementation of dataclasses.py (and tests on that version passed with it removed). This makes me wary :-)

This is basically a continuation of python#96151, where fixing this was welcomed in python#98143 (comment)
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
@hauntsaninja @warsaw
pythongh-98886: Fix issues with dataclass fields with special undersc… …
a740700 
…ore names (python#102032)

This commit prefixes `__dataclass` to several things in the locals dict:
- Names like `_dflt_` (which cause trouble, see first test)
- Names like `_type_` (not known to be able to cause trouble)
- `_return_type` (not known to able to cause trouble)
- `_HAS_DEFAULT_FACTORY` (which causes trouble, see second test)

In addition, this removes `MISSING` from the locals dict. As far as I can tell, this wasn't needed even in the initial implementation of dataclasses.py (and tests on that version passed with it removed). This makes me wary :-)

This is basically a continuation of python#96151, where fixing this was welcomed in python#98143 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@carljm carljm

@ericvsmith ericvsmith Awaiting requested review from ericvsmith ericvsmith is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@hauntsaninja @carljm @bedevere-bot