[ruff] Mark RUF023 fix as unsafe if __slots__ is not a set and the binding is used elsewhere

[AlexWaygood]

Fixes #12653. This required a little bit of refactoring: in order to know whether the __slots__ binding is used or not, the rule must run after the AST tree has been visited in its entirety; that meant that the rule's hook had to be moved out of statements.rs and into bindings.rs. This, in turn, meant that the function implementing the rule had to accept a Binding rather than two AST nodes, and had to return an Option<Diagnostic> rather than ().

[github-actions]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -0 violations, +0 -6 fixes in 1 projects; 53 projects unchanged)

DisnakeDev/disnake (+0 -0 violations, +0 -6 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview

- disnake/components.py:189:34: RUF023 [*] `Button.__slots__` is not sorted
+ disnake/components.py:189:34: RUF023 `Button.__slots__` is not sorted
- disnake/components.py:269:34: RUF023 [*] `BaseSelectMenu.__slots__` is not sorted
+ disnake/components.py:269:34: RUF023 `BaseSelectMenu.__slots__` is not sorted
- disnake/components.py:646:34: RUF023 [*] `TextInput.__slots__` is not sorted
+ disnake/components.py:646:34: RUF023 `TextInput.__slots__` is not sorted

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
RUF023	6	0	0	0	6

[dscorbett]

I don’t think it is possible to know that the __slots__ binding is not used, because it could be used outside the module, or even outside the whole project. The documentation doesn’t explicitly say whether this is supported or whether __slots__ is meant to be treated as private, but it does say:

If a dictionary is used to assign __slots__, the dictionary keys will be used as the slot names. The values of the dictionary can be used to provide per-attribute docstrings that will be recognised by inspect.getdoc() and displayed in the output of help().

which implies that __slots__ is meant to be accessible by other modules (e.g. inspect) so it is not enough to check for usages within the same module.

[AlexWaygood]

Yeah -- I agree with all that @dscorbett. And by the same logic, we could also argue that the fix for RUF022 should always be marked as unsafe.

I do however feel like it's pretty uncommon to iterate over the __slots__ of a class from an external module. I agree that __slots__ is in a grey area where it's not really private API -- but practically speaking, I think the vast majority of library users think of the presence of __slots__ on a class as an optimisation/implementation detail, and the order of the items in __slots__ even more so.

Overall I feel like this is a good compromise that reflects the fact that for arguably 99% of cases, this fix is safe if we can see that a class's __slots__ are not read in the file in which that class is defined.

The documentation doesn’t explicitly say whether this is supported or whether __slots__ is meant to be treated as private, but it does say:

If a dictionary is used to assign slots, the dictionary keys will be used as the slot names. The values of the dictionary can be used to provide per-attribute docstrings that will be recognised by inspect.getdoc() and displayed in the output of help().

FWIW, I wrote that bullet point in the documentation ;-)

[AlexWaygood]

To be clear: you may be right. This isn't an opinion I'm certain in; possibly this rule (and RUF022 as well?) should just have their autofixes marked as unsafe in nearly all instances. I'm interested to hear what other maintainers think here. (@carljm, maybe?)

[carljm]

I don't think it's that hard to imagine a scenario where __slots__ are iterated from outside the module, e.g. I can totally imagine some framework/ORM doing something like that (though I wouldn't recommend it.)

I also generally feel like we have unsafe fixes for a reason, and we should err on the side of marking fixes unsafe if there's a plausible scenario where they could break someone's code. ("Plausible" is of course doing a lot of heavy lifting there, and leaves a lot of room for debate.)

I would probably mark this fix as unsafe, unless __slots__ are defined as a set. But it's quite possible I'm too conservative on this. We could also go with the approach in this PR and wait to see if anyone ever files an issue for this rule breaking their code due to introspection of __slots__ from another module.

[AlexWaygood]

I also generally feel like we have unsafe fixes for a reason, and we should err on the side of marking fixes unsafe if there's a plausible scenario where they could break someone's code.

agreed -- but there is also a cost to marking a fix as unsafe. It means users don't get the autofix unless they either opt into all unsafe fixes (the --unsafe-fixes flag), or they use the extend-safe-fixes config option (which just overrides the default to mark this fix as safe all the time). I sort-of feel like the autofix is much more useful than the actual lint in this case -- to me (as the rule's author!), it feels too pedantic to bother with if it's not going to be autofixed -- so I'm slightly reluctant to put the fix behind another flag, when I feel like most users who've opted into the rule will probably have chiefly done so for the autofix in the first place anyway.

[dscorbett]

The documentation for fix safety says “The meaning and intent of your code will be retained when applying safe fixes” but meaning and intent are hard to determine. Technically, no fix is 100% safe because a module’s source code can be inspected at runtime. That is an extreme case, of course; I’m just acknowledging that there’s always a trade-off.

In the particular example with __match_args__, the fix is not merely unsafe: the order of __match_args__ is central to its purpose, so indirectly changing its order will almost certainly break something. In a case like that I would still report the rule violation but not offer any fix, because the rule is clearly violated but it’s not clear what to do about it.

Maybe for edge cases (i.e. the fixes which this PR still marks as safe) it would be sufficient to mark the fix as safe but document in the rule’s “Fix safety” section that the fix is actually unsafe in certain circumstances. I leave to you all to determine where to draw the line in this case and how complex to make the check, but I hope the unsafety will be taken into account somewhere, whether code or documentation.

[AlexWaygood]

Maybe for edge cases (i.e. the fixes which this PR still marks as safe) it would be sufficient to mark the fix as safe but document in the rule’s “Fix safety” section that the fix is actually unsafe in certain circumstances.

Great suggestion. I pushed some more docs on fix safety -- for this rule and for RUF022 -- in 77bf580.

[MichaReiser]

Nit nit: We may want to consider adding an is_unused method. It reads more naturally

[AlexWaygood]

Agreed -- but there's quite a few other places where we might consider using that method, so I'll propose it in a followup

[AlexWaygood]

See #12729