Implement configuration options from `flake8-type-checking`

[charliermarsh]

• [x] exempt-modules
• [x] strict
• [ ] pydantic-enabled
• [ ] pydantic-enabled-baseclass-passlist
• [ ] fastapi-enabled
• [ ] fastapi-dependency-support-enabled
• [ ] cattrs-enabled

See: #1785

[charliermarsh]

@sondrelg - not urgent, but any input on these options based on your experience? Any thing you'd warn me about, etc.?

[sbrugman]

Is it feasible to create another script in addition to add_rule.py and add_plugin.py, e.g. add_option.py? Not sure if this can be automated, didn't look into it. Could be a good thought to have in mind when new options are implemented.

ps. @charliermarsh some other options are labelled isort but I think should also have the configuration tag

[sondrelg]

Yeah exempt-modules and strict are easy and probably fine to implement, but the rest are.. sub-optimal:

The follow snippets represent code that's incompatible with the general premise of the flake8-type-checking plugin:

Pydantic model annotations

from pydantic import BaseModel
from pandas import DataFrame    


class Foo(BaseModel):
    df: DataFrame

FastAPI view function annotations

@app.get()
def foo(request: Request) -> Response:
    ...

Any FastAPI dependency functions

# dependencies.py
async def is_authorized(request: Request) -> bool:  # Request here cannot be wrapped in quotes
    ...

# endpoints.py
async def foo(request: Request, is_authorized: bool = Depends(is_authorized)) -> bool:
    ...

---

Since we're performing static single file analysis, there is no way to be certain whether a function can be used as a "dependency" in FastAPI, or whether a class that inherits from some base class is pydantic model or not. attrs classes are a little simpler to spot, but we can never be certain there either.

The reason this matters is because if we guard an import within a TYPE_CHECKING block, and it is attempted to be evaluated by, e.g., pydantic at runtime the consequences are runtime errors. This is a pretty terrible consequence, so my thinking has been that the bar for what uncertainty we'll tolerate should be very high.

So the pydantic plugin essentially just treats ALL classes with base classes as potential pydantic classes. The type-checking-pydantic-enabled-baseclass-passlist setting then gives users a way to tell us which base classes are definitely not an alias for- or abstract class containing BaseModel.

The same thing happens for the FastAPI option. Any decorated function is ignored, and you can opt-into ignoring any function, since they may be used in the Depends wrapper.

---

I think I would recommend adding a disclaimer to the readme informing users that this plugin is fundamentally incompatible with projects that evaluate type hints at runtime, and drop these config options -- but obviously feel free to add them if you want. In general though I think it's a lot of work for very little gain.

[sasanjac]

I don't understand the problem for pydantic support. Can't we just assume that every class that has pydantic.BaseModel as a base class needs to have the types of their class attributes checked at runtime?

[charliermarsh]

Oh, I believe we do now require that class attributes are present at runtime.

[charliermarsh]

Weird to link to a tweet for reference but: https://twitter.com/charliermarsh/status/1623389778375843840

[sondrelg]

I don't understand the problem for pydantic support. Can't we just assume that every class that has pydantic.BaseModel as a base class needs to have the types of their class attributes checked at runtime?

What if you do this?

# a.py

class AppBaseModel(BaseModel):
    name: str  # every model in my app will have this attribute

# b.py 
from a import AppBaseModel

class Foo(AppBaseModel):

It's been a while since we discussed it, but I think there's a number of way to assume incorrectly. And since the consequence of a false negative is potential runtime errors in your prod app, we should try to avoid that at all costs.

Does that make sense @sasanjac?

[sondrelg]

That's news to me @charliermarsh. Is that a 3.11 or 3.12 thing?

[sasanjac]

nvm, my brain left me there for a bit.

Does that make sense @sasanjac?

~~No, because Foo.mro() still contains BaseModel.~~

[charliermarsh]

@sondrelg - I don't think so but I haven't really looked back on it. You can see the docs here. The logic is that if you're in a module or class scope, the annotations need to be added to __annotations__, so they end up getting evaluated at runtime. In a function scope, there's no such construct, so they just get skipped.

There are some comments from Python core contributors in that thread: https://twitter.com/MissingClara/status/1623435238289608709

[sasanjac]

nvm, my brain left me there for a bit.

Does that make sense @sasanjac?

~No, because Foo.mro() still contains BaseModel.~

But can't we then turn the procedure around? Explicitely state all base classes that inherit from BaseModel?

[ofek]

Tangentially related: is there a way to enforce imports that must be present like from __future__ import annotations?

[charliermarsh]

@ofek - Yeah, we support it via required-imports (which maps to I002).

[sondrelg]

nvm, my brain left me there for a bit.

Does that make sense @sasanjac?

~No, because Foo.mro() still contains BaseModel.~

But can't we then turn the procedure around? Explicitely state all base classes that inherit from BaseModel?

The example above isn't the worst case - we could have n layers of base classes and mixins to traverse located in m different files. Since flake8 operates in a one-file-at-the-time type of way, leaving the scope of the single file was not something that seemed worth the investment in the original plugin, but maybe ruff has some foundations that make it worth considering :slightly_smiling_face:

[sasanjac]

Yeah I totally forgot about that. But why don't leave it up to the user to specify all pydantic base classes in the config file? I mean with the default config, the user will encounter runtime errors anyway.

[sondrelg]

Mostly because no one has requested it :slightly_smiling_face: How will they run into runtime errors anyway?

[sasanjac]

Because with the default config they will move all typing imports into the TYPE_CHECKING block. For me it just seems the most straightforward way to have something like type-checking-pydantic-enabled-baseclasses instead of dropping the config options.

[sondrelg]

Maybe just call it type-checking-pydantic-baseclasses? Yeah I don't see why not if you want that feature :slightly_smiling_face:

[sasanjac]

Ok, now I only have to learn rust 🙈

[charliermarsh]

The remaining options are covered by #3292 (albeit with an API that differs from the original flake8-type-checking), thanks to @sasanjac!