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-30406: Make async and await proper keywords #1669

Merged
merged 26 commits into from Oct 6, 2017

Conversation

@JelleZijlstra
Copy link
Contributor

JelleZijlstra commented May 19, 2017

https://bugs.python.org/issue30406

Copy link
Contributor

AraHaan left a comment

Looks alright to me.

@JelleZijlstra

This comment has been minimized.

Copy link
Contributor Author

JelleZijlstra commented May 19, 2017

I might need some time to fix the test failures.

@JelleZijlstra JelleZijlstra changed the title bpo-30406: Make async and await proper keywords bpo-30406: [WIP] Make async and await proper keywords May 19, 2017
@AraHaan

This comment has been minimized.

Copy link
Contributor

AraHaan commented May 19, 2017

Alright, But in general it looks like a nice start.

@JelleZijlstra JelleZijlstra changed the title bpo-30406: [WIP] Make async and await proper keywords bpo-30406: Make async and await proper keywords May 19, 2017
@1st1 1st1 self-assigned this May 20, 2017
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.

Copy link
@vstinner

vstinner May 20, 2017

Member

wait... this one looks like a bug.

This comment has been minimized.

Copy link
@JelleZijlstra

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.

Copy link
@vstinner

vstinner May 20, 2017

Member

ditto (also a bug)

This comment has been minimized.

Copy link
@JelleZijlstra

JelleZijlstra May 20, 2017

Author Contributor

Same here, this is also lib2to3.

@vstinner vstinner requested a review from 1st1 May 20, 2017
@1st1

This comment has been minimized.

Copy link
Member

1st1 commented May 20, 2017

Overall looks good. I'll make a detailed review in a few days.

Copy link
Member

vstinner left a comment

@JelleZijlstra

This comment has been minimized.

Copy link
Contributor Author

JelleZijlstra commented May 20, 2017

grammar.html just directly includes Grammar/Grammar, so it shouldn't require further changes.

@AraHaan

This comment has been minimized.

Copy link
Contributor

AraHaan commented May 20, 2017

@JelleZijlstra you mind also removing 'asyncio.async' as well as that thing might cause an SyntaxError.

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented May 20, 2017

@JelleZijlstra

This comment has been minimized.

Copy link
Contributor Author

JelleZijlstra commented May 21, 2017

@AraHaan it's currently implemented as something like globals()['async'] = ensure_future, so the implementation won't cause a SyntaxError (as you can tell from the test suite passing). I suppose I could remove the alias, but I also don't want to make this patch bigger than necessary, so I'll wait for Yury to review it.

@AraHaan

This comment has been minimized.

Copy link
Contributor

AraHaan commented May 21, 2017

Ok

@1st1

This comment has been minimized.

Copy link
Member

1st1 commented Jun 10, 2017

I'll merge this when I return from my vacation in 11 days. Thank you for keeping the pr up to date!

@JelleZijlstra

This comment has been minimized.

Copy link
Contributor Author

JelleZijlstra commented Jul 20, 2017

@1st1 ping – have you had a chance to look at this PR?

Copy link
Member

1st1 left a comment

Overall looks good. Could you please rename it once again and add a news entry with blurb?

@1st1

This comment has been minimized.

Copy link
Member

1st1 commented Jul 20, 2017

s/rename/rebase

@JelleZijlstra JelleZijlstra force-pushed the JelleZijlstra:asyncsyntax branch from f2838a0 to 6562616 Jul 21, 2017
@JelleZijlstra

This comment has been minimized.

Copy link
Contributor Author

JelleZijlstra commented Jul 21, 2017

Just rebased from current master and added a news entry using blurb.

@JelleZijlstra

This comment has been minimized.

Copy link
Contributor Author

JelleZijlstra commented Jul 21, 2017

The AppVeyor failure is in test_idle and looks unrelated to this PR.

@JelleZijlstra

This comment has been minimized.

Copy link
Contributor Author

JelleZijlstra commented Jul 21, 2017

