[flake8-pyi] Implement PYI059

[tusharsadhwani]

Summary

Implements Y059 from flake8-pyi: typing.Generic[] should be the last base in a class' bases list.

If Multiple Generic[] classes are found, we don't generate a fix for it.

Test Plan

cargo test and cargo insta review.

[tusharsadhwani]

Is it okay to add the autofix in the same PR?

[zanieb]

Just wondering, what happens if a class has two Generic[] bases?

[zanieb]

Is it okay to add the autofix in the same PR?

Yep!

[tusharsadhwani]

what happens if a class has two Generic[] bases?

It's a runtime error:

>>> import typing
>>> T = typing.TypeVar('T')
>>> U = typing.TypeVar('U')
>>> class C(typing.Generic[T], typing.Generic[U]):
...   ...
...
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/opt/homebrew/Cellar/[email protected]/3.12.2_1/Frameworks/Python.framework/Versions/3.12/lib/python3.12/typing.py", line 1102, in _generic_init_subclass
    raise TypeError(
TypeError: Cannot inherit from Generic[...] multiple times.

[zanieb]

Good to know. We should make sure the rule doesn't behave poorly on that case regardless, i.e. it could cause an infinite fix loop.

[github-actions[bot]]

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

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

rotki/rotki (+2 -0 violations, +0 -0 fixes)

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

+ rotkehlchen/db/filtering.py:577:25: PYI059 [*] `Generic[]` should always be the last base class
+ rotkehlchen/types.py:977:34: PYI059 [*] `Generic[]` should always be the last base class

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
PYI059	2	2	0	0	0

[tusharsadhwani]

We should make sure the rule doesn't behave poorly on that case regardless

Should multiple Generic[] case not raise the issue at all then?

[AlexWaygood]

Should multiple Generic[] case not raise the issue at all then?

I'd say probably not. The behaviour we implemented at flake8-pyi is to ignore this rule if Generic[] appeared multiple times in the bases tuple, and I think it makes sense to do the same thing here. If you have something like class Foo(int, Generic[T], Generic[S]): pass, I think it's honestly just confusing for a user if they get a message about Generic[] "not being the last base class". There's a much more significant issue in their code (you can't have more than one Generic[] in the bases tuple), and if they solve the more significant issue, then Generic[] not being the last base class would be naturally solved as a side effect of that.

(We shouldn't flag multiple Generic[]s in this check; that could possibly be a separate check.)

[AlexWaygood]

(Everything looks great to me now except the autofix code, which I'm still not quite sure about -- both in terms of code readability and in terms of the number of .expect() and .unwrap() calls in it :-)

[AlexWaygood]

@tusharsadhwani, I've just pushed https://github.com/astral-sh/ruff/pull/11233/commits/6fce0fd868344efb7e3d4e1336e252fd8f622f35, which gets rid of the remaining .expect() and .unwrap() calls from generate_fix(). Instead, the errors are bubbled up by returning an anyhow::Result<Fix> from generate_fix(). In combination with the Diagnostic::try_set_fix() method, that means that, rather than panicking, we'll gracefully log the error and move on if we fail to construct a Fix. That's usually preferable for this kind of thing, since it's not 100% essential that a Fix is constructed, but panicking would be pretty bad.

[tusharsadhwani]

Okay, this is much cleaner.