Skip to content

gh-89529: disallow default_factory for fields in dataclasses without __init__ #123070

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.

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

picnixz
Copy link
Member

@picnixz picnixz commented Aug 16, 2024

This is a proposal for rejecting default_factory on fields if the dataclass does not have an __init__ method.

@picnixz
Copy link
Member Author

picnixz commented Aug 17, 2024

Should we consider this a bugfix or a feature actually? (to determine whether it requires backports or not, and update the issue's labels, which I tagged as 3.12, 3.13 and 3.14 + bug).

@picnixz
Copy link
Member Author

picnixz commented Oct 25, 2024

Friendly ping @ericvsmith

@picnixz picnixz requested a review from sobolevn November 16, 2024 09:00
Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

Technically, this is a breaking change. For example, it can break abstract dataclasses, which do not have __init__ on purpose. And their subclasses have __init__s.

But, I also see that this bug can indeed happen.

So, I am not sure what is best.

@picnixz
Copy link
Member Author

picnixz commented Nov 16, 2024

For example, it can break abstract dataclasses, which do not have init on purpose. And their subclasses have __init__s

That's something I entirely missed. Maybe we can work out by checking if the class is abstract (namely, declared using abc.ABC or metaclass=abc.ABCMeta) and do not raise in this case? Otherwise we can drop this behaviour.

Anyway, this solves one of my question about whether this is a bug fix or a feature. Since it could break something, whether we want it or not, I'd prefer tagging it as a feature rather than a bug fix.

@picnixz
Copy link
Member Author

picnixz commented Mar 30, 2025

@ericvsmith friendly ping

@picnixz
Copy link
Member Author

picnixz commented Apr 25, 2025

I don't think I'll land this one because of this:

For example, it can break abstract dataclasses, which do not have __init__ on purpose. And their subclasses have __init__s.

To be clear, it makes sense to indeed have an error but it's hard to know whether the error should be pre-caught as there are cases where it can be said that it would be unreachable (namely, we expect concrete classes to have their own __init__ and deal with what they need to I guess?).

So I'll mark this one as stale and a draft as I need to add tests for abstract classes as well..

@picnixz picnixz marked this pull request as draft April 25, 2025 08:55
@picnixz picnixz added awaiting review stale Stale PR or inactive for long period of time. labels Apr 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review stale Stale PR or inactive for long period of time. topic-dataclasses
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants