[flake8-type-checking] Adds implementation for TC007 and TC008
#12927
Conversation
The implementation for TCH007 is incomplete and also requires some changes to TCH004 in order to avoid conflicts between TCH004 and TCH007
|
| code | total | + violation | - violation | + fix | - fix |
|---|---|---|---|---|---|
| TC008 | 6 | 6 | 0 | 0 | 0 |
This comment was marked as resolved.
This comment was marked as resolved.
CodSpeed Performance ReportMerging #12927 will not alter performanceComparing Summary
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Also fixes lexicographical lookup not taking shadowed bindings into account.
|
@charliermarsh It might be worth to move the TCH010 logic to where the TCH008 logic currently sits, so we can actually autofix TCH010 in the same cases where we would otherwise emit a TCH008 by removing the quotes when we know that the quoted expression should be entirely available at runtime. Concretely we would pass it at the stage where we parse deferred string annotations and check if the current parent expression is a The drawback would be that it may be more difficult to create an autofix in the opposite case that expands the quotes to the entire type expression, since we would need to walk the parent nodes in order to determine the root node of the type expression. |
|
Thanks @Daverball, sorry that I haven't had a chance to review this yet. |
|
@charliermarsh Just a friendly reminder in case this PR slipped through the cracks somehow |
...es/ruff_linter/src/rules/flake8_type_checking/rules/runtime_import_in_type_checking_block.rs
Outdated
Show resolved
Hide resolved
| @@ -662,12 +662,117 @@ impl<'a> SemanticModel<'a> { | |||
| } | |||
| } | |||
|
|
|||
| // FIXME: Shouldn't this happen above where `class_variables_visible` is set? | |||
There was a problem hiding this comment.
Why? Do you plan to follow-up on this? It seems unrelated to the PR.
There was a problem hiding this comment.
Because for BindingKind::Annotation this assignment is skipped. Maybe there's something subtle at play here, like BindingKind::Annotation never occurring inside a function scope, but otherwise this seems like a bug, albeit one very unlikely to affect anyone in any real way.
I noted it because simulate_runtime_load has a similar structure but moved the update of this variable above to where class_variable_visible is updated.
If you don't think there's a bug here I'm happy to remove the comment, otherwise I'll either leave it in and follow up or fix it as part of this PR, whichever you'd prefer.
There was a problem hiding this comment.
Let's address this in a follow up PR
| // things from the outer class(es) that have been defined before the inner | ||
| // class. | ||
| if seen_function && !scope.kind.is_type() { | ||
| source_order_sensitive_lookup = false; |
There was a problem hiding this comment.
Do you think you could include some examples in the comments here (like, Python cod examples) to explain what this source-sensitive lookup is doing, and why?
There was a problem hiding this comment.
Actually, this prompted me to improve the function, since it currently takes some shortcuts under the assumption, that it will only be called for type definitions, but if that ever becomes untrue, or type definitions can start showing up in some of the other problematic places, then there are other kinds of bindings than AnnAssign and ClassDef where the target can occur before a child expression that doesn't yet have access to the binding. The semantic model handles this by walking the nodes in the correct order if I'm not mistaken.
I.e. things like regular assignments and named assignments. Comprehensions have the opposite problem, where the bindings can occur after use, especially with named expressions inside one of the conditions. In the flake8 plugin I'm handling all of these corner-cases by carefully choosing the position of when the binding starts being available.
There is one case I can't handle with pure source position and accept being wrong on, and that would be a named expression referring to itself within a condition of a comprehension. But everything else works. Backtracking the AST to look for bindings seems more robust, but also a lot more expensive.
I think for now I will handle the few additional easy cases and add documentation about when this method will fail to produce the correct result.
| @@ -662,12 +662,117 @@ impl<'a> SemanticModel<'a> { | |||
| } | |||
| } | |||
|
|
|||
| // FIXME: Shouldn't this happen above where `class_variables_visible` is set? | |||
There was a problem hiding this comment.
Let's address this in a follow up PR
|
@MichaReiser The tests should still be in their own test function, since both rules are enabled in that test function. That's how we made sure there were no circularity issues. |
|
@MichaReiser Also the fix safety comment is only correct for one of the rules, the other rule is always marked as unsafe but I still don't know why, I'm just relying on the fix safety of the other rule that uses the same fix. Either way I'm going to force push my local branch and include some of your changes, I already made some of the same changes locally, but I also elaborated on the documentation of |
|
Oh sorry. I think it would be good to add a comment to that test explaining why it is a separate tests or it's likely that we'll merge the tests again when we promote the rule to preview. Could you also extend/correct the fix safety comments because that's the only remaining thing that needs resolving before merging this PR |
Daverball
force-pushed
the
feat/tch007-tch008
branch
from
4b84f46
to
8cf8f27
Compare
|
@MichaReiser I think this should cover everything, but feel free to give it another cursory glance. |
|
Congrats @Daverball on implementing this rather complex rules! |
## Summary This came up as part of #12927 when implementing `SemanticModel::simulate_runtime_load`. Should be fairly self-explanatory, if the scope returns a binding with `BindingKind::Annotation` the bottom part of the loop gets skipped, so there's no chance for `seen_function` to have been updated. So unless there's something subtle going on here, like function scopes never containing bindings with `BindingKind::Annotation`, this seems like a bug. ## Test Plan `cargo nextest run`
This is part of a series of pull requests fulfilling my promise in #9573 to port some of the enhancements and new rules from flake8-type-checking to ruff.
Summary
This adds the missing flake8-type-checking rules TC007 and TC008 including auto fixes.
There is some overlap between TC007 and TC004, currently the existing rule takes precedence, although ideally we would still emit both violations and share a fix for overlaps based on the selected strategy (if it is possible for violations of two different rules to share the same fix). We could probably move the analysis for TC007 into the same function as TC004 in order to accomplish that. But we could defer that to a future pull request.
There is also some potential overlap between TC008 and TC010. Currently TC010 is prioritized, but since it currently has no fix it might make more sense to either prioritize TC008 or add a fix for TC010, although that could be covered by a future pull request.
Test Plan
cargo nextest run