-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[flake8-pyi] Fix PYI041 not resolving string annotations
#19023
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[flake8-pyi] Fix PYI041 not resolving string annotations
#19023
Conversation
f4758d9 to
bc0685b
Compare
|
bc0685b to
b7d7160
Compare
| def good1(arg: "int") -> "int | bool": | ||
| ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! While detection works, the fix did indeed drop the quotation marks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tripple quoted type annotations actually work fine. There are some in the test set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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. |
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.