Skip to content
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

[red-knot] Detect classes with incompatible __slots__ being used in multiple inheritance #14931

Open
AlexWaygood opened this issue Dec 12, 2024 · 4 comments · May be fixed by #15129
Open

[red-knot] Detect classes with incompatible __slots__ being used in multiple inheritance #14931

AlexWaygood opened this issue Dec 12, 2024 · 4 comments · May be fixed by #15129
Labels
red-knot Multi-file analysis & type inference

Comments

@AlexWaygood
Copy link
Member

This isn't necessarily a priority for red-knot, but it's something that we'll want to do at some point and it could be easily done now.

Two classes that both have nonempty __slots__ cannot be used in multiple inheritance. We should detect this and emit a diagnostic for it.

>>> class Foo:
...     __slots__ = ("a",)
...     
>>> class Bar:
...     __slots__ = ("b",)
...     
>>> class Baz(Foo, Bar): ...
... 
Traceback (most recent call last):
  File "<python-input-2>", line 1, in <module>
    class Baz(Foo, Bar): ...
TypeError: multiple bases have instance lay-out conflict

We'll probably want to check for this as part of this method here:

/// Iterate over all class definitions to check that the definition will not cause an exception
/// to be raised at runtime. This needs to be done after most other types in the scope have been
/// inferred, due to the fact that base classes can be deferred. If it looks like a class
/// definition is invalid in some way, issue a diagnostic.
///
/// Among the things we check for in this method are whether Python will be able to determine a
/// consistent "[method resolution order]" and [metaclass] for each class.
///
/// [method resolution order]: https://docs.python.org/3/glossary.html#term-method-resolution-order
/// [metaclass]: https://docs.python.org/3/reference/datamodel.html#metaclasses
fn check_class_definitions(&mut self) {

Notes on the semantics:

(1) A class with no __slots__ can be used in multiple inheritance with a class that has __slots__

>>> class A: ...
... 
>>> class B:
...     __slots__ = ("a",)
...     
>>> class C(A, B): ...
... 
>>> 

(2) Empty __slots__ are fine in combination with multiple inheritance:

>>> class A:
...     __slots__ = ()
...     
>>> class B:
...     __slots__ = ("a",)
...     
>>> class C(A, B): ...
... 
>>> 

(3) But even two classes with equal __slots__ will fail in multiple inheritance if they are both non-empty:

>>> class A:
...     __slots__ = ("a",)
...     
>>> class B:
...     __slots__ = ("a",)
...     
>>> class C(A, B): ...
... 
Traceback (most recent call last):
  File "<python-input-11>", line 1, in <module>
    class C(A, B): ...
TypeError: multiple bases have instance lay-out conflict

(5) And __slots__ are inherited by subclasses:

>>> class A:
...     __slots__ = ("a",)
...     
>>> class B(A): ...
... 
>>> class C:
...     __slots__ = ("c",)
...     
>>> class D(C): ...
... 
>>> class E(B, D): ...
... 
Traceback (most recent call last):
  File "<python-input-16>", line 1, in <module>
    class E(B, D): ...
TypeError: multiple bases have instance lay-out conflict
@AlexWaygood AlexWaygood added the red-knot Multi-file analysis & type inference label Dec 12, 2024
@carljm
Copy link
Contributor

carljm commented Dec 14, 2024

Perhaps the case where this comes up most is with builtin/extension types, that aren't implemented in Python and thus don't use __slots__ Python API, but do still have a bespoke instance memory layout (rather than just a dictionary of attributes), like a Python class that uses __slots__. E.g. you can't multiple-inherit int and str for this reason.

But typeshed does not explicitly apply __slots__ to types like int or str that effectively do have slots. We could potentially advocate to change this. But in the meantime, fully implementing this feature would also require adding a special case to always consider some known typeshed classes as having slots.

@AlexWaygood
Copy link
Member Author

Yeah, I think there's three distinct issues here:

  1. If two classes both have non-empty __slots__ definitions that are "visible" (i.e., we can see the __slots__ definitions in the source-code files we treat as the canonical source of information for those classes) to red-knot, and they're used in multiple inheritance, should we emit a diagnostic? I think we clearly should, because we have all the information we require to detect that an exception will definitely be raised at runtime. Lots of libraries and applications use classes with __slots__, so I think we'll catch a lot of possible bugs just by implementing this.

  2. Typeshed generally doesn't include __slots__ for any classes (even pure-Python) in its stubs. It probably should, as it would give type checkers a lot more information to work with in this domain, allowing them to catch a lot more bugs. This has been discussed previously in Consider adding __slots__ to stubs? python/typeshed#8832.

  3. As you say, you get an identical error message at runtime to the __slots__ error message if you try to multiple-inherit from various builtin classes such as str or int that CPython considers to be "solid bases", despite the fact that these classes do not have __slots__ definitions (at least, not __slots__ definitions that are visible from the Python level):

    >>> class Foo(int, str): ...
    ... 
    Traceback (most recent call last):
      File "<python-input-0>", line 1, in <module>
        class Foo(int, str): ...
    TypeError: multiple bases have instance lay-out conflict
    >>> hasattr(int, "__slots__")
    False
    >>> hasattr(str, "__slots__")
    False

    There are various possible ways we could detect this. Typeshed could pretend that they have __slots__ definitions even though they don't (at least, not in the same way as pure-Python classes that define __slots__); we could hardcode a list of builtin classes in red-knot that you can't combine in multiple inheritance; or we could work on a new standardised type-system feature that allows us to express when a class is a "solid base" that you can't use in multiple inheritance.

I think these are all quite separate questions; the only one I was attempting to tackle in this issue was question (1).

@InSyncWithFoo
Copy link
Contributor

InSyncWithFoo commented Dec 23, 2024

Some other problems to consider:

  1. Invalid

    class A:
    	__slots__ = 1    # Not iterable
    
    class B:
    	__slots__ = '1'  # Not identifier
  2. String literal

    class A:
    	__slots__ = 'abc'
    
    A().abc = 1  # This is fine
    A().a = 1    # This is not
  3. Possibly unbound

    class A:
    	if ...:
    		__slots__ = ('a', 'b')
    
    class B:
    	__slots__ = ('c', 'd')
    
    # Might or might not be valid at runtime
    class C(A, B): ...
  4. Dynamic

    t = ('a', 'b') if ... else ()
    
    class A:
    	__slots__ = t  # or `tuple(e for e in t)`
    
    class B:
    	__slots__ = ('c', 'd')
    
    # Might or might not be valid at runtime
    class C(A, B): ...
  5. Post-hoc modifications

    class A:
    	__slots__ = ['a', 'b']
    	__slots__.clear()
    
    class B:
    	__slots__ = ('c', 'd')
    
    # Valid at runtime
    class C(A, B): ...
  6. Some or all of the above mixed together

  7. Some or all of the above mixed together, in an unresolvable MRO

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Dec 23, 2024

Those are great edge cases @InSyncWithFoo! I think for an initial implementation of this check we could easily defer them all however, and only emit the incompatible-slots error for two classes in a bases list if both classes have non-dynamic slots definitions that are definitely bound. We could then open follow-up issues to expand the check and detect issues in more cases. This would make the initial PR easy for us to review and would be in keeping with the principle that we should avoid aim to avoid false positives as much as possible (even if it causes false negatives in some situations) in our initial implementation of red-knot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
red-knot Multi-file analysis & type inference
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants