Add a new declare-non-slot error code

[adamtuft]

Type of Changes

	Type
✓	✨ New feature

Description

Refs #9499

Implement declare-non-slot which reports an error when a class has a __slots__ member and a type hint on the class is not present in __slots__.

Details

Tests:

• Add test case which should report the error code.

• Add test case where the error is suppressed due to __dict__ in __slots__.

• Add test to detect annotation not in any base class's __slots__.

• Add test against false-positive when base doesn't have __slots__ (since there is still then an instance __dict__ thanks to base)

• Add test against false-positive when base has __slots__ with __dict__ entry.

• Ignore declare-non-slots for regression_5479.py which is for assigning-non-slot.

Implementation:

Implement check for declare-non-slot in class_checker.py and 2 new helper methods:

• Add method _check_declare_non_slot to report declare-non-slot error. This method checks node and all bases for a valid __slots__, gathering the names in all __slots__ found. If node has an annotation not in any __slots__, then declare-non-slot is reported.

• Add helper method _has_valid_slots which returns True if a valid __slots__ is found on a ClassDef (re-use logic from _check_slots).

• Refactor _check_redefined_slots to split out logic for getting __slots__ names into _get_classdef_slots_names helper, since the same logic can be used by both functions.

Some thoughts:

• 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.

To Do:

• Implement missing-slot-annotation from Check "__slots__" and "@dataclass" attributes #9499
• Add documentation (good/bad code examples)
• Add changelog/towncrier entry

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.84%. Comparing base (afd5edf) to head (05e0e14).
Report is 141 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9564      +/-   ##
==========================================
+ Coverage   95.83%   95.84%   +0.01%     
==========================================
  Files         174      174              
  Lines       18812    18862      +50     
==========================================
+ Hits        18028    18078      +50     
  Misses        784      784              

Files with missing lines	Coverage Δ
pylint/checkers/classes/class_checker.py	93.69% <100.00%> (+0.32%)	⬆️

[adamtuft]

🤖 Effect of this PR on checked open source code: 🤖

Effect on pandas:
The following messages are now emitted:

1. declare-non-slot:
   No such name 'values' in slots
   https://github.com/pandas-dev/pandas/blob/bb0fcc23eed9f6a1a6506c6e27b98fb397ce747e/pandas/core/internals/blocks.py#L2097
2. declare-non-slot:
   No such name 'values' in slots
   https://github.com/pandas-dev/pandas/blob/bb0fcc23eed9f6a1a6506c6e27b98fb397ce747e/pandas/core/internals/blocks.py#L2147

This comment was generated for commit 245be58

Based on these messages, the error should probably exclude any annotation that looks like a descriptor. I'm assuming that's how such members are implemented but I'll need to look into it.

EDIT: upon further reading, an empty __slots__ is actually required when multiple inheritance is involved [link]. I suspect this is what is happening here.

[Pierre-Sassoulas]

Looks great already, thank you. I don't have the time to really review the change at the moment but regarding the CI fail: You can create an example in the doc (see for example bad-chained-comparison : https://github.com/pylint-dev/pylint/tree/main/doc/data/messages/b/bad-chained-comparison) and a changelog entry for towncrier (Some doc here, but you can copy paste an existing fragment of course: https://pylint.readthedocs.io/en/stable/development_guide/contributor_guide/contribute.html#creating-a-pull-request)

[DanielNoord]

Left some comments. Code looks really good, thanks for opening the PR. I intended to have a look at your fork, but this is much easier as I can make in-line comments. Nice job on your first PR!

[adamtuft]

Thanks very much for the feedback! I'll incorporate suggestions when I have a little more time, hopefully in the next week or so.

[adamtuft]

Thanks so much for the review comments! I've reflected on my implementation and I think it needs a bit more work 🤔 - just to share a few thoughts:

