◐ Shell
reader mode source ↗
Skip to content

gh-96151: Use a private name for passing builtins to dataclass#98143

Merged
ericvsmith merged 8 commits into
python:mainfrom
hauntsaninja:gh-96151
Oct 31, 2022
Merged

gh-96151: Use a private name for passing builtins to dataclass#98143
ericvsmith merged 8 commits into
python:mainfrom
hauntsaninja:gh-96151

Conversation

@hauntsaninja

@hauntsaninja hauntsaninja commented Oct 10, 2022 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

There's no indication that BUILTINS is a special name. Other names that are special to dataclass are all prefixed by an underscore.

As mentioned in the issue, we can also avoid this locals dance altogether by using ().__class__.__base__ instead of BUILTINS.object.

@hauntsaninja
pythongh-96151: Use a private name for passing builtins to dataclass …
3e03d52 
There's no indication that BUILTINS is a special name. Other names that
are special to dataclass are all prefixed by an underscore.

As mentioned in the issue, we can also avoid this locals dance
altogether by using `().__class__.__base__` instead of
`BUILTINS.object`.
sobolevn
sobolevn reviewed Oct 10, 2022
View reviewed changes

@sobolevn sobolevn 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

I think such subtile change needs a test case :)

@ericvsmith ericvsmith self-assigned this Oct 10, 2022
@hauntsaninja

hauntsaninja commented Oct 17, 2022

Copy link
Copy Markdown
Contributor Author

@JelleZijlstra I made the change, but note that if single underscore names are considered fair game for users, you can elicit all kinds of bad behaviour from dataclass. For example:

from dataclasses import dataclass, field

@dataclass
class X:
    x: int = field(default_factory=lambda: 111)
    _dflt_x: int = field(default_factory=lambda: 222)

X()

I'm happy to open an issue and fix all of these as well, if you think it's worth doing.

@TeamSpen210

TeamSpen210 commented Oct 17, 2022

Copy link
Copy Markdown

If we’re only actually using setattr, since __init__ is probably a rather hot bit of code would it be a bit more efficient to bind the __setattr__ method itself in the scope? Then there’s less lookups for each attribute being set.

@hauntsaninja

hauntsaninja commented Oct 17, 2022

Copy link
Copy Markdown
Contributor Author

Maybe? I heard a rumour that obj.method(...) is faster these days than m = obj.method; m(...). I'll do some benchmarking and if I get good results putting object or object.__setattr__ into scope, I'll open an issue.

@ericvsmith

ericvsmith commented Oct 17, 2022

Copy link
Copy Markdown
Member

About single underscores:

I'm happy to open an issue and fix all of these as well, if you think it's worth doing.

That's worth opening an issue for.

@hauntsaninja hauntsaninja mentioned this pull request Oct 30, 2022
Closed
@hauntsaninja

hauntsaninja commented Oct 30, 2022

Copy link
Copy Markdown
Contributor Author

Thanks, I opened #98886 for the single underscores.

Meanwhile, I also modified this PR to use a slightly different name, since my suggestion in #98886 is to have all these special names prefixed with __dataclass_ (like we already do for __dataclass_self__). I think this PR should be good to go.

@bedevere-bot bedevere-bot mentioned this pull request Oct 31, 2022
Closed
@ericvsmith ericvsmith added 3.11 only security fixes only security fixes labels Oct 31, 2022
@ericvsmith

ericvsmith commented Oct 31, 2022

Copy link
Copy Markdown
Member

This all looks great. I'm not sure if this will backport cleanly, but I've added the tags for 3.10 and 3.11 backports I see this as a bug fix that should be backported, at least to 3.11.

@ericvsmith ericvsmith merged commit 29f98b4 into python:main Oct 31, 2022
@AlexWaygood AlexWaygood removed the 3.11 only security fixes label Oct 31, 2022
@AlexWaygood AlexWaygood added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes and removed 3.10 only security fixes labels Oct 31, 2022
@miss-islington

miss-islington commented Oct 31, 2022

Copy link
Copy Markdown
Contributor

Thanks @hauntsaninja for the PR, and @ericvsmith for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@miss-islington

miss-islington commented Oct 31, 2022

Copy link
Copy Markdown
Contributor

Thanks @hauntsaninja for the PR, and @ericvsmith for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 31, 2022
@hauntsaninja @miss-islington
pythongh-96151: Use a private name for passing builtins to dataclass.… …
d0debcb 
… This now allows for a field named BUILTIN (pythongh-98143)

(cherry picked from commit 29f98b4)

Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
@bedevere-bot

bedevere-bot commented Oct 31, 2022

Copy link
Copy Markdown

GH-98899 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Oct 31, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 31, 2022
@hauntsaninja @miss-islington
pythongh-96151: Use a private name for passing builtins to dataclass.… …
61a456a 
… This now allows for a field named BUILTIN (pythongh-98143)

(cherry picked from commit 29f98b4)

Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
@bedevere-bot

bedevere-bot commented Oct 31, 2022

Copy link
Copy Markdown

GH-98900 is a backport of this pull request to the 3.11 branch.

ericvsmith pushed a commit that referenced this pull request Oct 31, 2022
@miss-islington @hauntsaninja
[3.10] gh-96151: Use a private name for passing builtins to dataclass… …
789d132 
…. This now allows for a field named BUILTIN (gh-98143) (gh-98899)

gh-96151: Use a private name for passing builtins to dataclass. This now allows for a field named BUILTIN (gh-98143)
(cherry picked from commit 29f98b4)

Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>

Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
ericvsmith pushed a commit that referenced this pull request Oct 31, 2022
@miss-islington @hauntsaninja
[3.11] gh-96151: Use a private name for passing builtins to dataclass… …
ca24e49 
…. This now allows for a field named BUILTIN (gh-98143) (gh-98900)

gh-96151: Use a private name for passing builtins to dataclass. This now allows for a field named BUILTIN (gh-98143)
(cherry picked from commit 29f98b4)

Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>

Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
@hauntsaninja hauntsaninja deleted the gh-96151 branch November 21, 2022 06:59
hauntsaninja added a commit to hauntsaninja/cpython that referenced this pull request Feb 18, 2023
@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)
@hauntsaninja hauntsaninja mentioned this pull request Feb 18, 2023
Merged
hauntsaninja added a commit that referenced this pull request Mar 25, 2023
@hauntsaninja
gh-98886: Fix issues with dataclass fields with special underscore na… …
718e866 
…mes (#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 #96151, where fixing this was welcomed in #98143 (comment)
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

@ericvsmith ericvsmith ericvsmith left review comments

@sobolevn sobolevn sobolevn left review comments

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes

Assignees

@ericvsmith ericvsmith

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@hauntsaninja @TeamSpen210 @ericvsmith @miss-islington @bedevere-bot @JelleZijlstra @sobolevn @AlexWaygood