bpo-30406: Make async and await proper keywords by JelleZijlstra · Pull Request #1669 · python/cpython (original) (raw)

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

Already on GitHub?Sign in to your account

Conversation56 Commits26 Checks0 Files changed

Conversation

This file contains hidden or 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

[ Show hidden characters]({{ revealButtonHref }})

Contributor

@AraHaan AraHaan left a comment • Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks alright to me.

I might need some time to fix the test failures.

JelleZijlstra changed the title~~bpo-30406: Make async and await proper keywords~~ bpo-30406: [WIP] Make async and await proper keywords

May 19, 2017

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

JelleZijlstra changed the title~~bpo-30406: [WIP] Make async and await proper keywords~~ bpo-30406: Make async and await proper keywords

May 19, 2017

self.invalid_syntax("""def foo():
async for a in b: pass""")
self.validate("""def foo():
async for a in b: pass""")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait... this one looks like a bug.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto (also a bug)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, this is also lib2to3.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Thanks for the clarification :-) I let Yury review the change. He knows the parser much better than me!

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

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

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

Member

@1st1 1st1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

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

Rebase again and it should be fine.

Member

@1st1 1st1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can drop this comment now.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change to:

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

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

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.

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

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

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

JelleZijlstra added a commit to JelleZijlstra/cpython that referenced this pull request

Oct 9, 2017

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

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

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

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.

ambv mentioned this pull request