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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bpo-39320: Fix changes in the evaluation logic of unpacking operations. #18264

Open
wants to merge 6 commits into
base: master
from

Conversation

@brandtbucher
Copy link
Member

brandtbucher commented Jan 30, 2020

Commits 13bc139 and 8a4cd70 introduced subtle changes in the evaluation logic of unpacking operations. Previously, all elements were evaluated prior to being collected in a container. Now, these operations are interleaved.

For example, the code [*foo, print("XXX")] used to print "XXX" before calling foo.__iter__. This is no longer the case.

I've included several similar regression tests in this PR. @markshannon, your intuition about keeping the opargs for the DICT_UPDATE/DICT_MERGE ops was good.

https://bugs.python.org/issue39320

@pablogsal pablogsal requested a review from markshannon Jan 30, 2020
@brandtbucher brandtbucher changed the title bpo-39320: Fix regressions in the evaluation logic of unpacking operations. bpo-39320: Fix changes in the evaluation logic of unpacking operations. Jan 30, 2020
@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 30, 2020

Codecov Report

Merging #18264 into master will increase coverage by 1.07%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master   #18264       +/-   ##
===========================================
+ Coverage   82.12%   83.19%    +1.07%     
===========================================
  Files        1955     1570      -385     
  Lines      588723   414212   -174511     
  Branches    44383    44401       +18     
===========================================
- Hits       483465   344607   -138858     
+ Misses      95630    59966    -35664     
- Partials     9628     9639       +11     
Impacted Files Coverage Δ
Lib/distutils/tests/test_bdist_rpm.py 30.00% <0.00%> (-65.00%) ⬇️
Lib/distutils/command/bdist_rpm.py 7.63% <0.00%> (-56.88%) ⬇️
Lib/test/test_urllib2net.py 76.92% <0.00%> (-13.85%) ⬇️
Lib/test/test_smtpnet.py 78.57% <0.00%> (-7.15%) ⬇️
Lib/ftplib.py 63.85% <0.00%> (-6.06%) ⬇️
Lib/test/test_ftplib.py 87.11% <0.00%> (-4.72%) ⬇️
Tools/scripts/db2pickle.py 17.82% <0.00%> (-3.97%) ⬇️
Tools/scripts/pickle2db.py 16.98% <0.00%> (-3.78%) ⬇️
Lib/test/test_socket.py 71.94% <0.00%> (-3.77%) ⬇️
Lib/test/test_asyncio/test_base_events.py 91.84% <0.00%> (-3.30%) ⬇️
... and 441 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a327677...7f65751. Read the comment docs.

@markshannon

This comment has been minimized.

Copy link
Contributor

markshannon commented Jan 30, 2020

According to https://docs.python.org/3/reference/expressions.html#evaluation-order expressions within a list are evaluated left to right. So, [*None, print("XXX")] should not print "XXX" as *None should be evaluated first and raise a TypeError.
Also compare with [+None, print("XXX")], [None[:], print("XXX")] and [None(), print("XXX")]

In the expression [a, b] a should be fully evaluated before b.

The same rationale applies to ** unpacking.

@brandtbucher

This comment has been minimized.

Copy link
Member Author

brandtbucher commented Jan 30, 2020

I don't think I agree.

According to https://docs.python.org/3/reference/expressions.html#evaluation-order expressions within a list are evaluated left to right.

In the expression [a, b] a should be fully evaluated before b.

Absolutely, 100%. And in [*a, *b], the same holds. *a is not an expression, though!

"Unary *" isn't an operator; it doesn't appear on the operator precedence table at the link you provided, and you can't evaluate *x. Like the brackets and the comma, it's part of the syntax of the outer display expression, not the inner one. It specifies how the list should be built, so it should be evaluated last, as part of the list construction (as it always has been).

I think one of the examples in the link you provided is the most clarifying here:

In the following lines, expressions will be evaluated in the arithmetic order of their suffixes:

...
expr1(expr2, expr3, *expr4, **expr5)
...

Note that the stars are not part of expressions 1-5, but are a part of the top-level call expression that operates on them all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.