Adds support for __slots__ assignment, refs #10801

[sobolevn]

Description

We can now detect assignment that are not matching defined __slots__.

Example:

class A:
   __slots__ = ('a',)

class B(A):
   __slots__ = ('b',)
   def __init__(self) -> None:
       self.a = 1  # ok
       self.b = 2  # ok
       self.c = 3  # error

b: B
reveal_type(b.c)

In runtime it produces:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 6, in __init__
AttributeError: 'B' object has no attribute 'c'

Closes #10801

Test Plan

• I need to test that single class with __slots__ works
• I need to test that slotted class with __slots__ in super classes works
• I need to test that a class can extend slotted class without extra __slots__ definition
• I need to test that assignment of wrong __slots__ raise an error
• I need to test that __slots__ with wrong elements raise an error
• I need to test that __slots__ with dynamic elements are ignored
• I need to test that __slots__ = 1 outside of class definition context should be fine
• I need to test __dict__ corner case

Future steps

• I can also modify attrs plugin to include slots if @attr.s(slots=True) is passed

[sobolevn]

Improtant special case: __slots__ = ('__dict__',) which should be supported.

[sobolevn]

I am not sure why this fails: https://app.travis-ci.com/github/python/mypy/jobs/526676991

_____________________ testOverloadLambdaUnpackingInference _____________________

[gw0] linux -- Python 3.10.0 /home/travis/build/python/mypy/.tox/py/bin/python

data: /home/travis/build/python/mypy/test-data/unit/check-overloading.test:4002:

SystemExit: 1

----------------------------- Captured stderr call -----------------------------

The typed_ast package is not installed.

For Python 2 support, install mypy using `python3 -m pip install "mypy[python2]"`Alternatively, you can install typed_ast with `python3 -m pip install typed-ast`.

It does not look related.

[sobolevn]

@hauntsaninja @JelleZijlstra friendly ping. Any chance to get this PR reviewed? 🙂

[A5rocks]

Just a while ago I came across this behaviour of ignoring __slots__ and it didn't seem very intuitive, thanks for doing this!

[sobolevn]

@ilevkivskyi @JukkaL Hi! Any chance to push this forward? 🙂

[emmatyping]

I think this looks pretty reasonable overall. Just a couple of questions/comments

[emmatyping]

If a class has a mix of slots and non-slots members, wouldn't this report an error incorrectly?

E.g.

class A:
    __slots__ = ('a',)
    b = 4

Edit: ah based on the tests it looks like this does not report an error incorrectly

[sobolevn]

I will add a test case for this, thank you. It actually was not consistent with Python's runtime behavior:

class A:
    __slots__ = ('a',)
    b = 4

    def __init__(self) -> None:
        self.a = 1
        self.b = 2  # error here in runtime

A()
# AttributeError: 'A' object attribute 'b' is read-only

I will change the code to make this an error with mypy as well.

[emmatyping]

Isn't this an error if B didn't inherit from another class? It seems weird to me this would be accepted since it isn't in the __slots__ for B nor in the mro.

[sobolevn]

This is how Python works in runtime. If any base class has __dict__ that the child class itself would have __dict__. So, when using B(A) you will implicitly have __dict__ in place. Try this:

class A:
    pass  # no slots

class B(A):
    __slots__ = ("a", "b")

    def __init__(self) -> None:
        self.a = 1
        self.b = 2
        self.missing = 3

b = B()
b.a = 1
b.b = 2
b.missing = 3
b.extra = 4  # E: "B" has no attribute "extra"

But, without A super class it would be different:

class B:
    __slots__ = ("a", "b")

    def __init__(self) -> None:
        self.a = 1
        self.b = 2
        self.missing = 3  # AttributeError: 'B' object has no attribute 'missing'

b = B()

[sobolevn]

I think, that I need to add this to the docs of mypy. Do you agree?
What would be the best place for it? Common issues?

[emmatyping]

Aha, I see, that makes more sense. I would probably add it to class basics: https://mypy.readthedocs.io/en/stable/class_basics.html#class-basics

[emmatyping]

Based on the mypy_primer output, it seems there are also a couple of cases that should be handled:

• __slots__ is Any or list[Any]
• the rvalue is tuple() or similar for dict, list, etc.

It also seems to report a few true errors :)

[sobolevn]

slots is Any or list[Any]

Like just __slots__ = returns_any()?

the rvalue is tuple() or similar for dict, list, etc.

Will do! 👍
For now, I will just ignore cases like list(('a', 'b', 'c')), because it requires quite a lot of code for extra inference.
We can add this later. Moreover, __slots__ = list(('a', 'b', 'c')) is not quite common thing to write, people usually just write __slots__ = ('a', 'b').

But, tests will be added with no errors reported.

[sobolevn]

Ok, this reports too many false positives. I will fix that + add more tests for @property.setter

[sobolevn]

One more important case I've missed: __setattr__ support. Adding tests for it as well.

This code works in runtime:

class F:
    __slots__ = ('a',)
    def __init__(self) -> None:
        self.a = {}
    def __setattr__(self, k, v) -> None:
        if k != 'a':
            self.a[k] = v
        else:
            F.__dict__[k].__set__(self, v)

f = F()
f.extra = 2
print(f.a)

https://stackoverflow.com/questions/19566419/can-setattr-can-be-defined-in-a-class-with-slots

[sobolevn]

Now, let's look on cases highlighted by mypy_primer:

1. dulwich. Seems like a bug, I've opened a PR: Fixes Tag.signature field jelmer/dulwich#901 Nevermind, this was a false positive with using a = custom_property(get_a, set_a). Tests for this case are added
2. parso. It looks valid: https://github.com/davidhalter/parso/blob/master/parso/tree.py#L374-L385 NodeOrLeaf does not have parent field, that's true. I've opened a PR: Fixes __slots__ definition in NodeOrLeaf davidhalter/parso#199
3. bidict. Looks like a false-positive. It uses nxt = property(a, b). I've missed this form in tests https://github.com/jab/bidict/blob/56ccfad1d9577c39828f43f8d29f39ccc0bdbb62/bidict/_orderedbase.py#L83 I will fix it today.

[emmatyping]

@sobolevn wow that's great progress in the mypy_primer output, thank you!

[sobolevn]

Looks like that mypy_primer now detects a single correct bug from #10864 (comment) 🎉 🎉

[github-actions]

Diff from mypy_primer, showing the effect of this PR on open source code:

parso (https://github.com/davidhalter/parso.git)
+ parso/tree.py:385: error: Trying to assign name "parent" that is not in "__slots__" of type "parso.tree.NodeOrLeaf"

[sobolevn]

@ethanhs any other corner-cases you can think of? 🤔

[emmatyping]

@ethanhs any other corner-cases you can think of? 🤔

Not really, this seems to cover things pretty well!

Do you want to add the docs change in a separate PR?

[sobolevn]

Not really, this seems to cover things pretty well!

Anyways, I am here to help / fix any problems.
The last question: should we hide this behind a configuration flag?

Do you want to add the docs change in a separate PR?

@ethanhs yes, I need to think about good places to fit it 👍

[emmatyping]

The last question: should we hide this behind a configuration flag?

I don't think it should be. It seems to be a useful check and with the most recent changes I don't think it should raise too many false positives, and we have so many options already...

[sobolevn]

@ethanhs agreed. Anything else I need to do to get this merged? 🙂

[emmatyping]

Thanks!

[sobolevn]

Thanks, @ethanhs! I will submit a PR with the docs today 🙂