gh-143055: Implementation of PEP 798 #143056
Conversation
* add error messages for dict unpacking in dict keys/values * change phrasing of existing error messages about using dict unpacking in a listcomp / genexp
adqm
requested review from
AA-Turner,
Eclips4,
JelleZijlstra,
iritkatriel,
isidentical,
lysnikolaou,
markshannon,
pablogsal,
tomasr8 and
willingc
as code owners
JelleZijlstra
left a comment
JelleZijlstra
left a comment
There was a problem hiding this comment.
Thanks! A few nitpicky comments, I didn't read past Lib/test yet.
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
JelleZijlstra
left a comment
JelleZijlstra
left a comment
There was a problem hiding this comment.
The C code all looks good. I'll run it through the buildbots to make sure though.
JelleZijlstra
added
the
🔨 test-with-buildbots
|
🤖 New build scheduled with the buildbot fleet by @JelleZijlstra for commit 81e86b3 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F143056%2Fmerge If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again. |
bedevere-bot
removed
the
🔨 test-with-buildbots
ZeroIntensity
left a comment
ZeroIntensity
left a comment
There was a problem hiding this comment.
I'm assuming the plan is to document this in 3.15 "What's New" later?
|
@ZeroIntensity Thanks for the review!
I haven't drafted a blurb for "What's New" yet, but I'm happy to do so. I can do that in a separate PR later, or I don't mind adding it to this one if that's better. |
|
Probably easiest to add the What's New entry here too. |
|
Will try to make a pass over the grammar next week if @lysnikolaou doesn't beat me to it |
lysnikolaou
left a comment
lysnikolaou
left a comment
There was a problem hiding this comment.
I had a look at the grammar. The changes look great in general. I left a couple of minor comments, but feel free to merge if the PR is not blocked otherwise.
Maybe @pablogsal wants to give it a look as well?
Grammar/python.gram
Outdated
|
|
||
| if_expression[expr_ty]: | ||
| | a=disjunction 'if' b=disjunction 'else' c=expression { _PyAST_IfExp(b, a, c, EXTRA) } | ||
| | invalid_if_expression |
There was a problem hiding this comment.
I'm pretty sure this alternative is never reached, since, in the second pass, it's encountered in expression before we get to if_expression and only in invalid rules otherwise that will overwrite the error either way.
There was a problem hiding this comment.
Thanks for taking a look! Yep, I think you're right that we can't reach invalid_if_expression here, good catch. I'll push that change today (along with a what's new entry).
| | &'(' (genexp | tuple | group) | ||
| | &'[' (listcomp | list) | ||
| | &'{' (dictcomp | setcomp | dict | set) |
There was a problem hiding this comment.
Changing the order here is probably only about error messages, right? If so, have you checked whether this affects parsing performance at all? To be clear, I'm okay to sacrifice a bit of performance for better error messages.
There was a problem hiding this comment.
Yep, the reordering here was purely for error reporting. I hadn't done any performance testing before, but I did a quick, unscientific test this morning, doing some quick timings on ast.parse for a few things.
As might be expected, compared to the place I branched from, it looks like this branch is, in general, a little bit faster on comprehensions and a little bit slower on literal lists. It's a little hard to say how this works out on the whole, but the time for parsing the full standard library was always within ±2% every time I ran it. I can look more thoroughly if we're concerned about this, but this little test at least suggests that things haven't blown up on average.
Sample results:
| Test | Reason | This Branch | Merge base | Change |
|---|---|---|---|---|
[] (1e5 times) | empty list | 0.5838 | 0.5539 | ~5% slower |
[x, y, z, a, b, c] (1e5 times) | well-formed list | 1.4051 | 1.3827 | ~2% slower |
[1, 2, 3, 4, 5, 6 (1e5 times) | broken list | 1.1750 | 1.1413 | ~3% slower |
[x for x in y if z and a] (1e5 times) | well-formed comprehension | 1.1642 | 1.2393 | ~6% faster |
[x for x in (1e5 times) | broken comprehension | 0.6568 | 0.7014 | ~6% faster |
[**x for x in y] (1e5 times) | broken comprehension unpacking (new specific error message) | 1.2675 | 1.0519 | ~20% slower |
[x if x else y] (1e5 times) | good ifexp | 1.0163 | 0.9890 | ~3% slower |
[*x if x else y] (1e5 times) | broken ifexp (new specific error message) | 1.3299 | 1.7870 | ~26% faster |
| full stdlib (5 times) | 26.1641 | 26.2642 | ~equal | |
|
Going to "Update branch" to see if it fixes CI |
|
Thanks @JelleZijlstra! Does "update branch" run a different set of tests? Unless I'm missing something, it looks like the test that's currently failing wasn't run on any of the earlier commits. Is that something I should be concerned about? |
|
It's relatively new, so it wasn't on this PR before the update. I think it's an issue with |
6 participants
This PR is an implementation of PEP 798 (Unpacking in Comprehensions), reflecting the SC's required modification to genexp semantics from the original proposal.
Feedback is welcome! :)
📚 Documentation preview 📚: https://cpython-previews--143056.org.readthedocs.build/