bpo-34616: add flags to allow top-level-await #13148
Merged
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
Contributor Author
| If You wish to try it, here is a fake-repl session script that work as you would expect; The weird-ish think is the need to use Works as well with trio (cc @njsmith) if you use |
Contributor
| CC @oremanj |
1st1 requested changes May 7, 2019
asyncex Outdated
Member
There was a problem hiding this comment.
This file shouldn't be part of the PR. We'll need to add a couple of unit tests though.
trycomp Outdated
Member
There was a problem hiding this comment.
This file shouldn't be part of the PR.
Python/compile.c Outdated
Member
There was a problem hiding this comment.
This check should be the top-level if; then nest the other two checks inside.
Python/compile.c Outdated
Member
There was a problem hiding this comment.
I'm not sure if this is actually necessary, try to remove this line and see if the tests pass (and add tests :)).
Contributor Author
There was a problem hiding this comment.
... well it actually is or I get
unhandled exception during asyncio.run() shutdown task: <Task finished name='Task-2' coro=<<async_generator_athrow without __name__>()> exception=RuntimeError("can't send non-None value to a just-started coroutine")> RuntimeError: can't send non-None value to a just-started coroutine Not quite sure I understand exactly why...
| A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
1st1 reviewed May 7, 2019
Python/compile.c Outdated
Member
There was a problem hiding this comment.
I'm still not sure if this line is needed, but if it is, it should only be set when (c->c_flags->cf_flags & PyCF_ALLOW_TOP_LEVEL_AWAIT) is true.
Carreau commented May 7, 2019
Contributor Author
| You were right that the ste->ste_coroutine was not necessary in AwaitKind, though it is necessary in Here is a short asyncio-repl with some example inputs: |
Contributor Author
| I have made the requested changes; please review again |
| Thanks for making the requested changes! @1st1: please review the changes made to this pull request. |
Carreau changed the title Contributor Author
| I've pushed again to restart the failing test on travis; all seem to work. Let me know what else I can do; I'd love to get that in 3.8. |
tirkarthi reviewed May 11, 2019
Lib/test/test_builtin.py Outdated
Member
There was a problem hiding this comment.
Align msg with SyntaxError ? PEP 8 enforces 80 character limit per line but I will leave it to reviewer on the same.
Contributor Author
There was a problem hiding this comment.
I've done that and used itertools.product to remove one level of indent. I'm still slightly over.
Member
There was a problem hiding this comment.
Perhaps have msg as a variable outside of context manager.
msg = 'source={!r} mode={!r})'.format(source, mode) with self.assertRaises(SyntaxError, msg): compile(source, '?' , mode)or even better master has f-string debugging merged into master where f"{source=}" expands into f"source={source!r}". So you can rebase onto master and write below :
with self.assertRaises(SyntaxError, msg=f'{source=} {mode=})'): # 75 characters compile(source, '?' , mode) Contributor Author
There was a problem hiding this comment.
Yep good idea. Done that and rebased on master.
This now works: import asyncio import types print('here') L = { 'asyncio': asyncio, 'a': 'something' } code = compile(''' async with asyncio.Lock(): a = await asyncio.sleep(1, result=42) ''', '', 'single', 0x2000) f = types.FunctionType(code, L) print('before', L) asyncio.run(f()) print('after', L) In some narrow but rate case; it is useful to be allow to compile code with top-level await features; for example when writing a REPL. This attempt to allow that.
thanks to tirkarthi Co-Authored-By: Xtreak <tir.karthi@gmail.com>
Use itertools product to remove one level of nesting, and make things a tiny bit more readable.
|
|
Member
| @Carreau looks like |
|
|
|
|
|
|
|
Member
| Please double check the bpo issue number. This PR appears to be unrelated. |
1st1 added a commit to 1st1/cpython that referenced this pull request May 21, 2019
Member
@ned-deily It's related -- the bpo is about adding an async exec and this PR is the way to do that. I'm working on unbreaking buildbots in #13473 |
Member
| Sorry, I should have been more specific. For some reason, PR #13148 is also showing up as a PR under https://bugs.python.org/issue25234. I'm not yet sure why. |
|
Contributor Author
| Yes I can have a look... I'll try to catch up on what happen in the last few hours. |
ned-deily pushed a commit that referenced this pull request May 21, 2019
Member
| (It looks like the spurious bpo entry was caused by something in one of the intermediate commits. I'll just delete the PR entry from bpo-25234.) |
Member
Never mind, I think I've fixed it already. |
Contributor Author
Ok, Thanks apologies for the breakage... |
Member
NP, it's not your fault at all. Strangely, |
Contributor Author
Thanks for guiding me, it was way easier with an expert on my side. And looking forward to contribute more. Would be great to see more contributions from you! We need help with asyncio and many other CPython modules.
Is there an issue for that, and the fact the f-debug is not supported by test_unparse ? |
Member
Great! 🙏
I'm not sure, but would appreciate if you could double check that. |
Member
| This is great, but I wish there was also a documented way to generate an AST for such code. Currently the feature of letting (*) Well, it's not completely undocumented -- the ast module's docs reference this. But |
Contributor Author
| I believe that since 3.7, the SyntaxError is raised only by Do you want clarification in the docs; or still add a flag to ast-parse to prevent top-level-await unless opted-in ? |
Member
| Huh, I didn't realize that some of these "syntax" errors come from compile.c (not just that one, also about e.g. return outside function). Given this, I don't think it makes sense to tweak ast.parse to *prevent* this. I am dithering about whether to document all the flags supported by compile() with the compile() function itself -- IIRC they are intentionally not documented there currently because we don't want to place the burden of supporting all of this on all other Python implementations. So now I'm not sure whether there's any action item at all. What do you think? What do others think? |
Contributor Author
To be fair, some of these used to happen both in making the ast and compile.c. I'm inclined toward leaving this as-is for 3.8, and potentially refine for 3.9. It is a pretty narrow use case; and seeing what people are using this for we can refine the API(s)/documentation. This is still quite tricky to use if you look at Yuri Asyncio REPL., so maybe once this stabilise we can actually recommend a specific way of using it. |
Member
| OK, let's leave it alone. |
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
8 participants
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.
Based on a work with @1st1 during PyCon 2019; and can be seen used in ipython/ipython#11713 (which would dramatically reduce thee complexity and shave an extra few hundreds lines of IPython if we were supporting 3.8+ only).
https://bugs.python.org/issue34616