[ruff] Classes with mixed type variable style (RUF053)#15841
[ruff] Classes with mixed type variable style (RUF053)#15841AlexWaygood merged 10 commits intoastral-sh:mainfrom
ruff] Classes with mixed type variable style (RUF053)#15841Conversation
|
AlexWaygood
left a comment
There was a problem hiding this comment.
Thanks! Some comments below. I haven't looked in detail at the implementation yet, just at docs and the behaviour of the rule as indicated by the snapshots.
I don't think this should be a pyupgrade rule, since all our existing pyupgrade rules are modernisation/stylistic rules. This rule is different, since it detects (and fixes) behaviour that would cause the code to raise an exception at runtime. I understand that you want to share the code being used in the existing pyugprade PEP-695 rules, but we could always move those utilities to somewhere else in the ruff_linter crate, where it could be accessed by both the pyupgrade rules and this rule
crates/ruff_linter/src/rules/pyupgrade/rules/pep695/class_with_mixed_type_vars.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pyupgrade/rules/pep695/class_with_mixed_type_vars.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pyupgrade/rules/pep695/class_with_mixed_type_vars.rs
Show resolved
Hide resolved
...uff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP050.py.snap
Outdated
Show resolved
Hide resolved
...uff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP050.py.snap
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/pyupgrade/rules/pep695/class_with_mixed_type_vars.rs
Outdated
Show resolved
Hide resolved
...uff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP050.py.snap
Outdated
Show resolved
Hide resolved
...uff_linter/src/rules/pyupgrade/snapshots/ruff_linter__rules__pyupgrade__tests__UP050.py.snap
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I haven't reviewed the code in detail yet either, but it looks a little longer than I expected. I might be oversimplifying things, but can we pretty much reuse the logic from the UP046 class rule and just perform an Edit::insertion at the end of type parameter list instead of writing a full list? That would save you the effort of duplicating all the checks from UP046 (like the default kwarg that Alex pointed out) and converting existing TypeParams to TypeVars only to write them back into TypeParams. This would also lose you any deduplication it looks like you're doing with is_existing_param_of_same_class, but the incoming code was a real mess if it had mixed type variables with duplicates 😆
I added a testcase for this: # Original
class C[
T # Comment about `T`
](Generic[_A]): ...
# Wrong fix
class C[
T # Comment about `T`, _A
]: ...
# Also wrong fix (slightly)
class C[
T, _A # Comment about `T`
]: ...
# Correct fix, but looks irritating
class C[
T # Comment about `T`
, _A]: ...I wasn't sure if relying on the formatter is the way to go. |
|
Ah interesting, I did not catch that about comments, thanks! I'll try to give this a fuller review today. |
79763f4 to
7915f10
Compare
|
Could you possibly retitle the PR and edit the description to make clear that this is no longer proposed as a pyupgrade rule? |
pyupgrade] Classes with mixed type variable style (UP050)ruff] Classes with mixed type variable style (RUF060)
ntBre
left a comment
There was a problem hiding this comment.
Thanks for this! It looks good to me overall besides a couple of nits and a bigger question about reusing the TypeVarReferenceVisitor.
crates/ruff_linter/src/rules/ruff/rules/class_with_mixed_type_vars.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/class_with_mixed_type_vars.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/class_with_mixed_type_vars.rs
Outdated
Show resolved
Hide resolved
ntBre
left a comment
There was a problem hiding this comment.
Thanks for your work on this. I especially appreciate the thorough test fixtures. I think I'll need to steal some of those for UP046 and UP047, if you don't get to them first 🙂
I'd like to see my latest comments addressed, but they're mostly nits so I'm still happy to approve.
crates/ruff_linter/src/rules/ruff/rules/class_with_mixed_type_vars.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/class_with_mixed_type_vars.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/class_with_mixed_type_vars.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/class_with_mixed_type_vars.rs
Outdated
Show resolved
Hide resolved
547a1bd to
32684c9
Compare
AlexWaygood
left a comment
There was a problem hiding this comment.
Thank you -- this overall looks great now! And I'll second @ntBre that your tests are excellent. I just have a few coding nits:
crates/ruff_linter/src/rules/ruff/rules/class_with_mixed_type_vars.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/class_with_mixed_type_vars.rs
Outdated
Show resolved
Hide resolved
crates/ruff_linter/src/rules/ruff/rules/class_with_mixed_type_vars.rs
Outdated
Show resolved
Hide resolved
...ter/src/rules/ruff/snapshots/ruff_linter__rules__ruff__tests__preview__RUF060_RUF060.py.snap
Show resolved
Hide resolved
ruff] Classes with mixed type variable style (RUF060)ruff] Classes with mixed type variable style (RUF053)
Summary
Resolves #15620.
RUF053reports the firstGeneric[...]base of any class with a PEP 695 type parameter list. It reuses much of the logic added in #15565, #15659 and other PRs.Since the original code is fundamentally flawed, the fix takes some liberty in its rewrite. As such, it is always marked as unsafe.
Test Plan
cargo nextest runandcargo insta test.