Simplify some while-loops with walrus operator

[nickdrozd]

BPO	45975
Nosy	@rhettinger, @terryjreedy, @zware, @miss-islington, @nickdrozd, @iritkatriel, @AlexWaygood
PRs	bpo-45975: Simplify some while-loops with walrus operator #29347 bpo-45975: Use walrus operator for while loops in IDLE #31083 [3.10] bpo-45975: Use walrus operator for some idlelib while loops (GH-31083) #31091 [3.9] bpo-45975: Use walrus operator for some idlelib while loops [GH-31083] #31092 bpo-45975: IDLE - Remove extraneous parens #31107 [3.10] bpo-45975: IDLE - Remove extraneous parens (GH-31107) #31109 [3.9] bpo-45975: IDLE - Remove extraneous parens (GH-31107) #31110

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = None
created_at = <Date 2021-12-03.23:15:50.210>
labels = ['library', '3.11']
title = 'Simplify some while-loops with walrus operator'
updated_at = <Date 2022-02-03.20:44:27.220>
user = 'https://github.com/nickdrozd'

bugs.python.org fields:

activity = <Date 2022-02-03.20:44:27.220>
actor = 'miss-islington'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2021-12-03.23:15:50.210>
creator = 'nickdrozd'
dependencies = []
files = []
hgrepos = []
issue_num = 45975
keywords = ['patch']
message_count = 13.0
messages = ['407615', '407619', '407629', '407649', '407675', '407684', '411839', '412410', '412411', '412413', '412459', '412466', '412467']
nosy_count = 7.0
nosy_names = ['rhettinger', 'terry.reedy', 'zach.ware', 'miss-islington', 'nickdrozd', 'iritkatriel', 'AlexWaygood']
pr_nums = ['29347', '31083', '31091', '31092', '31107', '31109', '31110']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = None
url = 'https://bugs.python.org/issue45975'
versions = ['Python 3.11']

[nickdrozd]

The following pattern occurs a few times in the codebase:

  while 1:
      data = conn.recv(blocksize)
      if not data:
          break
      ...

There is an unbounded while loop in which some kind of input is read. If the input is there, it is processed and the loop is continued; otherwise the loop is broken.

This can be expressed more cleanly with the walrus operator:

  while data := conn.recv(blocksize):
     ...

Some of this code goes back to the days of Python 1. I assume that the original authors would have used the walrus idiom if it had been available, which it wasn't. But now it is.

Rewriting the instances of this pattern shaves off 148 lines from the codebase. Removing the manual break statements makes the logic more straightforward.

This should not change the behavior of anything. I'm assuming that this code is all under test and that any deviations from the existing behavior will be caught. Anything that isn't tested shouldn't be changed (and should be tested).

[zware]

As a general rule, refactorings like this tend to be rejected as the risk of inadvertently adding a new bug outweighs the benefit of subjectively cleaner code ("if it ain't broke, don't fix it!").

Is there any measurable performance benefit from this? Reducing several hundred thousand lines of code by 148 is not a compelling benefit :)

[rhettinger]

The general rule as stated by Zachary is correct; however, I'm in mildly favor of this PR because these are the use cases that the walrus operator was specifically designed for. That said, it would be nice to verify that timings don't get worse and to carefully review each edit to make sure it doesn't introduce a bug.

[iritkatriel]

As I mentioned in the PR, merging this can make future backports to 3.6 and 3.7 more complicated.

[rhettinger]

As I mentioned in the PR, merging this can make future
backports to 3.6 and 3.7 more complicated.

That's correct, but it is also true that we do very few of those and the likelihood of a conflict with one of these edits is low.

[nickdrozd]

Is there any measurable performance benefit from this?

I wouldn't expect any performance changes either way. If it worked out to be slower, that would an unpleasant surprise and a good reason to reject this change. If it worked out to be faster, well, that would be great!

Reducing several hundred thousand lines of code by 148 is not a compelling benefit :)

But it's not just any old 148 lines. By my count it includes the removal of 47 break statements. For a change of this nature there's certainly a chance of introducing errors. On the other hand, every one of those break statements is a site of manual loop-handling logic, and those already present some risk of introducing errors.

[terryjreedy]

To me, the five idlelib changes make the code easier to understand. Nick, please either move them into a separate PR or allow me to do so, and subject to manual testing, I will merge and backport. (I requested this on the PR by maybe you missed it.)

If there are other plausible changes in idlelib that you would like considered, make a spinoff issue where we can discuss them first.

[terryjreedy]

New changeset 51a95be by Nick Drozd in branch 'main':
bpo-45975: Use walrus operator for some idlelib while loops (GH-31083)
51a95be

[miss-islington]

New changeset 2ddc278 by Miss Islington (bot) in branch '3.10':
bpo-45975: Use walrus operator for some idlelib while loops (GH-31083)
2ddc278

[terryjreedy]

New changeset fafd2da by Terry Jan Reedy in branch '3.9':
[3.9] bpo-45975: Use walrus operator for some idlelib while loops (GH-31083)
fafd2da

[terryjreedy]

New changeset 916d0d8 by Terry Jan Reedy in branch 'main':
bpo-45975: IDLE - Remove extraneous parens (GH-31107)
916d0d8

[miss-islington]

New changeset 63523e7 by Miss Islington (bot) in branch '3.10':
bpo-45975: IDLE - Remove extraneous parens (GH-31107)
63523e7

[miss-islington]

New changeset cf7cb1a by Miss Islington (bot) in branch '3.9':
bpo-45975: IDLE - Remove extraneous parens (GH-31107)
cf7cb1a

[ethanfurman]

I believe this is complete; feel free to reopen if it is not.