This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: dataclasses: make it an error to have initialized non-fields in a dataclass
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.7
process
Status: closed Resolution: wont fix
Dependencies: Superseder:
Assigned To: eric.smith Nosy List: eric.smith, gvanrossum, levkivskyi
Priority: normal Keywords:

Created on 2017-12-26 18:34 by eric.smith, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Messages (12)
msg309065 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2017-12-26 18:34
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.
msg309071 - (view) Author: Ivan Levkivskyi (levkivskyi) * (Python committer) Date: 2017-12-26 23:24
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 `Field`s?

After some thinking I am actually leaning towards the first option.
msg309075 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2017-12-27 02:09
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?
msg309112 - (view) Author: Ivan Levkivskyi (levkivskyi) * (Python committer) Date: 2017-12-28 00:19
> 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)
msg309189 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2017-12-29 19:20
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>
> _______________________________________
>
msg309416 - (view) Author: Ivan Levkivskyi (levkivskyi) * (Python committer) Date: 2018-01-03 16:49
> 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.
msg309417 - (view) Author: Ivan Levkivskyi (levkivskyi) * (Python committer) Date: 2018-01-03 16:52
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
msg309442 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-01-03 22:06
> > 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.
msg309443 - (view) Author: Ivan Levkivskyi (levkivskyi) * (Python committer) Date: 2018-01-03 22:09
> 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.
msg309584 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2018-01-06 22:19
I'm closing this, and will open another issue to raise an error for:

@dataclass
class C:
    x = field()
msg309590 - (view) Author: Ivan Levkivskyi (levkivskyi) * (Python committer) Date: 2018-01-07 00:14
@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__.
msg309591 - (view) Author: Eric V. Smith (eric.smith) * (Python committer) Date: 2018-01-07 00:17
Correct. I'm working on that one now. I'll open it tonight or tomorrow.
History
Date User Action Args
2022-04-11 14:58:56adminsetgithub: 76609
2018-01-07 00:17:53eric.smithsetmessages: + msg309591
2018-01-07 00:14:29levkivskyisetmessages: + msg309590
2018-01-06 22:19:35eric.smithsetstatus: open -> closed
resolution: wont fix
messages: + msg309584

stage: resolved
2018-01-03 22:09:44levkivskyisetmessages: + msg309443
2018-01-03 22:06:53gvanrossumsetmessages: + msg309442
2018-01-03 16:52:50levkivskyisetmessages: + msg309417
2018-01-03 16:49:41levkivskyisetmessages: + msg309416
2017-12-29 19:20:49gvanrossumsetmessages: + msg309189
2017-12-28 00:19:31levkivskyisetmessages: + msg309112
2017-12-27 02:09:29eric.smithsetmessages: + msg309075
2017-12-26 23:24:43levkivskyisetnosy: + gvanrossum, levkivskyi
messages: + msg309071
2017-12-26 18:34:08eric.smithcreate