snok / flake8-type-checking

Flake8 plugin for managing type-checking imports & forward references.
BSD 3-Clause "New" or "Revised" License
113 stars 16 forks source link

TC004: False positive when variable name matches name of module in `TYPE_CHECKING` block #160

Closed olepbr closed 1 year ago

olepbr commented 1 year ago

My hypothesis is that the checker doesn't differentiate between module and variable names, and thus thinks that rest_framework.request and the variable request are one and the same (or at least that use of the latter constitutes a use of the latter).

Having modules and variables with the same name is maybe a bad idea anyway, so I think it's fair to not solve this, too.

Reproducer

Requires DRF, but should be possible to find an example in Python itself.

from __future__ import annotations

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from rest_framework import request

def create(request: request.Request) -> None:
    str(request)
sondrelg commented 1 year ago

Now I see what you mean! Agree it generally seems like bad practice to shadow import names in function scopes, but this actually isn't all that hard to fix - we already have handling for function-scoped imports.

I think https://github.com/snok/flake8-type-checking/pull/161 fixes the issue, if you want to take a look.

We could also handle this:

from __future__ import annotations

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from rest_framework import request

def foo():
    request = 5
    print(request)

and other weirder edge cases.. but I think I'll draw a line in the sand here hehe

olepbr commented 1 year ago

We could also handle this:

from __future__ import annotations

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from rest_framework import request

def foo():
    request = 5
    print(request)

and other weirder edge cases.. but I think I'll draw a line in the sand here hehe

I'm not sure what there is to handle here? Here the import isn't used and flake8 itself correctly flags this as F811 (redefinition of unused import).

sondrelg commented 1 year ago

Also raises TC004 for me :thinking: So does

def foo():
    request = lambda: None

and

def foo():
    class request:
        pass

    print(request.__name__)

and a bunch of other varieties of shadowing :sweat_smile: Python is so dynamic, there's probably tens or hundreds of variations of this where the plugin will report false positive TC004s

olepbr commented 1 year ago

Also raises TC004 for me 🤔 So does

My bad; it does for me, too.

and a bunch of other varieties of shadowing sweat_smile Python is so dynamic, there's probably tens or hundreds of variations of this where the plugin will report false positive TC004s

Yup. Tbh, I'm leaning towards having flake8 disallow shadowing across module/variable domains :-)