Oh, probably related to 7c5798e.

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Jul 21, 2017

The AppVeyor failure is in test_idle and looks unrelated to this PR.

Rebase again and it should be fine.

@JelleZijlstra JelleZijlstra force-pushed the JelleZijlstra:asyncsyntax branch from 16949e3 to 36ea452 Jul 21, 2017
Copy link
Member

1st1 left a comment

Playing with the code right now. One of the things you also need to do is to cleanup forbidden_name() function in Python/ast.c.

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.

Copy link
@1st1

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.

Copy link
@JelleZijlstra

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.

Copy link
@1st1

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.

Copy link
@1st1

1st1 Jul 21, 2017

Member

You can drop this comment now.

@@ -0,0 +1 @@
``async`` and ``await`` are now proper keywords, as specified in PEP 492.

This comment has been minimized.

Copy link
@1st1

1st1 Jul 21, 2017

Member

Change to:

Make ``async`` and ``await`` proper keywords, as specified in PEP 492.
@1st1

This comment has been minimized.

Copy link
Member

1st1 commented Sep 26, 2017

Ping? I plan to work on this myself if this isn't resolved soon.

@JelleZijlstra

This comment has been minimized.

Copy link
Contributor Author

JelleZijlstra commented Sep 26, 2017

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.

@1st1

This comment has been minimized.

Copy link
Member

1st1 commented Sep 26, 2017

I was unable to resolve the issue I mentioned in #1669 (comment). [..]

Interesting. I'll take a look at that too.

@JelleZijlstra JelleZijlstra force-pushed the JelleZijlstra:asyncsyntax branch from 2ed73d1 to 78ddbde Oct 6, 2017
@JelleZijlstra

This comment has been minimized.

Copy link
Contributor Author

JelleZijlstra commented Oct 6, 2017

Just rebased and adjusted the lib2to3 grammar changes to make local tests pass.

@1st1 1st1 merged commit ac31770 into python:master Oct 6, 2017
4 checks passed
4 checks passed
bedevere/issue-number Issue number 30406 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull request Oct 9, 2017
@ambv

This comment has been minimized.

Copy link
Contributor

ambv commented Mar 12, 2018

@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 async and await as names.

@1st1

This comment has been minimized.

Copy link
Member

1st1 commented Mar 12, 2018

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).

@ambv

This comment has been minimized.

Copy link
Contributor

ambv commented Mar 12, 2018

Yes, this is what I'm inclined to do. Revert the lib2to3 part.

@JelleZijlstra

This comment has been minimized.

Copy link
Contributor Author

JelleZijlstra commented Mar 12, 2018

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.

@JelleZijlstra JelleZijlstra deleted the JelleZijlstra:asyncsyntax branch Mar 12, 2018
JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull request Mar 18, 2018
This reverts commit ac31770.

(Reverts only the lib2to3 part.)
JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull request Mar 18, 2018
This reverts commit ac31770.

(Reverts only the lib2to3 part.)
@JelleZijlstra

This comment has been minimized.

Copy link
Contributor Author

JelleZijlstra commented Mar 18, 2018

OK, I made that up—tests pass cleanly if I undo my lib2to3 changes. @ambv @1st1 I submitted #6143 to fix the regression.

miss-islington added a commit to miss-islington/cpython that referenced this pull request Mar 18, 2018
…" (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>
ambv added a commit that referenced this pull request Mar 18, 2018
…6143)

This reverts commit ac31770.

(Reverts only the lib2to3 part.)
miss-islington added a commit that referenced this pull request Mar 18, 2018
…H-6143)

This reverts commit ac31770.

(Reverts only the lib2to3 part.)
(cherry picked from commit f64aae4)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
jo2y added a commit to jo2y/cpython that referenced this pull request Mar 23, 2018
@ddfisher ddfisher mentioned this pull request Jun 30, 2018
yahya-abou-imran added a commit to yahya-abou-imran/cpython that referenced this pull request Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.