dataclasses: make it an error to have initialized non-fields in a dataclass

[ericvsmith]

BPO	32428
Nosy	@gvanrossum, @ericvsmith, @ilevkivskyi

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/ericvsmith'
closed_at = <Date 2018-01-06.22:19:35.688>
created_at = <Date 2017-12-26.18:34:08.781>
labels = ['3.7', 'type-bug', 'library']
title = 'dataclasses: make it an error to have initialized non-fields in a dataclass'
updated_at = <Date 2018-01-07.00:17:53.053>
user = 'https://github.com/ericvsmith'

bugs.python.org fields:

activity = <Date 2018-01-07.00:17:53.053>
actor = 'eric.smith'
assignee = 'eric.smith'
closed = True
closed_date = <Date 2018-01-06.22:19:35.688>
closer = 'eric.smith'
components = ['Library (Lib)']
creation = <Date 2017-12-26.18:34:08.781>
creator = 'eric.smith'
dependencies = []
files = []
hgrepos = []
issue_num = 32428
keywords = []
message_count = 12.0
messages = ['309065', '309071', '309075', '309112', '309189', '309416', '309417', '309442', '309443', '309584', '309590', '309591']
nosy_count = 3.0
nosy_names = ['gvanrossum', 'eric.smith', 'levkivskyi']
pr_nums = []
priority = 'normal'
resolution = 'wont fix'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue32428'
versions = ['Python 3.7']

[ericvsmith]

For this class:

@dataclass
class C:
   x: int = 0
   y = 0

Generate a TypeError. 'y' is not a field (as defined by @DataClass). It should either be a regular field (by giving it a type annotation) or a ClassVar field.

[ilevkivskyi]

A possible question here is should we give an error for any non-callable name in __dict__ which is not in __annotations__ or only for Fields?

After some thinking I am actually leaning towards the first option.

[ericvsmith]

I'm not sure I understand the distinction. You have to look through everything in __dict__, then exclude those things that are any type of field (so, real fields or pseudo-fields). Those are the things that are in __annotations__, anyway.

The trick is what else to exclude. In this class:

class C:
    x: int = 0
    y = 0

    def func(self): pass

    @staticmethod
    def staticmeth() : pass

    @classmethod
    def classmeth(cls) : pass

    @property
    def prop(self): pass

These are the non-callables:

print([k for k in C.__dict__ if not callable(getattr(C, k))])

['__module__', '__annotations__', 'x', 'y', 'prop', '__dict__', '__weakref__', '__doc__']

How do we only pick out y and probably prop, and ignore the rest, without being overly fragile to new things being added? I guess ignoring dunders and things in __annotations__. Is that close enough?

[ilevkivskyi]

I'm not sure I understand the distinction.

Initially I thought about only flagging code like this:

@dataclass
class C:
    x = field()

But not this:

@dataclass
class C:
    x = 42

Now I think we should probably flag both as errors.

How do we only pick out y and probably prop, and ignore the rest, without being overly fragile to new things being added? I guess ignoring dunders and things in __annotations__. Is that close enough?

We had a similar problem while developing Protocol class (PEP-544). Currently we just a have a whitelist of names that are skipped:

'__abstractmethods__', '__annotations__', '__weakref__', '__dict__',
'__slots__', '__doc__', '__module__'

(plus some internal typing API names)

[gvanrossum]

I liked the original design better, where things without annotations would
just be ignored. What changed?

On Dec 27, 2017 5:19 PM, "Ivan Levkivskyi" <report@bugs.python.org> wrote:

Ivan Levkivskyi levkivskyi@gmail.com added the comment:

I'm not sure I understand the distinction.

Initially I thought about only flagging code like this:

@DataClass
class C:
x = field()

But not this:

@DataClass
class C:
x = 42

Now I think we should probably flag both as errors.

How do we only pick out y and probably prop, and ignore the rest,
without being overly fragile to new things being added? I guess ignoring
dunders and things in __annotations__. Is that close enough?

We had a similar problem while developing Protocol class (PEP-544).
Currently we just a have a whitelist of names that are skipped:

'abstractmethods', 'annotations', 'weakref', 'dict',
'slots', 'doc', 'module'

(plus some internal typing API names)

----------

---

Python tracker <report@bugs.python.org>
<https://bugs.python.org/issue32428\>

---

[ilevkivskyi]

I liked the original design better, where things without annotations would just be ignored. What changed?

With the original proposal the ignored variables without annotations will behave as class variables. This "conflicts" with PEP-526 which requires class variables to be annotated with ClassVar[...]. On the other hand some people may be unhappy that they need to import typing to define a class variable in a dataclass. So this is a convenience vs consistence question. I am more in favour of consistence here, but only +0.

[ilevkivskyi]

Just to clarify the previous comment, I still think that flagging this

@dataclass
class C:
    x = field()

is important, since simply ignoring a field() will be too confusing (especially for attrs users).

The previous comment is about

@dataclass
class C:
    x = 42

[gvanrossum]

> I liked the original design better, where things without annotations would just be ignored. What changed?

With the original proposal the ignored variables without annotations will behave as class variables. This "conflicts" with PEP-526 which requires class variables to be annotated with ClassVar[...]. On the other hand some people may be unhappy that they need to import typing to define a class variable in a dataclass. So this is a convenience vs consistence question. I am more in favour of consistence here, but only +0.

There is no real conflict with PEP-526 though. PEP-526 introduces ClassVar so the type checker can be made to understand. PEP-557 allows omitting ClassVar in case you don't care about type checkers. So I think we should stick with the current spec of PEP-557 (which lets you omit ClassVar), except for this special case:

I still think that flagging this

@DataClass
class C:
x = field()

is important, since simply ignoring a field() will be too confusing (especially for attrs users).

Agreed. That's a special case and I'm fine with flagging it as an error.

[ilevkivskyi]

There is no real conflict with PEP-526 though. PEP-526 introduces ClassVar so the type checker can be made to understand. PEP-557 allows omitting ClassVar in case you don't care about type checkers. So I think we should stick with the current spec of PEP-557 (which lets you omit ClassVar), except for this special case: ...

OK.

[ericvsmith]

I'm closing this, and will open another issue to raise an error for:

@dataclass
class C:
    x = field()

[ilevkivskyi]

@eric

I'm closing this, and will open another issue to raise an error for: ...

I think we also need a separate issue for not overriding __repr__ etc, if '__repr__' in cls.__dict__.

[ericvsmith]

Correct. I'm working on that one now. I'll open it tonight or tomorrow.