Skip to content

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
@codecov
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 67 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%> (ΓΈ)

@github-actions

This comment has been minimized.

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

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
@github-actions

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)

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

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).

@github-actions

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?

@github-actions
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