ruff: Enable B023 Function definition does not bind loop variable.

Python’s loop scoping is misdesigned, resulting in a very common
gotcha for functions that close over loop variables [1].  The general
problem is so bad that even the Go developers plan to break
compatibility in order to fix the same design mistake in their
language [2].

Enable the Ruff rule function-uses-loop-variable (B023) [3], which
conservatively prohibits functions from binding loop variables at all.

[1] https://docs.python-guide.org/writing/gotchas/#late-binding-closures
[2] https://go.dev/s/loopvar-design
[3] https://beta.ruff.rs/docs/rules/function-uses-loop-variable/

Signed-off-by: Anders Kaseorg <anders@zulip.com>

analytics/views/installation_activity.py

@@ -73,26 +73,24 @@ def get_realm_day_counts() -> Dict[str, Dict[str, Markup]]:
     for row in rows:
         counts[row["string_id"]][row["age"]] = row["cnt"]
 
+    def format_count(cnt: int, style: Optional[str] = None) -> Markup:
+        if style is not None:
+            good_bad = style
+        elif cnt == min_cnt:
+            good_bad = "bad"
+        elif cnt == max_cnt:
+            good_bad = "good"
+        else:
+            good_bad = "neutral"
+
+        return Markup('<td class="number {good_bad}">{cnt}</td>').format(good_bad=good_bad, cnt=cnt)
+
     result = {}
     for string_id in counts:
         raw_cnts = [counts[string_id].get(age, 0) for age in range(8)]
         min_cnt = min(raw_cnts[1:])
         max_cnt = max(raw_cnts[1:])
 
-        def format_count(cnt: int, style: Optional[str] = None) -> Markup:
-            if style is not None:
-                good_bad = style
-            elif cnt == min_cnt:
-                good_bad = "bad"
-            elif cnt == max_cnt:
-                good_bad = "good"
-            else:
-                good_bad = "neutral"
-
-            return Markup('<td class="number {good_bad}">{cnt}</td>').format(
-                good_bad=good_bad, cnt=cnt
-            )
-
         cnts = format_count(raw_cnts[0], "neutral") + Markup().join(map(format_count, raw_cnts[1:]))
         result[string_id] = dict(cnts=cnts)
 

pyproject.toml