• Could we just return early if _get_classdef_slots_names returns []? Add a new declare-non-slot error code #9564 (comment)
• What to do about empty __slots__, which is required if the class takes part in multiple inheritance [link] (I suspect this is the cause of the messages in this comment).
• Make it clearer under what conditions the check should return early, and whether to be permissive or conservative when trying to gather names from __slots__ across base classes (I don't want to cause lots of false-positives!)
• Given these challenges, should this message be restricted just to classes decorated with @dataclass as suggested in the original issue Check "__slots__" and "@dataclass" attributes #9499

[adamtuft]

Re. reporting an error for an empty __slots__, I think the best thing to do in this case is abort the check. An empty __slots__ is required if a class takes part in multiple inheritance with other classes that may use __slots__, so it is probably best to assume this is the intention if a class has an empty __slots__. This will prevent false positives like those seen above in the internals of Pandas.

[DanielNoord]

Just letting you know that I do want to review this but haven't found the time. I hope to get to this in the next 7 days.

[DanielNoord]

I'm on mobile but this seems very similar to _check_slots. I'm a bit worried that we're duplicating checks and run the risk of code drift.

Have you considered refactoring the other method to serve both purposes?

[adamtuft]

I agree that _check_slots and _has_valid_slots are similar and that it would be best not to duplicate these checks. Refactoring is complicated by the fact that _check_slots is doing a few jobs at the same time:

• checking whether __slots__ is valid
• if not, add 1 of 2 possible messages based on the reason why it isn't valid
• If __slots__ looks valid, get the slot items and apply 2 further checks.

The difference between _check_slots and _has_valid_slots are that _check_slots cares about the reason why __slots__ isn't valid, whereas _has_valid_slots only detects whether __slots__ is valid. It might be wise to introduce a method that serves both purposes e.g. _validate_slots that returns an Enum that either reports a valid __slots__ or says why it is invalid. This would allow you to separate the multiple concerns of the _check_slots method a little more clearly.

The question is whether refactoring _check_slots is within the scope of this MR. To me that feels like quite a big refactor that should be its own MR. I would propose that if the present MR is accepted an issue should then be opened to highlight the need to refactor these slot checks to reduce the duplication. I'd be happy to work on that issue as a separate MR.

[DanielNoord]

Thanks, I had another good look and think this is good to go.

@Pierre-Sassoulas do you want to do a second review? Or shall we just merge this? :)

[Pierre-Sassoulas]

Just a nit if it's not too much work to fully cover the new code. This looks amazing, a great new check, thank you @adamtuft

[Pierre-Sassoulas]

We have self.a = 42 line 183 in the functional test, maybe getting coverage for this is as simple as adding a line with self.b : str = "AnnAssign.value is not None" too ?

[adamtuft]

Thanks, I've added this, hopefully this will fix the code coverage complaint. Waiting for the CI checks to run now.

[Pierre-Sassoulas]

There might be specific condition to enter this condition, (a "b" value in slot maybe ?)

[adamtuft]

The codecov test result seems to have changed now, it's claiming that a line isn't covered which (surely?) must be covered by the tests which generate the new message. I don't understand why the codecov test is failing, are you able to shed any light on this please?

[Pierre-Sassoulas]

I tried something with b: str being declared. I assume the diff between main and this branch got too big. Also fixed the doc generation.

[adamtuft]

Thank you both for the reviews again! I don't understand why the codecov check is still failing... surely the highlighted line is covered by tests since it has to be executed for the tests which generate the new error?

[Pierre-Sassoulas]

Added this line to cover everything. Do we agree it shouldn't raised @adamtuft ?

[adamtuft]

Thanks, I see now that I added that annotation to a test which doesn't report the message, so it wasn't covering the required line.

Yes, looks good, that doesn't raise the error 👍 thank you for your help!

[github-actions]

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 05e0e14

[Pierre-Sassoulas]

Great first contribution @adamtuft, congratulation on becoming a pylint contributor ! This will be released in 3.3.0 :)

[adamtuft]

Thank you very much @Pierre-Sassoulas and @DanielNoord for your help and feedback on this contribution! Very happy to have a contribution accepted 🥳