bpo-34616: add flags to allow top-level-await #13148
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
|
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 |
|
CC @oremanj |
1st1
requested changes
May 7, 2019
asyncex
Outdated
| @@ -0,0 +1,20 @@ | |||
| import asyncio | |||
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
| @@ -0,0 +1,16 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
This file shouldn't be part of the PR.
Python/compile.c
Outdated
| c->u->u_ste->ste_coroutine = 1; | ||
| } | ||
|
|
||
| if (!(c->c_flags->cf_flags & PyCF_ALLOW_TOP_LEVEL_AWAIT)) { |
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
| if (c->u->u_scope_type != COMPILER_SCOPE_ASYNC_FUNCTION && | ||
| c->u->u_scope_type != COMPILER_SCOPE_COMPREHENSION) | ||
| return compiler_error(c, "'await' outside async function"); | ||
| c->u->u_ste->ste_coroutine = 1; |
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 :)).
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
| return compiler_error(c, "'await' outside async function"); | ||
| } | ||
| } | ||
| c->u->u_ste->ste_coroutine = 1; |
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
|
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: |
|
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
|
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
| pass''']): | ||
| source = dedent(code_sample) | ||
| with self.assertRaises(SyntaxError, | ||
| msg='source={!r} mode={!r})'.format(source, mode)): |
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.
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.
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)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.
|
|
|
@Carreau looks like |
|
|
|
|
|
|
|
|
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
@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 |
|
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. |
|
|
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
|
(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.) |
Never mind, I think I've fixed it already. |
Ok, Thanks apologies for the breakage... |
NP, it's not your fault at all. Strangely, |
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 ? |
Great! 🙏
I'm not sure, but would appreciate if you could double check that. |
|
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 |
|
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 ? |
|
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?
|
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. |
|
OK, let's leave it alone.
|
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.
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