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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement late-binding loop check #265

Merged
merged 5 commits into from Jul 1, 2022

Conversation

Zac-HD
Copy link
Member

@Zac-HD Zac-HD commented Jun 23, 2022

Closes #263.

  • Avoid duplicate warnings when inside nested loops (i.e. dedupe before appending)
  • Only warn on names used in a Load context, e.g. ... for _ in ... is OK in a function, even if _ is also a loop variable
  • Assigning to attributes or indexed items should not be considered an access, e.g. self.count += 1 does not make self a loop variable. I'm generally fine ruling that detecting bugs around attributes is out of scope, but the false-alarms need to be fixed

I've run this over our codebase at work (the motivating example 馃槄), and there are only a few false-positives where the function is used and then discarded within the loop iteration that defines it. These cases are also infeasible to reliably detect, so I'm comfortable leaving them to be handled by either manual var=var capture or # noqa comments.

@Zac-HD Zac-HD force-pushed the late-binding-closures branch 3 times, most recently from ea64354 to 249dc66 Compare June 23, 2022 23:41
i.e. don't complain about the default value of arguments, since that's an explicit solution to late binding!
@Zac-HD Zac-HD requested a review from cooperlees June 24, 2022 08:04
@Zac-HD
Copy link
Member Author

Zac-HD commented Jul 1, 2022

Ping @cooperlees?

Copy link
Collaborator

@cooperlees cooperlees left a comment

Choose a reason for hiding this comment

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

Sorry about the delay. Been busy.

Makes sense to me. Includes Carl's suggestion + well documented + tested

  • Plus links to a great doc for people to read to fully understand (like I did)

I feel this is going to find a lot of cases doing this. So lets release this on it's own and wreak havoc.

@cooperlees cooperlees merged commit aaad1d6 into PyCQA:main Jul 1, 2022
6 checks passed
@Zac-HD Zac-HD deleted the late-binding-closures branch July 1, 2022 18:13
@Zac-HD
Copy link
Member Author

Zac-HD commented Jul 1, 2022

No worries, life happens! Bring on the havoc, we'll see if people are complaining about false-alarms or just alarms they don't like 馃榿

@paw-lu paw-lu mentioned this pull request Jul 2, 2022
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.

Check for late-binding closures (e.g. lambdas) in loops
2 participants