Add back list() when iterating over d.items() in a for statement #262
Conversation
This reverts commit 8a1ef24. It is often incorrect to omit the list() when converting for x in d.items() to python3 because if you omit list(), then adding or removing elements while iterating will raise RuntimeError: dictionary changed size during iteration.
| """, | ||
| """\ | ||
| for k in x.items(): | ||
| pass | ||
| for k in list(x.items()): |
There was a problem hiding this comment.
really this should be for k in x.copy().items(): because list(x.items()) isn't threadsafe when x.items() was threadsafe on Python 2
There was a problem hiding this comment.
That’s something to consider. copy() does have the advantage of being atomic and being about 10% faster:
# python3.9
>>> import timeit
>>> timeit.timeit(setup='d = dict(zip(range(10000), range(10000)))', stmt='sum(d.copy().keys())', number=10000)
0.814354999999999
>>> timeit.timeit(setup='d = dict(zip(range(10000), range(10000)))', stmt='sum(list(d.keys()))', number=10000)
0.9197422089999989
Downsides of switching to copy():
- collections.abc.Mapping does not include
copy()(source) - There’s at least one mapping that does not contain
copy(): shelve.Shelf
The point of this PR is to restore the 2to3 fix_dict behavior which identifies all the items(), keys(), and values() that may need to change. After manual review, you may find statements where the list() is unnecessary (and you could have used iteritems() before) or you may switch to d.copy() for concurrency and performance. I think changing from list() to .copy() warrants a separate PR.
There was a problem hiding this comment.
@graingert are you requesting me to change it to .copy()?
Fix #176.
This reverts commit 8a1ef24 from #120. That commit’s commit message says, “For some reason, 2to3 only treats a for loop as a special context for iter* methods, so it will add a list() call to e.g.
for x in d.values().”There is a good reason that 2to3’s
fix_dictconsiders for statements and for comprehensions a special context (wherelist()is unnecessary) for iter dict methods but not list dict methods.fix_dictis conservative in omittinglist()ind.keys()-->list(d.keys())anditer()ind.iterkeys()-->iter(d.keys()). For the list functions (python2’sd.keys(),d.items(),d.values()), thelistcan be safely omitted if the result is consumed immediately by a function (e.g.sorted). For the iterator functions (python2’sd.iterkeys(),d.iteritems(),d.itervalues()), theitercan be safely omitted from the view if the result is converted to an iterator or consumed immediately by a function or by a for statement or for comprehension.It is often incorrect to omit the
list()when convertingfor x in d.items()to python3 because if you omitlist(), then adding or removing elements while iterating will raise “RuntimeError: dictionary changed size during iteration”. Example:Furthermore, the overridden
in_special_contextthat considerskeys()to be the same asiterkeys()is incorrect because the super method implementation considersiterto be a special context (where addingiter()is not needed) whenisiter==True. Soiter(d.keys())should be converted toiter(list(d.keys())), not left unchanged as currently happens.