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 used-before-assignment false negative when a function is defined under TYPE_CHECKING #10034

Merged
merged 6 commits into from
Nov 4, 2024

Conversation

zenlyj
Copy link
Contributor

@zenlyj zenlyj commented Oct 19, 2024

Type of Changes

Type
βœ“ πŸ› Bug fix
✨ New feature
πŸ”¨ Refactoring
πŸ“œ Docs

Description

Raise used-before-assignment when a function is defined under TYPE_CHECKING and used later.

Also extending the nonlocal handling to resolve the following false positive that was discovered by the primer tests, where a variable usage references a nonlocal binding from an outer, enclosing frame.

These changes mainly include a search for nonlocal declarations in all enclosing frames of the variable usage. The idea is to rely on existing nonlocal checks and to only check outer frames if all nodes are filtered out by the consumer.

Refs #10028

@zenlyj zenlyj marked this pull request as draft October 19, 2024 07:20
Copy link

codecov bot commented Oct 19, 2024

Codecov Report

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

Please upload report for BASE (main@15a5ac0). Learn more about missing BASE report.
Report is 5 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #10034   +/-   ##
=======================================
  Coverage        ?   95.80%           
=======================================
  Files           ?      174           
  Lines           ?    18956           
  Branches        ?        0           
=======================================
  Hits            ?    18160           
  Misses          ?      796           
  Partials        ?        0           
Files with missing lines Coverage Ξ”
pylint/checkers/variables.py 97.24% <100.00%> (ΓΈ)

This comment has been minimized.

@jacobtylerwalls jacobtylerwalls added this to the 4.0.0 milestone Oct 19, 2024

This comment has been minimized.

@zenlyj zenlyj force-pushed the fix/10028-fn branch 2 times, most recently from cab818f to 86d91b6 Compare October 22, 2024 10:52

This comment has been minimized.

@jacobtylerwalls
Copy link
Member

Thanks so much @zenlyj, is this ready for review?

@zenlyj
Copy link
Contributor Author

zenlyj commented Oct 31, 2024

i don't think the current approach is correct, it silences the check in many cases and something like this is not flagged even though it would blow up in runtime:

def outer(callback):
    def inner():
        nonlocal callback
        def inner2():
            callback()  # this should raise used-before-assignment
            def callback():
                pass
        inner2()
    inner()

def foo():
    print('foo')

outer(foo)

This comment has been minimized.

pylint/checkers/variables.py Outdated Show resolved Hide resolved

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@zenlyj zenlyj marked this pull request as ready for review November 2, 2024 13:52
@zenlyj
Copy link
Contributor Author

zenlyj commented Nov 2, 2024

@jacobtylerwalls this is ready for review. let me know if there is anything i missed here, thank you!

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

LGTM, thank you, great work. Do you mind adding a fragment for the changelog ? (I'll also let Jacob review before merging).

This comment has been minimized.

@jacobtylerwalls
Copy link
Member

I'll handle the Home Assistant false positive in another PR.

@zenlyj
Copy link
Contributor Author

zenlyj commented Nov 3, 2024

hey @jacobtylerwalls, i was just thinking about this case:

def nonlocal_in_outer_frame_ok(callback, condition_a, condition_b):
    def outer():
        nonlocal callback
        def inner():
            if condition_a:
                def inner2():
                    callback()  # possibly-used-before-assignment?
                inner2()
            else:
                if condition_b:
                    def callback():
                        pass
        inner()
    outer()

we should probably raise a message here?

@jacobtylerwalls
Copy link
Member

we should probably raise a message here?

Good catch, I'm fine with opening a new issue for it. I think it should be real UBA, not "possibly".

@zenlyj
Copy link
Contributor Author

zenlyj commented Nov 3, 2024

it is worth noting that prior to my changes in this PR, we're emitting possibly-used-before-assignment for this snippet. but i feel like this will be tough to crack as it requires some form of control flow analysis?

Copy link
Contributor

github-actions bot commented Nov 3, 2024

πŸ€– 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/b09e54c961db279785b75b5c3d192624b3d65664/homeassistant/components/nanoleaf/__init__.py#L77

This comment was generated for commit 3808c4e

@Pierre-Sassoulas
Copy link
Member

Hello @zenlyj. You've been doing a lot of great work for pylint lately, including this one, would you be interested in becoming a triager ? (I would normally have asked you privately but your commit mail is a no reply :) )

@zenlyj
Copy link
Contributor Author

zenlyj commented Nov 3, 2024

Hello @zenlyj. You've been doing a lot of great work for pylint lately, including this one, would you be interested in becoming a triager ? (I would normally have asked you privately but your commit mail is a no reply :) )

hello, i just dropped you an email, let's talk there

@Pierre-Sassoulas Pierre-Sassoulas merged commit 5c59b48 into pylint-dev:main Nov 4, 2024
44 checks passed
@zenlyj zenlyj deleted the fix/10028-fn branch November 9, 2024 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[used-before-assignment] Functions defined in type-checking guard not flagged when used later
4 participants