[flake8-pyi] Fix custom-typevar-for-self with string annotations (PYI019)#18311
Conversation
|
…tom_tv_for_self_string # Conflicts: # crates/ruff_linter/src/rules/flake8_pyi/rules/custom_type_var_for_self.rs
…tom_tv_for_self_string # Пожалуйста, введите сообщение коммита, для объяснения, зачем нужно # это слияние, особенно, если это слияние обновленной вышестоящей # ветки в тематическую ветку. # # Строки, начинающиеся с «#» будут проигнорированы, а пустое сообщение # отменяет процесс коммита.
close2code-palm
changed the title
|
@MichaReiser Hi! My mate adviced me to request review from you, can I?) |
|
I'm planning to review this, just haven't had a chance yet! |
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thanks! This is looking good, just a couple of suggestions.
| let self_or_cls_annotation = match self_or_cls_annotation_unchecked { | ||
| ast::Expr::StringLiteral(_) => { | ||
| let Some(literal_expr) = self_or_cls_annotation_unchecked.as_string_literal_expr() | ||
| else { | ||
| return; | ||
| }; | ||
| let Ok(parsed_expr) = checker.parse_type_annotation(literal_expr) else { | ||
| return; | ||
| }; | ||
| parsed_expr.expression() | ||
| } | ||
| ast::Expr::Subscript(_) | ast::Expr::Name(_) => self_or_cls_annotation_unchecked, | ||
| _ => return, | ||
| }; |
There was a problem hiding this comment.
I have two suggestions here:
| let self_or_cls_annotation = match self_or_cls_annotation_unchecked { | |
| ast::Expr::StringLiteral(_) => { | |
| let Some(literal_expr) = self_or_cls_annotation_unchecked.as_string_literal_expr() | |
| else { | |
| return; | |
| }; | |
| let Ok(parsed_expr) = checker.parse_type_annotation(literal_expr) else { | |
| return; | |
| }; | |
| parsed_expr.expression() | |
| } | |
| ast::Expr::Subscript(_) | ast::Expr::Name(_) => self_or_cls_annotation_unchecked, | |
| _ => return, | |
| }; | |
| let self_or_cls_annotation = match self_or_cls_annotation_unchecked { | |
| ast::Expr::StringLiteral(literal_expr) => { | |
| let Ok(parsed_expr) = checker.parse_type_annotation(literal_expr) else { | |
| return; | |
| }; | |
| parsed_expr.expression() | |
| } | |
| _ => self_or_cls_annotation_unchecked, | |
| }; |
One is just to simplify the as_string_literal_expr, which can be combined with the match arm itself, and the second is that the original code didn't restrict the Expr to a Subscript or Name, so I don't think we should do that here either. Instead any other Expr should be allowed, as before, with the wildcard arm (_).
| 189 |- def bad_instance_method_with_string_annotations(self: "_S", arg: str) -> "_S": ... # PYI019 | ||
| 189 |+ def bad_instance_method_with_string_annotations(self, arg: str) -> "Self": ... # PYI019 |
There was a problem hiding this comment.
We may also need to adjust the replacement range. I don't think we want to replace "_S" with "Self". I think it should just be an unquoted Self.
There was a problem hiding this comment.
I tried to look around for the right way how to extend this behaviour but failed even with ugly approach. So, do we need this, as some other rule applies a fix to this moment right after, we just see "Self" in tests only related to this rule? I can't disagree that rule should do this on its own, but it will require more code changes and help from others or more time.
There was a problem hiding this comment.
Okay, you may be right. I looked at this a bit and didn't see a good solution either. I was afraid that we might be adding an unused Self import if all of the Self instances were quoted, but that doesn't appear to be the case: https://play.ruff.rs/014e3efe-829c-42b9-af40-f3527508bc57 (no import warnings with ALL rules selected). So I'm happy moving forward with the current version. Thanks!
ntBre
changed the title
flake8-pyi] Fix custom-typevar-for-self with string annotations (PYI019)
* main: (38 commits) [`pyupgrade`] Suppress `UP008` diagnostic if `super` symbol is not builtin (#18688) [pylint] Fix `PLW0128` to check assignment targets in square brackets and after asterisks (#18665) [`refurb`] Make the fix for `FURB163` unsafe for `log2`, `log10`, `*args`, and deleted comments (#18645) [ty] allow `T: Never` as subtype of `Never` (#18687) [ty] Use more parallelism when running corpus tests (#18711) [ty] Support `dataclasses.KW_ONLY` (#18677) [`ruff`] Check for non-context-manager use of `pytest.raises`, `pytest.warns`, and `pytest.deprecated_call` (`RUF061`) (#17368) Add syntax error when conversion flag does not immediately follow exclamation mark (#18706) [`flake8-pyi`] Fix `custom-typevar-for-self` with string annotations (`PYI019`) (#18311) Drop confusing second `*` from glob pattern example (#18709) [ty] Stabilize completions (#18650) [ty] Correctly label typeshed-sync PRs (#18702) Update Rust crate memchr to v2.7.5 (#18696) Update dependency react-resizable-panels to v3.0.3 (#18691) Update Rust crate clap to v4.5.40 (#18692) Update Rust crate libcst to v1.8.2 (#18695) Update Rust crate jiff to v0.2.15 (#18693) Update Rust crate libc to v0.2.173 (#18694) Update Rust crate syn to v2.0.103 (#18698) Update Rust crate toml to v0.8.23 (#18699) ...
2 participants
Summary
Solves #18257
Test Plan
Snapshots updated with some cases (negative, positive, mixed annotations).