@@ -134,7 +134,6 @@ ignore = [
     "ANN401", # Dynamically typed expressions (typing.Any) are disallowed
     "B007", # Loop control variable not used within the loop body
     "B008", # Do not perform function calls in argument defaults
-    "B023", # Function definition does not bind loop variable
     "B904", # Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
     "C408", # Unnecessary `dict` call (rewrite as a literal)
     "COM812", # Trailing comma missing

tools/lib/pretty_print.py

@@ -107,6 +107,10 @@ def adjust_block_indentation(tokens: List[Token], fn: str) -> None:
 
 
 def fix_indents_for_multi_line_tags(tokens: List[Token]) -> None:
+    def fix(frag: str) -> str:
+        frag = frag.strip()
+        return continue_indent + frag if frag else ""
+
     for token in tokens:
         if token.kind == "code":
             continue
@@ -121,10 +125,6 @@ def fix_indents_for_multi_line_tags(tokens: List[Token]) -> None:
 
         frags = token.new_s.split("\n")
 
-        def fix(frag: str) -> str:
-            frag = frag.strip()
-            return continue_indent + frag if frag else ""
-
         token.new_s = frags[0] + "\n" + "\n".join(fix(frag) for frag in frags[1:])
 
 

tools/lib/template_parser.py

@@ -489,6 +489,20 @@ def validate(fn: Optional[str] = None, text: Optional[str] = None) -> List[Token
 
 
 def ensure_matching_indentation(fn: str, tokens: List[Token], lines: List[str]) -> None:
+    def has_bad_indentation() -> bool:
+        is_inline_tag = start_tag in HTML_INLINE_TAGS and start_token.kind == "html_start"
+
+        if end_line > start_line + 1:
+            if is_inline_tag:
+                end_row_text = lines[end_line - 1]
+                if end_row_text.lstrip().startswith(end_token.s) and end_col != start_col:
+                    return True
+            else:
+                if end_col != start_col:
+                    return True
+
+        return False
+
     for token in tokens:
         if token.start_token is None:
             continue
@@ -503,20 +517,6 @@ def ensure_matching_indentation(fn: str, tokens: List[Token], lines: List[str])
         end_line = end_token.line
         end_col = end_token.col
 
-        def has_bad_indentation() -> bool:
-            is_inline_tag = start_tag in HTML_INLINE_TAGS and start_token.kind == "html_start"
-
-            if end_line > start_line + 1:
-                if is_inline_tag:
-                    end_row_text = lines[end_line - 1]
-                    if end_row_text.lstrip().startswith(end_token.s) and end_col != start_col:
-                        return True
-                else:
-                    if end_col != start_col:
-                        return True
-
-            return False
-
         if has_bad_indentation():
             raise TemplateParserError(
                 f"""

zerver/lib/subscription_info.py

@@ -316,6 +316,9 @@ def bulk_get_subscriber_user_ids(
 ) -> Dict[int, List[int]]:
     """sub_dict maps stream_id => whether the user is subscribed to that stream."""
     target_stream_dicts = []
+    is_subscribed: bool
+    check_user_subscribed = lambda user_profile: is_subscribed
+
     for stream_dict in stream_dicts:
         stream_id = stream_dict["id"]
         is_subscribed = stream_id in subscribed_stream_ids
@@ -324,7 +327,7 @@ def bulk_get_subscriber_user_ids(
             validate_user_access_to_subscribers_helper(
                 user_profile,
                 stream_dict,
-                lambda user_profile: is_subscribed,
+                check_user_subscribed,
             )
         except JsonableError:
             continue

zerver/management/commands/check_redis.py

@@ -4,6 +4,7 @@ from typing import Any, Callable, Optional
 
 from django.conf import settings
 from django.core.management.base import BaseCommand, CommandError, CommandParser
+from returns.curry import partial
 
 from zerver.lib.rate_limiter import RateLimitedUser, client
 from zerver.models import get_user_profile_by_id
@@ -61,7 +62,7 @@ than max_api_calls! (trying to trim) %s %s",
 
         lists = client.keys(wildcard_list)
         for list_name in lists:
-            self._check_within_range(list_name, lambda: client.llen(list_name), trim_func)
+            self._check_within_range(list_name, partial(client.llen, list_name), trim_func)
 
         zsets = client.keys(wildcard_zset)
         for zset in zsets:
@@ -70,5 +71,7 @@ than max_api_calls! (trying to trim) %s %s",
             # elements to trim. We'd have to go through every list item and take
             # the intersection. The best we can do is expire it
             self._check_within_range(
-                zset, lambda: client.zcount(zset, 0, now), lambda key, max_calls: None
+                zset,
+                partial(client.zcount, zset, 0, now),
+                lambda key, max_calls: None,
             )

zerver/tests/test_events.py

@@ -14,6 +14,7 @@ from unittest import mock
 import orjson
 from dateutil.parser import parse as dateparser
 from django.utils.timezone import now as timezone_now
+from returns.curry import partial
 
 from zerver.actions.alert_words import do_add_alert_words, do_remove_alert_words
 from zerver.actions.bots import (
@@ -457,21 +458,21 @@ class NormalActionsTest(BaseAction):
         for i in range(3):
             content = "mentioning... @**" + user.full_name + "** hello " + str(i)
             self.verify_action(
-                lambda: self.send_stream_message(self.example_user("cordelia"), "Verona", content),
+                partial(self.send_stream_message, self.example_user("cordelia"), "Verona", content),
             )
 
     def test_topic_wildcard_mentioned_send_message_events(self) -> None:
         for i in range(3):
             content = "mentioning... @**topic** hello " + str(i)
             self.verify_action(
-                lambda: self.send_stream_message(self.example_user("cordelia"), "Verona", content),
+                partial(self.send_stream_message, self.example_user("cordelia"), "Verona", content),
             )
 
     def test_stream_wildcard_mentioned_send_message_events(self) -> None:
         for i in range(3):
             content = "mentioning... @**all** hello " + str(i)
             self.verify_action(
-                lambda: self.send_stream_message(self.example_user("cordelia"), "Verona", content),
+                partial(self.send_stream_message, self.example_user("cordelia"), "Verona", content),
             )
 
     def test_pm_send_message_events(self) -> None:
@@ -747,12 +748,12 @@ class NormalActionsTest(BaseAction):
             )
 
             self.verify_action(
-                lambda: do_update_message_flags(user_profile, "add", "read", [message]),
+                partial(do_update_message_flags, user_profile, "add", "read", [message]),
                 state_change_expected=True,
             )
 
             events = self.verify_action(
-                lambda: do_update_message_flags(user_profile, "remove", "read", [message]),
+                partial(do_update_message_flags, user_profile, "remove", "read", [message]),
                 state_change_expected=True,
             )
             check_update_message_flags_remove("events[0]", events[0])
@@ -761,12 +762,14 @@ class NormalActionsTest(BaseAction):
                 from_user=user_profile, to_user=self.example_user("cordelia"), content=content
             )
             self.verify_action(
-                lambda: do_update_message_flags(user_profile, "add", "read", [personal_message]),
+                partial(do_update_message_flags, user_profile, "add", "read", [personal_message]),
                 state_change_expected=True,
             )
 
             events = self.verify_action(
-                lambda: do_update_message_flags(user_profile, "remove", "read", [personal_message]),
+                partial(
+                    do_update_message_flags, user_profile, "remove", "read", [personal_message]
+                ),
                 state_change_expected=True,
             )
             check_update_message_flags_remove("events[0]", events[0])
@@ -778,12 +781,12 @@ class NormalActionsTest(BaseAction):
             )
 
             self.verify_action(
-                lambda: do_update_message_flags(user_profile, "add", "read", [huddle_message]),
+                partial(do_update_message_flags, user_profile, "add", "read", [huddle_message]),
                 state_change_expected=True,
             )
 
             events = self.verify_action(
-                lambda: do_update_message_flags(user_profile, "remove", "read", [huddle_message]),
+                partial(do_update_message_flags, user_profile, "remove", "read", [huddle_message]),
                 state_change_expected=True,
             )
             check_update_message_flags_remove("events[0]", events[0])
@@ -1712,8 +1715,11 @@ class NormalActionsTest(BaseAction):
         ):
             with fake_backends():
                 events = self.verify_action(
-                    lambda: do_set_realm_authentication_methods(
-                        self.user_profile.realm, auth_method_dict, acting_user=None
+                    partial(
+                        do_set_realm_authentication_methods,
+                        self.user_profile.realm,
+                        auth_method_dict,
+                        acting_user=None,
                     )
                 )
 
@@ -1727,8 +1733,14 @@ class NormalActionsTest(BaseAction):
         )
         for pinned in (True, False):
             events = self.verify_action(
-                lambda: do_change_subscription_property(
-                    self.user_profile, sub, stream, "pin_to_top", pinned, acting_user=None
+                partial(
+                    do_change_subscription_property,
+                    self.user_profile,
+                    sub,
+                    stream,
+                    "pin_to_top",
+                    pinned,
+                    acting_user=None,
                 )
             )
             check_subscription_update(
@@ -1795,8 +1807,14 @@ class NormalActionsTest(BaseAction):
             # First test with notification_settings_null enabled
             for value in (True, False):
                 events = self.verify_action(
-                    lambda: do_change_subscription_property(
-                        self.user_profile, sub, stream, setting_name, value, acting_user=None
+                    partial(
+                        do_change_subscription_property,
+                        self.user_profile,
+                        sub,
+                        stream,
+                        setting_name,
+                        value,
+                        acting_user=None,
                     ),
                     notification_settings_null=True,
                 )
@@ -1809,8 +1827,14 @@ class NormalActionsTest(BaseAction):
 
             for value in (True, False):
                 events = self.verify_action(
-                    lambda: do_change_subscription_property(
-                        self.user_profile, sub, stream, setting_name, value, acting_user=None
+                    partial(
+                        do_change_subscription_property,
+                        self.user_profile,
+                        sub,
+                        stream,
+                        setting_name,
+                        value,
+                        acting_user=None,
                     )
                 )
                 check_subscription_update(
@@ -1825,7 +1849,8 @@ class NormalActionsTest(BaseAction):
 
         for notifications_stream, notifications_stream_id in ((stream, stream.id), (None, -1)):
             events = self.verify_action(
-                lambda: do_set_realm_notifications_stream(
+                partial(
+                    do_set_realm_notifications_stream,
                     self.user_profile.realm,
                     notifications_stream,
                     notifications_stream_id,
@@ -1842,7 +1867,8 @@ class NormalActionsTest(BaseAction):
             (None, -1),
         ):
             events = self.verify_action(
-                lambda: do_set_realm_signup_notifications_stream(
+                partial(
+                    do_set_realm_signup_notifications_stream,
                     self.user_profile.realm,
                     signup_notifications_stream,
                     signup_notifications_stream_id,
@@ -1871,7 +1897,7 @@ class NormalActionsTest(BaseAction):
                 num_events = 5
 
             events = self.verify_action(
-                lambda: do_change_user_role(self.user_profile, role, acting_user=None),
+                partial(do_change_user_role, self.user_profile, role, acting_user=None),
                 num_events=num_events,
             )
             check_realm_user_update("events[0]", events[0], "role")
@@ -1919,7 +1945,7 @@ class NormalActionsTest(BaseAction):
             else:
                 num_events = 5
             events = self.verify_action(
-                lambda: do_change_user_role(self.user_profile, role, acting_user=None),
+                partial(do_change_user_role, self.user_profile, role, acting_user=None),
                 num_events=num_events,
             )
             check_realm_user_update("events[0]", events[0], "role")
@@ -1947,7 +1973,7 @@ class NormalActionsTest(BaseAction):
         do_change_user_role(self.user_profile, UserProfile.ROLE_MEMBER, acting_user=None)
         for role in [UserProfile.ROLE_MODERATOR, UserProfile.ROLE_MEMBER]:
             events = self.verify_action(
-                lambda: do_change_user_role(self.user_profile, role, acting_user=None),
+                partial(do_change_user_role, self.user_profile, role, acting_user=None),
                 num_events=4,
             )
             check_realm_user_update("events[0]", events[0], "role")
@@ -1982,7 +2008,7 @@ class NormalActionsTest(BaseAction):
             else:
                 num_events = 5
             events = self.verify_action(
-                lambda: do_change_user_role(self.user_profile, role, acting_user=None),
+                partial(do_change_user_role, self.user_profile, role, acting_user=None),
                 num_events=num_events,
             )
             check_realm_user_update("events[0]", events[0], "role")
@@ -2028,7 +2054,8 @@ class NormalActionsTest(BaseAction):
 
             for setting_value in [True, False]:
                 events = self.verify_action(
-                    lambda: do_change_user_setting(
+                    partial(
+                        do_change_user_setting,
                         self.user_profile,
                         notification_setting,
                         setting_value,
@@ -2042,7 +2069,8 @@ class NormalActionsTest(BaseAction):
 
                 # Also test with notification_settings_null=True
                 events = self.verify_action(
-                    lambda: do_change_user_setting(
+                    partial(
+                        do_change_user_setting,
                         self.user_profile,
                         notification_setting,
                         setting_value,
@@ -2070,8 +2098,12 @@ class NormalActionsTest(BaseAction):
 
         for val in [True, False]:
             events = self.verify_action(
-                lambda: do_change_user_setting(
-                    self.user_profile, presence_enabled_setting, val, acting_user=self.user_profile
+                partial(
+                    do_change_user_setting,
+                    self.user_profile,
+                    presence_enabled_setting,
+                    val,
+                    acting_user=self.user_profile,
                 ),
                 num_events=3,
             )
@@ -2545,7 +2577,7 @@ class NormalActionsTest(BaseAction):
 
             stream = self.make_stream(old_name)
             self.subscribe(self.user_profile, stream.name)
-            action = lambda: do_rename_stream(stream, new_name, self.user_profile)
+            action = partial(do_rename_stream, stream, new_name, self.user_profile)
             events = self.verify_action(action, num_events=3, include_streams=include_streams)
 
             check_stream_update("events[0]", events[0])
@@ -2574,14 +2606,14 @@ class NormalActionsTest(BaseAction):
     def test_deactivate_stream_neversubscribed(self) -> None:
         for i, include_streams in enumerate([True, False]):
             stream = self.make_stream(f"stream{i}")
-            action = lambda: do_deactivate_stream(stream, acting_user=None)
+            action = partial(do_deactivate_stream, stream, acting_user=None)
             events = self.verify_action(action, include_streams=include_streams)
             check_stream_delete("events[0]", events[0])
             self.assertIsNone(events[0]["streams"][0]["stream_weekly_traffic"])
 
     def test_subscribe_other_user_never_subscribed(self) -> None:
         for i, include_streams in enumerate([True, False]):
-            action = lambda: self.subscribe(self.example_user("othello"), f"test_stream{i}")
+            action = partial(self.subscribe, self.example_user("othello"), f"test_stream{i}")
             events = self.verify_action(action, num_events=2, include_streams=True)
             check_subscription_peer_add("events[1]", events[1])
 
@@ -2938,8 +2970,12 @@ class RealmPropertyActionTest(BaseAction):
             num_events = 1
 
             events = self.verify_action(
-                lambda: do_set_realm_property(
-                    self.user_profile.realm, name, val, acting_user=self.user_profile
+                partial(
+                    do_set_realm_property,
+                    self.user_profile.realm,
+                    name,
+                    val,
+                    acting_user=self.user_profile,
                 ),
                 state_change_expected=state_change_expected,
                 num_events=num_events,
@@ -3009,7 +3045,8 @@ class RealmPropertyActionTest(BaseAction):
             new_group_id = user_group.id
 
             events = self.verify_action(
-                lambda: do_change_realm_permission_group_setting(
+                partial(
+                    do_change_realm_permission_group_setting,
                     self.user_profile.realm,
                     setting_name,
                     user_group,
@@ -3089,8 +3126,12 @@ class RealmPropertyActionTest(BaseAction):
             now = timezone_now()
             state_change_expected = True
             events = self.verify_action(
-                lambda: do_set_realm_user_default_setting(
-                    realm_user_default, name, val, acting_user=self.user_profile
+                partial(
+                    do_set_realm_user_default_setting,
+                    realm_user_default,
+                    name,
+                    val,
+                    acting_user=self.user_profile,
                 ),
                 state_change_expected=state_change_expected,
             )
@@ -3174,8 +3215,12 @@ class UserDisplayActionTest(BaseAction):
                     num_events = 3
 
             events = self.verify_action(
-                lambda: do_change_user_setting(
-                    self.user_profile, setting_name, value, acting_user=self.user_profile
+                partial(
+                    do_change_user_setting,
+                    self.user_profile,
+                    setting_name,
+                    value,
+                    acting_user=self.user_profile,
                 ),
                 num_events=num_events,
                 user_settings_object=user_settings_object,
@@ -3201,8 +3246,12 @@ class UserDisplayActionTest(BaseAction):
 
         for value in values:
             events = self.verify_action(
-                lambda: do_change_user_setting(
-                    self.user_profile, "timezone", value, acting_user=self.user_profile
+                partial(
+                    do_change_user_setting,
+                    self.user_profile,
+                    "timezone",
+                    value,
+                    acting_user=self.user_profile,
                 ),
                 num_events=num_events,
             )

zerver/tests/test_invite.py

@@ -14,6 +14,7 @@ from django.http import HttpRequest
 from django.test import override_settings
 from django.urls import reverse
 from django.utils.timezone import now as timezone_now
+from returns.curry import partial
 
 from confirmation import settings as confirmation_settings
 from confirmation.models import (
@@ -1116,7 +1117,7 @@ so we didn't send them an invitation. We did send invitations to everyone else!"
         for email in existing:
             self.assertRaises(
                 PreregistrationUser.DoesNotExist,
-                lambda: PreregistrationUser.objects.get(email=email),
+                partial(PreregistrationUser.objects.get, email=email),
             )
         for email in new:
             self.assertTrue(PreregistrationUser.objects.get(email=email))

zerver/tests/test_upload.py

@@ -1350,20 +1350,20 @@ class EmojiTest(UploadSerializeMixin, ZulipTestCase):
         with self.assertRaises(BadImageError):
             resize_emoji(corrupted_img_data)
 
+        def test_resize(size: int = 50) -> None:
+            resized_img_data, is_animated, still_img_data = resize_emoji(
+                animated_large_img_data, size=50
+            )
+            im = Image.open(io.BytesIO(resized_img_data))
+            self.assertEqual((size, size), im.size)
+            self.assertTrue(is_animated)
+            assert still_img_data
+            still_image = Image.open(io.BytesIO(still_img_data))
+            self.assertEqual((50, 50), still_image.size)
+
         for img_format in ("gif", "png"):
             animated_large_img_data = read_test_image_file(f"animated_large_img.{img_format}")
 
-            def test_resize(size: int = 50) -> None:
-                resized_img_data, is_animated, still_img_data = resize_emoji(
-                    animated_large_img_data, size=50
-                )
-                im = Image.open(io.BytesIO(resized_img_data))
-                self.assertEqual((size, size), im.size)
-                self.assertTrue(is_animated)
-                assert still_img_data
-                still_image = Image.open(io.BytesIO(still_img_data))
-                self.assertEqual((50, 50), still_image.size)
-
             # Test an image larger than max is resized
             with patch("zerver.lib.upload.base.MAX_EMOJI_GIF_SIZE", 128):
                 test_resize()

zerver/tornado/django_api.py

@@ -9,6 +9,7 @@ from django.conf import settings
 from django.db import transaction
 from requests.adapters import ConnectionError, HTTPAdapter
 from requests.models import PreparedRequest, Response
+from returns.curry import partial
 from urllib3.util import Retry
 
 from zerver.lib.queue import queue_json_publish
@@ -185,7 +186,7 @@ def send_event(
         queue_json_publish(
             notify_tornado_queue_name(port),
             dict(event=event, users=port_users),
-            lambda *args, **kwargs: send_notification_http(port, *args, **kwargs),
+            partial(send_notification_http, port),
         )
 
 

zilencer/management/commands/queue_rate.py

@@ -70,10 +70,7 @@ class Command(BaseCommand):
                         lambda: queue_json_publish(queue_name, {}),
                         number=count,
                     )
-                    duration = timeit(
-                        lambda: worker.start(),
-                        number=1,
-                    )
+                    duration = timeit(worker.start, number=1)
                     print(f"    {i}/{reps}: {count}/{duration}s = {count / duration}/s")
                     total_time += duration
                     writer.writerow(