TC201 false positives for generics that are not generic at runtime

[Daverball]

There's a whole bunch of generics defined in third party stubs packages, that do not derive from typing.Generic and thus are not subscriptable at runtime. Currently this plugin will emit a TC201 if those annotations are properly forward-declared using a string literal to avoid a runtime exception.

The plugin will either need to inspect the class to make sure it derives from typing.Generic and have a separate whitelist for the builtins that provide a __class_getitem__ without inheriting from typing.Generic, such as dict and list or just do what some other plugins do and add a setting for a blacklist or whitelist of classes which should be considered generic only at type checking time.

Without any change this produces far too many false positives for packages with third party stubs for TC201 to be useful in a large existing code-base, since you will need to add hundreds of noqa: TC201 comments.

[sondrelg]

Would you mind adding some code examples?

[Daverball]

Sure, let's take WTForms as an example, it has no builtin type stubs and thus requires types-WTForms.

It defines an UnboundField as a temporary container which contains all the information to construct a specific Field instance once you create an instance of a Form. For the purposes of type checking UnboundField is generic, so you know what kind of a Field it will create once bound, but at runtime UnboundField does not inherit from typing.Generic so you will get a TypeError if you try to subscript it:

from wtforms import Field
from wtforms.fields.core import UnboundField

foo: 'UnboundField[Field]'  # TC201
bar: UnboundField[Field]   # TypeError – 'type' object is not subscriptable

[sondrelg]

The plugin will either need to inspect the class to make sure it derives from typing.Generic and have a separate whitelist for the builtins that provide a __class_getitem__ without inheriting from typing.Generic, such as dict and list

It's been a while since I've looked at Python AST, but I don't think it's possible to do this without loading the file from disk. That would add a significant runtime cost to each file checked. Very happy to be proven wrong though!

or just do what some other plugins do and add a setting for a blacklist or whitelist of classes which should be considered generic only at type checking time.

I guess adding a setting for allowed stringified types would be OK, but it does seem like a lot of work for a relatively small improvement - no offense. I'm not seeing this in my day to day, and since this is the first issue mentioning it I guess few people are affected in general?

That said, do you think it would be worth implementing despite this and/or do you think this type of false positive will become more prevalent? (I know there's some stuff happening with generics in Python 3.12, but have to admit I haven't looked at it properly yet)

Would be happy to accept a PR if you think it'd be worth the time investment. Otherwise, perhaps adding a few noqa's will do for now?

[Daverball]

@sondrelg Yes, it will probably be pretty costly to determine this dynamically and it would involve parsing the source module's code, so a whitelist/blacklist would definitely be a more economical solution.

The simplest solution would be to favor false negatives over false positives, meaning that you won't emit a TC201 if the string literal contains square brackets. I would also be happy enough if it was a setting or if it were split into two separate error codes for the two levels of strictness, so you can either favor false negatives or false positives, depending on your use-case.

Seeing as a false negative on TC201 is completely harmless (it even saves you some runtime cost), while fixing a false positive actually incurs a runtime exception, I would definitely prefer to have false negatives.

[sondrelg]

That's a good point. Nudging people towards runtime exceptions seems bad :+1:

Thinking about it, I cannot think of a reason why allowing string literals with square brackets wouldn't be a sound enough solution. Can you? TC201 is really just a helper to help avoid mixing the two styles of forward-referenced types, but it's not really important to not mix - it's just nice in general.

[Daverball]

@sondrelg Yeah, I agree. I will open a PR.

We can always make it more sophisticated in the future with a user-extensible whitelist of subscriptable types, if we wanted to decrease false negatives, although then you would probably want to take the python version into account for a sane default list and then you're treading into flake8-typing-imports territory.