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

Daverball · 2023-10-07T14:45:17Z

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.

This also adds TC2xx checks for the RHS of an explicit `TypeAlias`

It should behave like an annotation for the purposes of determining typing only imports.

Previously we performed a simple substring match, but this is prone to error for cases like `Foo` vs `FooBar` which are separate names, but would produce false negatives.

sondrelg

Depending on how the new type alias syntax in 3.12 interacts with from future import annotations we could also split this off into separate error codes in the TC0xx range, since at least for the x: TypeAlias = ... case we know the RHS will behave like an annotation with or without the future import, i.e. the expression will be evaluated immediately, so it needs to be wrapped if the symbols do not exist at runtime.

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.

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?

Did a very quick drive-by review, so left a few initial comments/questions, but will return to review properly once you consider the PR done-done 👍 Just ping me

flake8_type_checking/checker.py

sondrelg · 2023-10-08T11:07:49Z

flake8_type_checking/checker.py

+            # mark all the top-level statements
+            for stmt in node.body:
+                setattr(stmt, TOP_LEVEL_PROPERTY, True)


How about we name it TOP_LEVEL_IF_PROPERTY?

That reads to me like the if is top level rather than the statement. I tried to come up with a more descriptive name, but it ends up being too long. Most unambiguous would be something like TYPE_CHECKING_SCOPED_TOP_LEVEL_PROPERTY. If you don't mind it being that long-winded I'm happy to change it.

I realize now that there are a bunch of corner-cases still here, it's quite common to have an if TYPE_CHECKING block inside a class body, so declarations should remain scoped to the class body. It's probably more robust if we mark all the statements in global scope with GLOBAL_PROPERTY. This should be fairly straightforward, although we would need to add some recursion for control flow in global scope, since control flow does not open a new scope, while function/class definitions do.

Then we could combine GLOBAL_PROPERTY and in_type_checking_block to accomplish the same check or add a second marker, like we were doing here, but only if the if is also marked global.

I ended up changing the implementation accordingly and added an extra statement to one of the inverse regression tests to ensure this works.

We may want to consider ignoring imports that aren't in global scope in the same way, although less common it is possible that there's an import inside a function definition to resolve a cyclic dependency, which should probably not count towards the imports we check.

tests/test_tc101.py

Daverball · 2023-10-08T12:07:29Z

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.

Adds new error codes for type alias specific errors.

Fixes unnecessary filtering out of new-style aliases

Daverball · 2023-10-08T15:00:30Z

@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 · 2023-10-10T06:33:10Z

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 👍

Daverball · 2023-10-10T06:39:50Z

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 · 2023-10-16T13:04:40Z

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

sondrelg

Sorry for the delay @Daverball. Really solid work 💪

sondrelg · 2023-10-17T07:19:29Z

flake8_type_checking/constants.py

 TC100 = "TC100 Add 'from __future__ import annotations' import"
 TC101 = "TC101 Annotation '{annotation}' does not need to be a string literal"
 TC200 = "TC200 Annotation '{annotation}' needs to be made into a string literal"
-TC201 = "TC201 Annotation '{annotation}' does not need to be a string"
+TC201 = "TC201 Annotation '{annotation}' does not need to be a string literal"


We should update the readme for this too, if needed 👍

Daverball added 4 commits October 7, 2023 16:39

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

5896c03

This also adds TC2xx checks for the RHS of an explicit `TypeAlias`

Fixes offset in py312 specific test

55e96da

Don't visit the value expression for an explicit TypeAlias

b7d4789

It should behave like an annotation for the purposes of determining typing only imports.

fix: Extracts names from wrapped annotations for matching.

ed10ad6

Previously we performed a simple substring match, but this is prone to error for cases like `Foo` vs `FooBar` which are separate names, but would produce false negatives.

Daverball mentioned this pull request Oct 7, 2023

Fix: extract names from ForwardRef #171

Closed

sondrelg reviewed Oct 8, 2023

View reviewed changes

Daverball added 4 commits October 8, 2023 14:39

Improves name regex for Literal and adds tests.

9d1f7d7

Properly handles 3.12 style type aliases.

a0fa5cc

Adds new error codes for type alias specific errors.

Adds a couple of test cases for type aliases to TC001-TC003 tests

92ac05d

Fixes unnecessary filtering out of new-style aliases

Fixes comment [skip ci]

802482c

Daverball requested a review from sondrelg October 9, 2023 07:17

Daverball added 2 commits October 9, 2023 09:44

Updates a couple of docstrings to better reflect current state

568801a

Improves robustness of detecting type checking only declarations

bcc4654

sondrelg approved these changes Oct 17, 2023

View reviewed changes

sondrelg merged commit dc19495 into snok:main Oct 17, 2023
6 checks passed

Daverball deleted the fix/type-checking-decls branch October 17, 2023 20:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Daverball commented Oct 7, 2023 •

edited

Loading

sondrelg left a comment

sondrelg Oct 8, 2023

Daverball Oct 8, 2023 •

edited

Loading

Daverball Oct 9, 2023 •

edited

Loading

Daverball Oct 9, 2023

Daverball commented Oct 8, 2023 •

edited

Loading

Daverball commented Oct 8, 2023

sondrelg commented Oct 10, 2023

Daverball commented Oct 10, 2023

Daverball commented Oct 16, 2023

sondrelg left a comment

sondrelg Oct 17, 2023

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

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

Conversation

Daverball commented Oct 7, 2023 • edited Loading

sondrelg left a comment

Choose a reason for hiding this comment

sondrelg Oct 8, 2023

Choose a reason for hiding this comment

Daverball Oct 8, 2023 • edited Loading

Choose a reason for hiding this comment

Daverball Oct 9, 2023 • edited Loading

Choose a reason for hiding this comment

Daverball Oct 9, 2023

Choose a reason for hiding this comment

Daverball commented Oct 8, 2023 • edited Loading

Daverball commented Oct 8, 2023

sondrelg commented Oct 10, 2023

Daverball commented Oct 10, 2023

Daverball commented Oct 16, 2023

sondrelg left a comment

Choose a reason for hiding this comment

sondrelg Oct 17, 2023

Choose a reason for hiding this comment

Daverball commented Oct 7, 2023 •

edited

Loading

Daverball Oct 8, 2023 •

edited

Loading

Daverball Oct 9, 2023 •

edited

Loading

Daverball commented Oct 8, 2023 •

edited

Loading