Resolve false negative for used-before-assignment with TYPE_CHECKING guard

[zenlyj]

Type of Changes

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

Description

In the function responsible for filtering nodes defined under a TYPE_CHECKING block from consumption, usage scopes should only be updated when used-before-assignment is reported.

However, there is currently a misalignment between the _report_unfound_name_definition() and the filter function regarding when the error is raised. To resolve this, _report_unfound_name_definition() has been updated to return the check result, which is now passed to the filter function.

Additionally, the check has been extended to handle cases where a class is defined within an if block and used later.

Closes #8893

[zenlyj]

setting this to draft first, as i would like to check this more thoroughly. but feel free to discuss / give feedback!

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.80%. Comparing base (537e9da) to head (16d14e8).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #9990   +/-   ##
=======================================
  Coverage   95.80%   95.80%           
=======================================
  Files         174      174           
  Lines       18937    18939    +2     
=======================================
+ Hits        18143    18145    +2     
  Misses        794      794           

Files with missing lines	Coverage Δ
pylint/checkers/variables.py	97.20% <100.00%> (+<0.01%)	⬆️

[zenlyj]

@jacobtylerwalls can i get a review for this PR?

regarding the primer result, the sentry one seems legitimate. as an import is made under TYPE_CHECKING, then used for an isinstance check. the django ones should be reasonable reports as well? where a class is defined within an if block, but not within the associated else block, hence raising possibly-used-before-assignment

[jacobtylerwalls]

Thank you! Just a small request to handle one more case.

[jacobtylerwalls]

Any idea about the new Django message for a nonlocal?

[zenlyj]

seems that the message was raised due to a guarded function definition further down at line 1584. but from my understanding, this is a false positive? it should not flag as condition is provided as a function parameter in the enclosing function _deferredSkip()

[jacobtylerwalls]

I wonder if we need to explicitly handle nonlocals somewhere. If you don't have bandwidth then I guess we can back out your last change and punt this to a new issue so that we don't cause more false positives.

[zenlyj]

perhaps we should drop the latest commit and track this case with a separate issue? so that we can unblock #9964.

you may assign the issue to me and i'll find time to work on it

[jacobtylerwalls]

Very good--thank you!

[nickdrozd]

There is also the problem of unused-import false negatives:

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from math import pi  # unused-import false negative

Is it possible to fix that along the way?

[jacobtylerwalls]

you may assign the issue to me and i'll find time to work on it

Again, thanks for this offer. I opened #10028

[zenlyj]

you may assign the issue to me and i'll find time to work on it

Again, thanks for this offer. I opened #10028

hey no worries, i dropped the latest commit so this should be good to go.

There is also the problem of unused-import false negatives:

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from math import pi  # unused-import false negative

Is it possible to fix that along the way?

sure thing, i added it to #10028

[github-actions]

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

Effect on django:
The following messages are now emitted:

1. possibly-used-before-assignment:
   Possibly using variable 'ServerBindingCursor' before assignment
   https://github.com/django/django/blob/97c05a64ca87253e9789ebaab4b6d20a1b2370cf/django/db/backends/postgresql/base.py#L280
2. possibly-used-before-assignment:
   Possibly using variable 'ServerSideCursor' before assignment
   https://github.com/django/django/blob/97c05a64ca87253e9789ebaab4b6d20a1b2370cf/django/db/backends/postgresql/base.py#L416

Effect on sentry:
The following messages are now emitted:

1. used-before-assignment:
   Using variable 'GroupEvent' before assignment
   https://github.com/getsentry/sentry/blob/333e1ae42144f2d52a3f3e53b35cd792230f6073/src/sentry/eventstream/base.py#L146

This comment was generated for commit 16d14e8

[Pierre-Sassoulas]

The sentry primer looks legit.