-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Implement flake8-type-checking #1785
Comments
Note that the project was renamed to flake8-type-checking (https://github.com/snok/flake8-type-checking). |
Came here to file this exact same issue. I had no idea it was supported in flake8. This would be huge for us! cc: cc @smackesey |
I'd be very happy to take an initial stab at porting the library. I think most error codes can be auto-fixed, so the ported version might be significantly more useful in this form. Are there any docs or resources I should read up on before diving into the ruff source code? |
Awesome! We have some high-level docs on adding new rules in the contributing guide. Those are mostly helpful for guiding you on the mechanics of defining the rule, navigating the various files, etc. We'll also want to add a module in Beyond the rule logic, I suspect we'll also need to adjust My suggestion, just to get familiar with the abstractions and conventions, would be to start with maybe the empty type-checking block rule? It seems like that wouldn't require any significant changes to the |
Let me know if I can help, I'm competent in Rust but I'm not familiar with the code base. This would be an awesome feature and would reduce a lot of cognitive overhead for me personally because for Hatch I painstakingly use conditional and/or lazy imports to reduce CLI startup time. |
I’m happy to do a first PR today to lay some of the groundwork here. |
That would be neat since I'm going to cut a release in a few days which will generate projects that use Ruff and it would be nice to enable this. |
It might be worth discussing whether the rules are worth porting 1:1 and whether any APIs should change. For example, The I was thinking I'd sit down an take a stab at a first rule tomorrow. I agree, the empty type checking block is probably a good candidate. If you've already started though @charliermarsh, feel free to go ahead and I'll try to get involved after the groundwork has been done 👍 |
A couple things:
I may get started on this tonight (at least to setup the plugin scaffolding), but either way, I'll post here at the end of the day so that you know whether there's any overlap! |
@sondrelg - I went ahead and added the scaffolding for the plugin in #2048, along with a basic implementation of I think the biggest thing to figure out is what additional state we need to track in |
This PR adds the scaffolding files for `flake8-type-checking`, along with the simplest rule (`empty-type-checking-block`), just as an example to get us started. See: #1785.
The way the plugin works today is by exhaustively checking every element except annotations, then matching imports to "uses". IIRC, most uses are either It's also important to map the line start/end of all type checking blocks, since use of a guarded import within the scope of that guard, is fine: from typing import TYPE_CHECKING as some_alias
if some_alias:
import pandas as pd
df = pd.DataFrame() # this is fine
df = pd.DataFrame() # this is not Then we need a good way of normalizing aliases, if possible. Identifying whether Finally there's minor things like keeping track of futures imports, function scopes, and classifying imports as local or remote. The exhaustive list of plugin state can be found here: https://github.com/snok/flake8-type-checking/blob/main/flake8_type_checking/checker.py#L372:L434 I'll read over the PR now 🙂 |
Cool -- we have support for and a pattern for this (
👍 We do track future imports and function scopes as |
Yeah I noticed, that's great! What do you think about import classification? The plugin uses classify-imports to distinguish between local imports, builtin imports, and library imports. The rationale behind that mainly being that only local application imports lead to import circularity issues, so for some users it might be nice to limit the linting to that class of imports. I was thinking I might start taking a stab at porting the import classification logic we need as rust code? |
Our import classification logic is actually a port of The other question is whether we want to have separate rules for different import classes, as in your existing plugin. I don't feel strongly! If we have the ability to categorize imports, then it makes sense to me to support different rules. But I'm curious if people turns those on and off in practice, what's your sense from issues, etc.? |
@sondrelg - I realize it's probably unhelpful for me to just keep saying "We have something that does X" 😅 so let me take a second to outline how this plugin might be implemented in a bit of detail... If you're still interested, definitely let me know. If not, no worries at all. |
So, for the rules around whether an import should / shouldn't be a in
We probably want to have this information available to use when we run One way to implement this would be as follows:
(All of this ignores how we should treat redefinitions, i.e., modules that are imported multiple times.) |
That seems great.
This is true, but has been a bit of a pain to get right. We essentially try to shadow pyflake's
I think there might also be a need to track function scopes, but perhaps the functionality that hinges on that information can be added later, or done in another way. Essentially I think we handle this case by tracking function scopes: if TYPE_CHECKING:
from pandas import DataFrame
foo: DataFrame # this is fine
def bar():
from pandas import DataFrame
baz = DataFrame() # this is also fine, but only because of the function-scoped import This is a bit of a contrived example, but there are cases where this type of behavior can happen; typically in large files with circular import problems. Did you say there is tooling to classify imports as stdlib/application/third-party/future already as well? I know the futures import is mapped 👍 You are probably familiar, but Lastly, I'd be very happy to write some or most of the code, but to be realistic I think I'm down to being able to contribute 3-4 hours a week. If that's fine, I'm happy to commit to giving that a go, but if you or anyone else wants to jump on this and get it done, then I'd of course be happy to help wherever I can, but let you do most of the work 🙂 |
Yeah, it's all in
No prob at all. I may get to it, I may not -- if I do, I'll comment here so that we don't duplicate work. |
Yeah. Sorry about that rule @sondrelg. In principle it was a nice idea, but there are certainly problems as you've mentioned that we perhaps didn't foresee. The main thing it trying to achieve was to reduce the runtime performance hit from building complex types when they're only relevant for static type checking. An alternative approach is to use type aliases in global scope to avoid the hit for building these complex types repetitively in every function call, loop, or whatever. As such Ah, sweet hindsight... |
It's really only the lack of IDE support that makes it hard to work with in my experience. The rationale for the rule itself was great haha 👏 |
Okay, I just knocked this out in #2146 and #2147. It'll go out as soon as we can release again (we're blocked by PyPI limits right now and waiting for an exemption -- see #2136). The current version doesn't include any autofix capabilities (it's challenging because we need to remove imports from one part of the AST and re-add them elsewhere), but I tested on Dagster and the inference seems reliable. \cc @schrockn @smackesey @ofek |
My only hesitation here is that I used |
On second thought: |
I'm going to rename to |
I originally used TCH. Not sure if that's taken, but could be appropriate. Ugliest candidate yet though 😄 |
Oh, cool. If you used that originally, let's go with that over |
Nice to see all the progress on this. We haven't turned it on yet because I think autofix is essential here (to avoid annoying scenario where you are writing type annotations, import something via auto-complete, then have to go fix a lint error and manually move into a |
Please don't apologize, I'm quite fond of that rule and am not worried about pycharm since I don't use it. :P (I would be interested in using it from ruff, if it was implemented. It seems useful to avoid the runtime hit even once.) |
FYI: autofix for these rules will be enabled in the next release. |
It seems that this doesn't work well with mashumaro, I got
|
That’s interesting. Can you share the original and auto-fixed reproducible code? I think it’s worth to create a separate issue for that here. P.S. From what I’ve read, putting imports to
This assumption can’t be applied to all the existing code, so this dangerous feature in auto-fixing must be disabled by default. |
It's a useful tool for determining which imports aren't run-time required, reducing import time. Thanks!
The text was updated successfully, but these errors were encountered: