Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upbpo-30406: Make async and await proper keywords #1669
Conversation
Looks alright to me. |
This comment has been minimized.
This comment has been minimized.
I might need some time to fix the test failures. |
This comment has been minimized.
This comment has been minimized.
Alright, But in general it looks like a nice start. |
self.invalid_syntax("""def foo(): | ||
async for a in b: pass""") | ||
self.validate("""def foo(): | ||
async for a in b: pass""") |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
JelleZijlstra
May 20, 2017
•
Author
Contributor
I think lib2to3's parser in general doesn't do semantic checking of the sort that would be required to reject this. For example, it also doesn't fail on putting nonlocal
or yield from
outside a function, or yield from
inside an async generator.
self.invalid_syntax("""def foo(): | ||
async with a: pass""") | ||
self.validate("""def foo(): | ||
async with a: pass""") |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Overall looks good. I'll make a detailed review in a few days. |
https://docs.python.org/dev/reference/grammar.html should also be updated, no? |
This comment has been minimized.
This comment has been minimized.
grammar.html just directly includes Grammar/Grammar, so it shouldn't require further changes. |
This comment has been minimized.
This comment has been minimized.
@JelleZijlstra you mind also removing 'asyncio.async' as well as that thing might cause an SyntaxError. |
This comment has been minimized.
This comment has been minimized.
Thanks for the clarification :-) I let Yury review the change. He knows the
parser much better than me!
|
This comment has been minimized.
This comment has been minimized.
@AraHaan it's currently implemented as something like |
This comment has been minimized.
This comment has been minimized.
Ok |
This comment has been minimized.
This comment has been minimized.
I'll merge this when I return from my vacation in 11 days. Thank you for keeping the pr up to date! |
This comment has been minimized.
This comment has been minimized.
@1st1 ping – have you had a chance to look at this PR? |
Overall looks good. Could you please rename it once again and add a news entry with blurb? |
This comment has been minimized.
This comment has been minimized.
s/rename/rebase |
f2838a0
to
6562616
This comment has been minimized.
This comment has been minimized.
Just rebased from current master and added a news entry using blurb. |
This comment has been minimized.
This comment has been minimized.
The AppVeyor failure is in test_idle and looks unrelated to this PR. |
This comment has been minimized.
This comment has been minimized.
Oh, probably related to 7c5798e. |
This comment has been minimized.
This comment has been minimized.
Rebase again and it should be fine. |
16949e3
to
36ea452
Playing with the code right now. One of the things you also need to do is to cleanup |
comp_for_or_async: comp_async_for | comp_for | ||
comp_iter: comp_async_for | comp_for | comp_if | ||
comp_async_for: 'async' 'for' exprlist 'in' or_test [comp_iter] | ||
comp_for: 'for' exprlist 'in' or_test [comp_iter] | ||
comp_if: 'if' test_nocond [comp_iter] |
This comment has been minimized.
This comment has been minimized.
1st1
Jul 21, 2017
Member
Why did you do all these renames and added atom_expr
? Why don't we just replace ASYNC
with 'async'
(ditto for await)? I'd avoid making any changes to the grammar besides token adjustments.
This comment has been minimized.
This comment has been minimized.
JelleZijlstra
Jul 22, 2017
Author
Contributor
I don't remember exactly (it's been a few months), but I just tried to implement your suggestions and I'm getting failures in test_parser that I don't currently understand. I'll look into it some more. The errors are all on comprehensions and are similar to
File "/Users/jzijlstra-mpbt/py/cpython/Lib/test/test_parser.py", line 24, in roundtrip
self.fail("could not roundtrip %r: %s" % (s, why))
AssertionError: could not roundtrip '{x for x in seq}': Illegal terminal: expected NAME, got 330.
comp_for_or_async: comp_async_for | comp_for | ||
comp_iter: comp_async_for | comp_for | comp_if | ||
comp_async_for: 'async' 'for' exprlist 'in' testlist_safe [comp_iter] | ||
comp_for: 'for' exprlist 'in' testlist_safe [comp_iter] | ||
comp_if: 'if' old_test [comp_iter] |
This comment has been minimized.
This comment has been minimized.
1st1
Jul 21, 2017
Member
Adding to the point raised for changes in Grammar
: when you change/rename/add new productions here, you might be breaking some code that uses lib2to3
to define custom fixters. Again, I'd avoid any changes.
|
||
def test_goodsyntax_1(self): | ||
def test_badsyntax_4(self): | ||
# Tests for issue 24619 |
This comment has been minimized.
This comment has been minimized.
@@ -0,0 +1 @@ | |||
``async`` and ``await`` are now proper keywords, as specified in PEP 492. |
This comment has been minimized.
This comment has been minimized.
1st1
Jul 21, 2017
Member
Change to:
Make ``async`` and ``await`` proper keywords, as specified in PEP 492.
This comment has been minimized.
This comment has been minimized.
Ping? I plan to work on this myself if this isn't resolved soon. |
This comment has been minimized.
This comment has been minimized.
I was unable to resolve the issue I mentioned in #1669 (comment). I can push out the other fixes though and take another look within the next few days to see if I can get it working. |
This comment has been minimized.
This comment has been minimized.
Interesting. I'll take a look at that too. |
This comment has been minimized.
This comment has been minimized.
Just rebased and adjusted the lib2to3 grammar changes to make local tests pass. |
@1st1 requested this in python#1669 (comment).
This comment has been minimized.
This comment has been minimized.
@JelleZijlstra This change is incorrect in lib2to3. That library's grammar is explicitly kept in a way that is able to parse all code valid for Python 2 - Python 3.7. With your change we lost the ability to read valid Python 2 - Python 3.6 code that uses |
This comment has been minimized.
This comment has been minimized.
Hm, I'm not sure what options we have here. Any suggestions? About the only one that I have is to undo the change for lib2to3 entirely (but keeping the rest of the changes). |
This comment has been minimized.
This comment has been minimized.
Yes, this is what I'm inclined to do. Revert the lib2to3 part. |
This comment has been minimized.
This comment has been minimized.
If I recall correctly I made changes to lib2to3 because some test was failing otherwise, so it might not be as simple as just reverting parts of this PR. I can spend some time looking into this later today. |
This comment has been minimized.
This comment has been minimized.
…" (pythonGH-6143) This reverts commit ac31770. (Reverts only the lib2to3 part.) (cherry picked from commit f64aae4) Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
pythonGH-6143) This reverts commit ac31770. (Reverts only the lib2to3 part.)
pythonGH-6143) This reverts commit ac31770. (Reverts only the lib2to3 part.)
JelleZijlstra commentedMay 19, 2017
•
edited by bedevere-bot
https://bugs.python.org/issue30406