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

fix: Avoid TC201 false positives for top-level type checking decls #170

Closed Daverball closed 1 year ago

Daverball commented 1 year ago

Fixes #168

This also adds checks and new error codes for the RHS of an explicit TypeAlias. This also improves accuracy of matching against string annotations by extracting the names from the annotation.

Daverball commented 1 year ago

Seems like we could re-use TC101 for the TC1XX error range, but we might need to add a TC203 since users opt into just one of them.

True, but then we wouldn't have a matching error code for the opposite case where you do actually do need to quote it. If we end up having to add new errors for type aliases anyways it might make sense to separate type aliases out into their own two lists and generate new errors for them, but that does to some degree depend on how the new type alias syntax behaves. If it behaves like a regular annotation, then we would only need to special case x: TypeAlias = ... and could treat type x = ... like a normal annotation.

So just so I'm on the same page here. What we're trying to achieve here is to add assignments to type aliases that happen inside a TYPE_CHECKING block, to our list of non-runtime scoped annotations. This way we can raise valid error codes for uses of type aliases. Correct, or am I missing one or more details?

That's one of the two goals, it just happened to align with the other goal of remembering the top-level declarations inside if TYPE_CHECKING blocks, since those need to be treated like they don't exist at runtime as well, just like an import, i.e. things like type aliases or type vars defined inside a type checking block.

But yes, I also wanted to treat the RHS of an explicit alias like an annotation (not just the ones inside type checking blocks), to the degree it is possible, so we can improve the accuracy of the errors. Otherwise defining x: TypeAlias = Foo would imply that Foo is a runtime dependency we can't get rid of, but we can, if we wrap the RHS, just like with an annotation.

Daverball commented 1 year ago

@sondrelg Alright I think this should be ready for review. I separated out the type alias related errors into their own error codes TC007 and TC008, while the former is only relevant for old-style type aliases, the new syntax can only emit a TC007 (basically unconditionally when it is wrapped at all, since it always will be a ForwardRef, regardless of future import)

sondrelg commented 1 year ago

Sorry for the delay here. A bit busy this week, but will hopefully get to it in a day or two. End of the weekend at the latest :+1:

Daverball commented 1 year ago

Sorry for the delay here. A bit busy this week, but will hopefully get to it in a day or two. End of the weekend at the latest 👍

No worries, I ended up catching a potential problem with the implementation because of it. There's never any harm in letting your subconscious work on the problem for a few extra days.

Daverball commented 1 year ago

@sondrelg Just pinging you again in case this slipped through the cracks.