Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

zenlyj
Copy link
Contributor

@zenlyj zenlyj commented Sep 30, 2024

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 zenlyj marked this pull request as draft September 30, 2024 19:56
@jacobtylerwalls jacobtylerwalls added this to the 4.0.0 milestone Sep 30, 2024
@jacobtylerwalls jacobtylerwalls added the False Negative πŸ¦‹ No message is emitted but something is wrong with the code label Sep 30, 2024
@zenlyj zenlyj force-pushed the fix/8893-fn-type-checking-guard branch from 638a16a to 4c5272e Compare September 30, 2024 20:09
@zenlyj
Copy link
Contributor Author

zenlyj commented Sep 30, 2024

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

Copy link

codecov bot commented Sep 30, 2024

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 95.80%. Comparing base (fa64425) to head (ad646cd).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           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%) ⬆️

This comment has been minimized.

@zenlyj zenlyj force-pushed the fix/8893-fn-type-checking-guard branch from 5bf0ee8 to 90983b4 Compare October 1, 2024 10:34

This comment has been minimized.

@zenlyj zenlyj force-pushed the fix/8893-fn-type-checking-guard branch 3 times, most recently from aeb2789 to 6035068 Compare October 1, 2024 11:14

This comment has been minimized.

@zenlyj zenlyj force-pushed the fix/8893-fn-type-checking-guard branch from 6035068 to b404520 Compare October 5, 2024 17:34

This comment has been minimized.

@zenlyj zenlyj force-pushed the fix/8893-fn-type-checking-guard branch from b404520 to 95337f3 Compare October 5, 2024 18:59

This comment has been minimized.

This comment has been minimized.

@zenlyj zenlyj force-pushed the fix/8893-fn-type-checking-guard branch 4 times, most recently from 75f1fb4 to 4733d0f Compare October 6, 2024 10:29

This comment has been minimized.

@zenlyj zenlyj force-pushed the fix/8893-fn-type-checking-guard branch from 4733d0f to a3573a2 Compare October 6, 2024 13:03

This comment has been minimized.

@zenlyj zenlyj force-pushed the fix/8893-fn-type-checking-guard branch from a3573a2 to a27ad84 Compare October 13, 2024 16:02

This comment has been minimized.

@zenlyj zenlyj marked this pull request as ready for review October 13, 2024 17:28
@zenlyj
Copy link
Contributor Author

zenlyj commented Oct 13, 2024

@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

Copy link
Member

@jacobtylerwalls jacobtylerwalls left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pylint/checkers/variables.py Outdated Show resolved Hide resolved
tests/functional/u/used/used_before_assignment_typing.py Outdated Show resolved Hide resolved
Copy link
Contributor

πŸ€– Effect of this PR on checked open source code: πŸ€–

Effect on home-assistant:
The following messages are now emitted:

  1. possibly-used-before-assignment:
    Possibly using variable 'touch_event_callback' before assignment
    https://github.com/home-assistant/core/blob/8ba14ef113a072650c8abce26da9290872eb8192/homeassistant/components/nanoleaf/__init__.py#L77

Effect on django:
The following messages are now emitted:

  1. possibly-used-before-assignment:
    Possibly using variable 'condition' before assignment
    https://github.com/django/django/blob/cee95e6172f2ea3dbf8cfd4214562cfc92f95392/django/test/testcases.py#L1573
  2. possibly-used-before-assignment:
    Possibly using variable 'ServerBindingCursor' before assignment
    https://github.com/django/django/blob/cee95e6172f2ea3dbf8cfd4214562cfc92f95392/django/db/backends/postgresql/base.py#L280
  3. possibly-used-before-assignment:
    Possibly using variable 'ServerSideCursor' before assignment
    https://github.com/django/django/blob/cee95e6172f2ea3dbf8cfd4214562cfc92f95392/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/416d9084ac0be3de94b43ba0b2d93b6c394cb4a4/src/sentry/eventstream/base.py#L146

This comment was generated for commit ad646cd

@jacobtylerwalls
Copy link
Member

Any idea about the new Django message for a nonlocal?

@zenlyj
Copy link
Contributor Author

zenlyj commented Oct 14, 2024

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
Copy link
Member

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
Copy link
Contributor Author

zenlyj commented Oct 14, 2024

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
Copy link
Member

Very good--thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
False Negative πŸ¦‹ No message is emitted but something is wrong with the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

false negative: used-before-assignment w/ TYPE_CHECKING guard
2 participants