bpo-35224: PEP 572 Implementation #10497
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
emilyemorehouse
commented
Nov 13, 2018
emilyemorehouse
requested review from
serhiy-storchaka,
1st1,
benjaminp,
gvanrossum and
ncoghlan
There was some discussion around disallowing |
|
@emilyemorehouse: Can you please add a NEWS entry using blurb? Travis CI failed on "FAIL: test_exceptions (test.test_pickle.CompatPickleTests) [TargetScopeError]", I didn't check why yet. |
vstinner
changed the title
Ah, it seems like you don't want to merge this PR in its current state, so I took the liberty of adding [WIP] to the PR title and the DO-NOT-MERGE label. |
|
I hope the [WIP] label, DO-NOT-MERGE label and presence of caveats and
TODOs don't prevent qualified core devs from reviewing this PR. In order to
be able to experiment with the feature at all we need some baseline, and
this PR is meant to be that. In some cases we may find that the behavior of
the code is just fine and it's better to tweak the PEP. In other cases we
may want to postpone solving something and plan to follow up with another
PR that tweaks some edge case (e.g. to give better diagnostics). In yet
other cases we need to fix this PR before it can be merged (e.g. if there
are refcount bugs or other crashables).
|
|
@gvanrossum Agreed. I'll update the PR description to reflect this as well, but I'm leaning towards (in no particular order):
Then merge. Anything else can be handled in subsequent PRs. |
emilyemorehouse
changed the title
|
(But please, either leave the |
|
Yes, I'd focus on news entry, reverting current ((a) := 10) restriction, and CI failures -- and thorough reviews. (Serhiy?) |
This phrase in the PEP is normative:
Note that unlike regular assignment, PEP 572 does not allow any form of tuple unpacking. This is intentional: we couldn't find any examples where it would actually be useful. |
|
I removed the DO_NOT_MERGE label -- if this passes review I would be happy to see it merged (but please let Emily merge it herself). Can we get a review from @benjaminp or @vstinner ? Also @serhiy-storchaka if you can take a look that would be appreciated. |
emilyemorehouse
force-pushed
the
pep572
branch
from
f67a0ef to
79463ad
Compare
| @@ -108,7 +109,7 @@ atom: ('(' [yield_expr|testlist_comp] ')' | | |||
| '[' [testlist_comp] ']' | | |||
| '{' [dictorsetmaker] '}' | | |||
| NAME | NUMBER | STRING+ | '...' | 'None' | 'True' | 'False') | |||
| testlist_comp: (test|star_expr) ( comp_for | (',' (test|star_expr))* [','] ) | |||
| testlist_comp: (namedexpr_test|star_expr) ( comp_for | (',' (namedexpr_test|star_expr))* [','] ) | |||
There was a problem hiding this comment.
Does not [x := 1, 2] look too ambiguous? Why this syntax is allowed? {x := 1, 2}, {x := 1 : 2} and {1 : x := 2} are not allowed.
| @@ -134,6 +135,7 @@ arglist: argument (',' argument)* [','] | |||
| # multiple (test comp_for) arguments are blocked; keyword unpackings | |||
| # that precede iterable unpackings are blocked; etc. | |||
| argument: ( test [comp_for] | | |||
| test ':=' test | | |||
There was a problem hiding this comment.
This looks like a bug magnet: f(x=1) vs f(x:=1). Is this desired?
Lib/tokenize.py
Outdated
| @@ -168,7 +169,7 @@ def _compile(expr): | |||
| # recognized as two instances of =). | |||
| Operator = group(r"\*\*=?", r">>=?", r"<<=?", r"!=", | |||
| r"//=?", r"->", | |||
| r"[+\-*/%&@|^=<>]=?", | |||
| r"[+\-*/%&@|^=<>:]=?", | |||
There was a problem hiding this comment.
This adds the operator :.
| @@ -87,7 +88,7 @@ module Python | |||
| -- col_offset is the byte offset in the utf8 string the parser uses | |||
| attributes (int lineno, int col_offset) | |||
|
|
|||
| expr_context = Load | Store | Del | AugLoad | AugStore | Param | |||
| expr_context = Load | Store | Del | AugLoad | AugStore | Param | NamedStore | |||
There was a problem hiding this comment.
Why NamedStore is needed? Can not Store be used instead?
| @@ -2780,6 +2852,26 @@ ast_for_call(struct compiling *c, const node *n, expr_ty func, bool allowgen) | |||
| return NULL; | |||
| asdl_seq_SET(args, nargs++, e); | |||
| } | |||
| else if (TYPE(CHILD(ch, 1)) == COLONEQUAL) { | |||
| /* treat colon equal as positional argument */ | |||
| if (nkeywords) { | |||
There was a problem hiding this comment.
I prefer to avoid code duplication. Maybe merge this case with the case for NCH(ch) == 1?
if (NCH(ch) == 1 || (NCH(ch) == 3 && TYPE(CHILD(ch, 1)) == COLONEQUAL)) {
if (nkeywords) {
...
}
}
Sure. Go ahead!
I'm not sure about these changes, but I would prefer to not introduce new compiler warnings if possible. |
|
Oh, when I proposed to push directly into your branch, I forgot to give the context: Python 3.8a1 is coming soon, so it's time to merge this PR ;-) It would be great to have it in the first 3.8 (alpha) release ;-) |
|
I'm not sure if all compilers emit the same warnings, so here are the GCC 8.1. warnings: |
|
Ah, I only had the switch warning. I added your suggestion for the fallthrough, hopefully that fixes it since I cannot test it. Otherwise, if everything is green @gvanrossum already told me that I can merge. |
|
@emilyemorehouse: Please replace |
|
|
|
|
|
|
|
|
|
I confirm that "make" no longer emit warning with your merged change. Good. |
|
|
|
|
@emilyemorehouse: Congrats! You broke many buildbots :-D Each core developer has to do that once in their career :-) Don't worry, it's fine. test_tools failed multiple buildbots but passed on Travis CI, AppVeyor, and when I ran tests locally. Ok, I recall the joke. test_tools.test_unparse.DirectoryTestCase picks 10 random files from the stdlib, so the test may pass or fail, depending which files are picked. You have to unable the cpu resource using -u option. For example, just use "-u all". There is exactly one file which causes test_unparse to fail, Lib/test/test_named_expressions.py: Maybe a few buildbots have the "cpu" test resource enabled, I don't recall. |
|
|
My PR #11670 should fix test_tools. |
|
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is the implementation of PEP 572. It has been few a couple of rounds of refactoring per my and Guido's review, but is now ready for a more thorough review from others.
The only TODO items before merging are:
Additional Notes
The new test cases of note can be explicitly run using:
Please note the following caveats and future tasks. These items are NOT blocking reviews or merging, but are here as FYIs if they come up in review:
Must decide upon cleaner implementation of disallowing expressions such asThis change has been reverted and deferred((a) := 10), or to continuing allowing this. The current, and know-to-be-imperfect, implementation was introduced here.Differences between the PEP spec and implementation
From the "Scope of the target" section of the PEP, there are two cases that should raise a TargetScopeError: when an assignment expression is used in a comprehension inside a class body or for special cases in comprehensions.
Invalid examples for the latter include:
However, the following work in the implementation,though the PEP states they should be invalid:
The following does not work in the implementation (as desired), but does not throw a TargetScopeError as defined in the PEP:
https://bugs.python.org/issue35224