Skip to content
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

TC004 reported for object that exists both inside and out of type checking block #131

Closed
eli-schwartz opened this issue Aug 28, 2022 · 4 comments · Fixed by #174
Closed

Comments

@eli-schwartz
Copy link

See the following code: https://github.com/mesonbuild/meson/blob/d4733984e9e3970c23a9436dce8919f8a25194e8/mesonbuild/compilers/mixins/gnu.py#L30-L39

if T.TYPE_CHECKING:
    from ...compilers.compilers import Compiler
else:
    # This is a bit clever, for mypy we pretend that these mixins descend from
    # Compiler, so we get all of the methods and attributes defined for us, but
    # for runtime we make them descend from object (which all classes normally
    # do). This gives up DRYer type checking, with no runtime impact
    Compiler = object

...

# this is a mixin
class GnuLikeCompiler(Compiler, metaclass=abc.ABCMeta):

This reports:

mesonbuild/compilers/mixins/gnu.py:33:1: TC004 Move import 'Compiler' out of type-checking block. Import is used for more than type hinting.

But weirdly enough, this type-checking only import is shadowed at runtime and thus not actually used. Not sure if there's a good way to detect it -- even the code comments imply it's a bit too clever.

@sondrelg
Copy link
Member

Nice, this is something we should be able to add handling for I reckon 👍

I might have time to get this added in a few weeks, and in the meantime contributions are always welcome 🙂

@sondrelg
Copy link
Member

sondrelg commented Sep 1, 2022

I started to think about how to implement this, and unfortunately I think this can get quite complex. For the easy scenario, when a file has a single type-checking block at the top, we can map the import names in the else block and offset the "unused" imports by this set. This is pretty straight forward and make sense to do, as the runtime equivalents do exist, so there's actually no basis for the TC004 error. So far so good.

The problem arises when we consider how this will work for multiple type-checking blocks scoped differently around the code. We suddenly need a lot of logic to get this right I think :/

What do you think @eli-schwartz?

@eli-schwartz
Copy link
Author

Handling only the easy scenario would still be more than what you do now, so I guess it cannot possibly hurt. :)

Does it help if you only consider names in the else block, when they are defined in the immediately matching if block?

@sondrelg
Copy link
Member

sondrelg commented Sep 2, 2022

Take this example:

if T.TYPE_CHECKING:
    from  a import Compiler

def foo():
    if T.TYPE_CHECKING:
        from b import Compiler
    else:
         Compiler = object
    
    bar: Compiler
    return bar

The plugin doesn't know about scopes, it just knows about line numbers. So this isn't impossible; it's just hard to make the plugin do anything coherent without a lot of added complexity. And the false positives seem easy to ignore.

If you want to take a stab at implementing it, PRs are welcome 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants