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

SLOT: Classes with multiple base classes #15879

Open
Skylion007 opened this issue Feb 2, 2025 · 3 comments
Open

SLOT: Classes with multiple base classes #15879

Skylion007 opened this issue Feb 2, 2025 · 3 comments
Labels
rule Implementing or modifying a lint rule type-inference Requires more advanced type inference.

Comments

@Skylion007
Copy link
Contributor

Skylion007 commented Feb 2, 2025

Description

ruff check --select=SLOT .
ruff --version 0.8.4
class Backend(RandomClass, tuple):
   pass

^ suggests a fixit here. This is the recommended class structure for old versions of python before strenum was introduced. However, enum doesn't have slots defined, so defining a __slots__ wouldn't be that helpful. i also see this bug happen when a class has multiple ancestors, one of which is a tuple.

Actually, nevermind, this error was actually masking another error where enum was not properly imported. Seems odd to suggest a fixit in this situation. In other cases though, I do see the error pop up when only one of the ancestors is a tuple.

@Skylion007 Skylion007 changed the title SLOT lint false positive on classes with multiple ancestors SLOT lint false positive on classes with multiple ancestors for strenum Feb 2, 2025
@Skylion007 Skylion007 changed the title SLOT lint false positive on classes with multiple ancestors for strenum SLOT lint false positive on classes where one ancestor is not defined Feb 2, 2025
@MichaReiser
Copy link
Member

Could you explain what's the bug experience? I don't think I understand it from the issue.

Ruff does raise an error about a missing __slots__ even if the base class is known. So warning in the case if any base class is unknown doesn't seem incorrect, because the __slots__ attribute is always required if any parent is tuple.

Playground: https://play.ruff.rs/45b205ae-153c-4a73-8fe6-29995ef0a5c9

@MichaReiser MichaReiser added the needs-info More information is needed from the issue author label Feb 3, 2025
@Skylion007
Copy link
Contributor Author

Skylion007 commented Feb 3, 2025

@MichaReiser To clarify, what is the behavior if a class has two ancestors, and only one of them implements __slots__? My understanding is that the slots in the class itself won't have an effect because it will still need to consult the __dict__ of the first parent, but I could be wrong? Specifically, what happens here:
https://play.ruff.rs/26820797-8f34-4fc4-8c13-eb81bce03d0e

@MichaReiser
Copy link
Member

Oh, I see. That's a good point. In that case, I'd say it's not a false positive, at least not when the base class is under the user's control. However, the suggested fix/message is misleading because only adding __slots__ to the current class isn't enough; it also needs to be added to all non-tuple parent classes.

Now, analyzing base classes is something that Ruff struggles with, at least if they're defined in another file. Which is why I'm leaning towards leaving the rule as is for now and revisit it once we have better type inference.

@MichaReiser MichaReiser added rule Implementing or modifying a lint rule type-inference Requires more advanced type inference. and removed needs-info More information is needed from the issue author labels Feb 4, 2025
@MichaReiser MichaReiser changed the title SLOT lint false positive on classes where one ancestor is not defined SLOT: Classes with multiple base classes Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule Implementing or modifying a lint rule type-inference Requires more advanced type inference.
Projects
None yet
Development

No branches or pull requests

2 participants