Fix used-before-assignment TYPE_CHECKING false negatives

[zenlyj]

Type of Changes

	Type
✓	🐛 Bug fix
	✨ New feature
	🔨 Refactoring
	📜 Docs

Description

This PR addresses false negative cases for used-before-assignment, when imports are made within TYPE_CHECKING guard clauses.

The root cause of these false negatives is the consumption of Import, ImportFrom nodes within TYPE_CHECKING clauses. Upon consumption, subsequent usages are not evaluated by the checker.

My approach is to not consume these nodes and keep track of usage scopes, so that errors can be emitted in multiple scopes.

Feel free to discuss alternative solutions! 🙂

Closes #8198

[codecov]

Codecov Report

Merging #8431 (1e4990f) into main (b63c8a1) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8431   +/-   ##
=======================================
  Coverage   95.91%   95.91%           
=======================================
  Files         174      174           
  Lines       18368    18389   +21     
=======================================
+ Hits        17617    17638   +21     
  Misses        751      751           

Impacted Files	Coverage Δ
pylint/checkers/variables.py	97.20% <100.00%> (+0.03%)	⬆️

... and 5 files with indirect coverage changes

[jacobtylerwalls]

Hey, thanks for jumping on this! Mainly I'm wondering if what happens if we use in_type_checking_block, which has seen much more development. Is the implementation any simpler?

[jacobtylerwalls]

I'm also sort of wondering if the variables checker shouldn't be using the typing guard at all. We have a concept in the variables checker of names that are guarded under "always false" tests and thus not available at runtime. This seems to raise the warning we want, although I don't know what effect it has on unused-variable:

diff --git a/pylint/checkers/variables.py b/pylint/checkers/variables.py
index 734231103..a0016606e 100644
--- a/pylint/checkers/variables.py
+++ b/pylint/checkers/variables.py
@@ -780,10 +780,11 @@ scope_type : {self._atomic.scope_type}
         """
         uncertain_nodes = []
         for other_node in found_nodes:
-            if in_type_checking_block(other_node):
-                continue
-
-            if not isinstance(other_node, nodes.AssignName):
+            if isinstance(other_node, nodes.AssignName):
+                name = other_node.name
+            elif isinstance(other_node, (nodes.Import, nodes.ImportFrom)):
+                name = node.name
+            else:
                 continue
 
             closest_if = utils.get_node_first_ancestor_of_type(other_node, nodes.If)
@@ -796,7 +797,7 @@ scope_type : {self._atomic.scope_type}
 
             # Name defined in every if/else branch
             if NamesConsumer._exhaustively_define_name_raise_or_return(
-                other_node.name, closest_if
+                name, closest_if
             ):
                 continue

[Pierre-Sassoulas]

Could you rebase on the latest main, please ?

[zenlyj]

I'm on it, made a mistake while trying to rebase and squash to tidy up the log

[zenlyj]

Let me know if there is anything that needs to be updated according to the latest primer results.

[Pierre-Sassoulas]

Neat change ! Sorry for the delay in reviewing, I was conferencing and teaching the youngling.

Didn't look closely at the primer results and no time to do it atm.

[jacobtylerwalls]

Thanks for fixing this difficult false negative! Nice refactors also.

Didn't look closely at the primer results and no time to do it atm.

The last one is a real bug in sentry. The others are already captured in #8167, which we have some time to fix before 3.0.

[github-actions]

🤖 Effect of this PR on checked open source code: 🤖

Effect on music21:
The following messages are now emitted:

1. used-before-assignment:
   Using variable 'stream' before assignment
   https://github.com/cuthbertLab/music21/blob/f05ec0af047977eaa8a2c09f7cf99892a5983eb7/music21/stream/iterator.py#L1844

Effect on pandas:
The following messages are now emitted:

1. used-before-assignment:
   Using variable 'groupby' before assignment
   https://github.com/pandas-dev/pandas/blob/b661313cf5c9f5543c393d85339bb82410c18b7b/pandas/core/groupby/indexing.py#L232
2. used-before-assignment:
   Using variable 'groupby' before assignment
   https://github.com/pandas-dev/pandas/blob/b661313cf5c9f5543c393d85339bb82410c18b7b/pandas/core/groupby/indexing.py#L241

Effect on sentry:
The following messages are now emitted:

1. used-before-assignment:
   Using variable 'AuthenticatedToken' before assignment
   https://github.com/getsentry/sentry/blob/73418d502e8b12b877d90fc7d87c787a0139a13c/src/sentry/ratelimits/utils.py#L83

This comment was generated for commit 1e4990f