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

Check "__slots__" and "@dataclass" attributes #9499

Open
buhtz opened this issue Mar 13, 2024 · 9 comments
Open

Check "__slots__" and "@dataclass" attributes #9499

buhtz opened this issue Mar 13, 2024 · 9 comments
Assignees
Labels
Enhancement ✨ Improvement to a component Needs decision 🔒 Needs a decision before implemention or rejection

Comments

@buhtz
Copy link

buhtz commented Mar 13, 2024

Current problem

The following is valid code but implicates some problems.

from dataclasses import dataclass

@dataclass
class MyClass:
    __slots__ = ('foo', 'bar', )
    foo: str

    def __init__(self):
        self.foo = 'one'
        self.bar = 'two'

There are two slot variables (foo & bar) but only foo is handled by the @dataclass. For example str(MyClass()) will result in MyClass(foo="one"). The variable bar is missing in that output no matter that it exists.

Desired solution

PyLint should warn about that.

The list of variables in __slot__ do not fit to the list specificied for @dataclass.

Python 3.10 or higher to offer @dataclass(slots=True) to prevent situations like this. So this could be one possible solution PyLint could suggest. But there are also older Python versions still relevant.

Additional context

No response

@buhtz buhtz added the Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling label Mar 13, 2024
@Pierre-Sassoulas Pierre-Sassoulas added Enhancement ✨ Improvement to a component Needs specification 🔐 Accepted as a potential improvement, and needs to specify edge cases, message names, etc. and removed Needs triage 📥 Just created, needs acknowledgment, triage, and proper labelling labels Mar 18, 2024
@Pierre-Sassoulas
Copy link
Member

Thank you for opening the issue, how would you name this new message ?

There is already some slot related messages in the classe checker (https://github.com/pylint-dev/pylint/blob/main/pylint/checkers/classes/class_checker.py#L665), but it seems it could go in the data classes checker (https://github.com/pylint-dev/pylint/blob/main/pylint/checkers/dataclass_checker.py)

@buhtz
Copy link
Author

buhtz commented Mar 18, 2024

Thanks for asking.

how would you name this new message ?

I wonder that there still is an assigning-non-slot. Regarding that existing name I would suggest using declare-non-slot (or define-non-slot). I can't diffrenciate between declare and define.

For the docs and based on assigning-non-slot:

Problematic code

class Student:
    __slots__ = ("name",)

    name: str
    surname: str  # [define-non-slot]

Correct code:

class Student:
    __slots__ = ("name", "surname")

    name: str
    surname: str

@Pierre-Sassoulas Pierre-Sassoulas added Needs PR This issue is accepted, sufficiently specified and now needs an implementation and removed Needs specification 🔐 Accepted as a potential improvement, and needs to specify edge cases, message names, etc. labels Mar 18, 2024
@adamtuft
Copy link
Contributor

Hi there! I'm looking for a way to contribute and I'd like to try implementing this feature.

As well as the [define-non-slot] case I can see another case that you might want to warn on:

Problematic code:

class Student:
    __slots__ = ("name", "age") # [missing-slot-annotation]

    name: str

Correct code:

class Student:
    __slots__ = ("name", "age")

    name: str
    age: int

I'd consider this merely a warning rather than an error as this code will still run with the dataclass decorator, whereas it won't in the [define-non-slot] case.

These checks shouldn't be evaluated if __slots__ contains __dict__.

@DanielNoord
Copy link
Collaborator

Sounds like a useful warning to me!

@adamtuft
Copy link
Contributor

I have an implementation for declare-non-slot at https://github.com/adamtuft/pylint/tree/9499-slots-and-class-annotations, would be very grateful for comments & discussion! I will also implement missing-slot-annotation when I have a little more time.

A few points:

  • I thought this check belonged in the class checker instead of the dataclass checker since the problematic code is problematic even without the dataclass decorator. There may be other cases where the class annotations and __slots__ items should match, and making this check dataclass-specific would miss those cases.
  • I went with declare-non-slot over define-non-slot as annotating a class feels more like a declaration that such a member exists, rather than a definition of that member.
  • I made some changes to existing code to factor out some logic into a helper function _get_classdef_slots_names as this seemed the cleanest way to re-use existing logic. I hope this is ok.
  • I've checked that the existing tests still pass and have run the primer tests with python3 -m pytest -m primer_stdlib --primer-stdlib.

@Pierre-Sassoulas
Copy link
Member

Hey @adamtuft, thanks a lot for working on this ! Do you mind opening a pull request directly from your branch ? (Use "Refs #9499" in the description :) )

@Pierre-Sassoulas
Copy link
Member

@adamtuft do you think this can be closed following the merge of the new checker ?

@adamtuft
Copy link
Contributor

adamtuft commented Jun 6, 2024

@adamtuft do you think this can be closed following the merge of the new checker ?

The new checker covers the original issue, but there was also some discussion above of a missing-slot-annotation warning. Would you still want this? It could be implemented later, referencing this (closed) issue.

@Pierre-Sassoulas Pierre-Sassoulas removed this from the 3.3.0 milestone Jun 6, 2024
@Pierre-Sassoulas Pierre-Sassoulas added Needs decision 🔒 Needs a decision before implemention or rejection and removed Needs PR This issue is accepted, sufficiently specified and now needs an implementation labels Jun 6, 2024
@Pierre-Sassoulas
Copy link
Member

Let's keep open for discussion about missing-slot-annotation then. I'm +1 myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement ✨ Improvement to a component Needs decision 🔒 Needs a decision before implemention or rejection
Projects
None yet
Development

No branches or pull requests

4 participants