[flake8-pyi] Also check string annotations (PYI041)#19023
[flake8-pyi] Also check string annotations (PYI041)#19023ntBre merged 11 commits intoastral-sh:mainfrom
flake8-pyi] Also check string annotations (PYI041)#19023Conversation
robsdedude
force-pushed
the
fix/pyi041-in-string-annotation
branch
from
f4758d9 to
bc0685b
Compare
|
robsdedude
force-pushed
the
fix/pyi041-in-string-annotation
branch
from
bc0685b to
b7d7160
Compare
| def good1(arg: "int") -> "int | bool": | ||
| ... |
There was a problem hiding this comment.
Can you add a test for a type annotation consisting of a concatenated string literal:
| def good1(arg: "int") -> "int | bool": | |
| ... | |
| def good1(arg: "int") -> "int" "| bool": | |
| ... |
There was a problem hiding this comment.
Good catch! While detection works, the fix did indeed drop the quotation marks.
There was a problem hiding this comment.
This is non-trivial to fix. I'll have to go on a side-mission fixing #19184 first.
My approach was to alter parse_simple_type_annotation (just like parse_complex_type_annotation already does) such that the resulting resolved virtual expression spans that whole string. That way, the rule can just replace the whole string and wrap it in quotes if the original annotation was a forward reference.
But alas, that change to parse_simple_type_annotation amplifies the linked bug to also affect non-concatenated forward refs.
There was a problem hiding this comment.
I believe this was part of the reason why we decided not to support string annotations in the initial implementation as they aren't as common anymore and we didn't felt like they're worth all the complexity they add.
Could we disable the fix for problematic stringified annotations (I think you can also use triple quoted strings or put them across multiple lines). We should try to add tests for all of those if we decide that supporting them is worth the complexity
There was a problem hiding this comment.
Tripple quoted type annotations actually work fine. There are some in the test set.
There was a problem hiding this comment.
I ended up addressing this in a similar way as RUF013 handles is:
- analysis on stringized annotations: will do
- suggesting fixes on stringized annotations: only if simple sting (i.e., non-concatendated) and always mark as unsafe.
ntBre
left a comment
ntBre
left a comment
There was a problem hiding this comment.
Thanks and sorry for the delay. This looks good to me, but I think we may need to gate this behind preview since it's a pretty big change to a stable rule.
Otherwise I just had some small ideas/suggestions, assuming @MichaReiser's concerns are resolved.
crates/ruff_linter/src/rules/flake8_pyi/rules/redundant_numeric_union.rs
Outdated
Show resolved
Hide resolved
ntBre
changed the title
flake8_pyi] Fix PYI041 not resolving string annotationsflake8-pyi] Fix PYI041 not resolving string annotations
|
@ntBre @MichaReiser Could you have another look. I think I addressed all comments. Note to myself: when this is merged, take another look at #19184 to see if the same approach (#19023 (comment)) could be applied. |
There was a problem hiding this comment.
Thank you! And sorry for the delay in reviewing again.
I pushed a few commits applying the preview gate and fixing a few nits I had. I just moved map_maybe_stringized_annotation into the rule code for now instead of making it a new method on Checker and introduced a new AnnotationKind enum, which simplified a couple of the later checks. This also helped to avoid a fix in the presence of line continuations in the string annotation.
ntBre
changed the title
flake8-pyi] Fix PYI041 not resolving string annotationsflake8-pyi] Also check string annotations (PYI041)
* main: (43 commits) [`ruff`] Suppress diagnostic for strings with backslashes in interpolations before Python 3.12 (`RUF027`) (#21069) [flake8-bugbear] Fix B023 false positive for immediately-invoked lambdas (#23294) [ty] Add `Final` mdtests for loops and redeclaration (#23331) [`flake8-pyi`] Also check string annotations (`PYI041`) (#19023) Remove AlexWaygood as a flake8-pyi codeowner (#23347) [ty] Add comments to clarify the purpose of `NominalInstanceType::class_name` and `NominalInstanceType::class_module_name` (#23339) Add attestations for release artifacts and Docker images (#23111) [ty] Fix `assert_type` diagnostic messages (#23342) [ty] Force-update all insta snapshots (#23343) Add Q004 to the list of conflicting rules (#23340) [ty] Fix `invalid-match-pattern` false positives (#23338) [ty] new diagnostic called-match-pattern-must-be-a-type (#22939) [ty] Update flaky projects (#23337) [ty] Increase timeout for ecosystem report to 40 min (#23336) Bump ecosystem-analyzer pin (#23335) [ty] Replace `strsim` with CPython-based Levenshtein implementation (#23291) [ty] Add mdtest for staticmethod assigned in class body (#23330) [ty] fix inferring type variable from string literal argument (#23326) [ty] bytes literal is a sequence of integers (#23329) Update rand and getrandom (#23333) ...
3 participants
Summary
While working on #18637, I noticed that PYI041 does not properly resolve string annotations. This PR fixes that.
Test Plan
A whole new file with tests has been added.