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

Syntax error with async generator inside dictionary comprehension #77527

Closed
pablogsal opened this issue Apr 24, 2018 · 20 comments
Closed

Syntax error with async generator inside dictionary comprehension #77527

pablogsal opened this issue Apr 24, 2018 · 20 comments
Labels
3.7 (EOL) end of life topic-asyncio type-bug An unexpected behavior, bug, or error

Comments

@pablogsal
Copy link
Member

BPO 33346
Nosy @gvanrossum, @ericvw, @njsmith, @asvetlov, @serhiy-storchaka, @1st1, @MojoVampire, @ilevkivskyi, @pablogsal, @tirkarthi
PRs
  • bpo-33346: Allow async comprehensions inside implicit async comprehensions. #6766
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2021-07-13.21:27:58.712>
    created_at = <Date 2018-04-24.11:05:41.484>
    labels = ['type-bug', '3.7', 'expert-asyncio']
    title = 'Syntax error with async generator inside dictionary comprehension'
    updated_at = <Date 2021-07-13.21:27:58.708>
    user = 'https://github.com/pablogsal'

    bugs.python.org fields:

    activity = <Date 2021-07-13.21:27:58.708>
    actor = 'pablogsal'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-07-13.21:27:58.712>
    closer = 'pablogsal'
    components = ['asyncio']
    creation = <Date 2018-04-24.11:05:41.484>
    creator = 'pablogsal'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 33346
    keywords = ['patch']
    message_count = 20.0
    messages = ['315692', '316115', '316390', '316398', '316404', '316549', '316561', '316584', '316590', '321285', '326089', '337146', '337150', '337340', '337363', '337364', '344194', '372592', '372717', '397445']
    nosy_count = 10.0
    nosy_names = ['gvanrossum', 'ericvw', 'njs', 'asvetlov', 'serhiy.storchaka', 'yselivanov', 'josh.r', 'levkivskyi', 'pablogsal', 'xtreak']
    pr_nums = ['6766']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue33346'
    versions = ['Python 3.7']

    @pablogsal
    Copy link
    Member Author

    Given this async function:

    async def elements(n):
        yield n
        yield n*2
        yield n*3
        yield n*4

    This definition is considered invalid:

    async def test():
        return { n: [x async for x in elements(n)] for n in range(3)}

    SyntaxError: asynchronous comprehension outside of an asynchronous function

    The reason (I suspect) is because the dict comprehension is not an async context (it would be if range was an async iterable and we use async for n in range(3)).

    Is this expected behaviour or something it needs to change?

    @pablogsal pablogsal added 3.7 (EOL) end of life topic-asyncio type-bug An unexpected behavior, bug, or error labels Apr 24, 2018
    @ilevkivskyi
    Copy link
    Member

    My guess this is a consequence of the implicit function scope in comprehensions, see https://bugs.python.org/issue3692

    I would say a proper solution would be to drop the implicit function scope in favour of other mechanisms, but this will require some work and discussions.

    @serhiy-storchaka
    Copy link
    Member

    I think this can be fixed simpler. Currently a comprehension become asynchronous in two cases:

    1. Explicitly, when it contains 'async for'. This is visible at AST level.

    2. Implicitly, when any of inner expressions contains 'await'.

    But asynchronous comprehensions should behave the same way as 'await'. I think that a comprehension should be made implicitly asynchronous if any of inner expressions contains explicit or implicit asynchronous comprehension. This is implemented in PR 6766.

    @gvanrossum
    Copy link
    Member

    @ivan: Please stop bringing up that we should drop the implicit scope for comprehensions. I know you feel this way, but it's not going to happen.

    @serhiy: What does the PEP for async/await say? (Or is there a separate PEP for allowing async/await inside comprehensions?)

    @yury: Your thoughts?

    I do think the code from the OP's example should be expected to work.

    Does it / should it work the same way for generator expressions?

    @serhiy-storchaka
    Copy link
    Member

    There are several related PEPs:

    PEP-492 -- Coroutines with async and await syntax
    PEP-525 -- Asynchronous Generators
    PEP-530 -- Asynchronous Comprehensions

    I haven't found anything explicit about this case. PEP-492 says that just "await" is not enough for converting a function into a coroutine. Explicit "async def" is required. PEP-530 says nothing about implementation details, it omits the fact that comprehensions are implemented by creating and calling an implicit function. From the implementation's point of view PEP-530 means that "async for" and "await" inside an implicit function make it an asynchronous function, and implicit "await" is added in the place of it's call. The natural extension of this is than an implicit "await" should have the same effect as explicit "await", in particular it should make an outer implicit function an asynchronous function and add other implicit "await" in the place of it's call, and so forth. But all this is implied, and is not said explicitly.

    I don't understand one paragraph in PEP-530:

    """
    In principle, asynchronous generator expressions are allowed in
    any context. However, in Python 3.6, due to async and await
    soft-keyword status, asynchronous generator expressions are only
    allowed in an async def function. Once async and await
    become reserved keywords in Python 3.7, this restriction will be
    removed.
    """

    Does it mean that even more restrictions should be removed than PR 6766 does? And what is the relation between this restriction and making "async" and "await" reserved keywords?

    @1st1
    Copy link
    Member

    1st1 commented May 14, 2018

    [Serhiy]

    But asynchronous comprehensions should behave the same way as 'await'. I think that a comprehension should be made implicitly asynchronous if any of inner expressions contains explicit or implicit asynchronous comprehension. This is implemented in PR 6766.

    [Guido]

    @yury: Your thoughts?

    I do think the code from the OP's example should be expected to work.

    I agree with Serhiy and I like his proposal. Essentially, a comprehension is asynchronous when it contains an "await" or an "async for" in it. We want to add another case: make it async when any of its inner-expressions is an async comprehension. Essentially:

    [f: [x async for x in f(x)] for f in fs]
    

    The nested comprehension is obviously asynchronous, so the outer comprehension should become asynchronous too. I think this is a fairly obvious and easy to follow semantics.

    Guido, if you agree that this is a reasonable proposition I can update PEP-530 about this new behaviour (for Python 3.8) and review Serhiy's PR.

    @gvanrossum
    Copy link
    Member

    [f: [x async for x in f(x)] for f in fs]

    Did you mean {} for the outer brackets intead of []?

    I think it is reasonable that if the presence of 'async for' or 'await' in a comprehension makes it async, then this should also apply if that comprehension is nested inside another.

    All of these should only be allowed inside 'async def' though, right?

    @1st1
    Copy link
    Member

    1st1 commented May 14, 2018

    Did you mean {} for the outer brackets intead of []?

    Yes, my bad.

    All of these should only be allowed inside 'async def' though, right?

    Yep, except async generator expressions which are allowed to appear in synchronous contexts, e.g.:

       def foo():
           return (i async for i in ai)

    (this already works in 3.7).

    @gvanrossum
    Copy link
    Member

    Is there any problem that is solved by allowing this example? The asymmetry
    with using [...async for...] in the same position (which is not allowed)
    worries me. Do you have a real use case where it's clearer to write an
    async generator expression instead of a nested async function?

    def foo():
        async def inner():
            async for i in ai:
                yield i
        return inner

    I would encourage you to think about various ways of nesting async
    generator expressions and comprehensions to see if you can poke a hole in
    the design.

    @serhiy-storchaka
    Copy link
    Member

    PR 6766 adds numerous examples of nesting async
    generator expressions and comprehensions as tests.

    @serhiy-storchaka
    Copy link
    Member

    What is the status of this issue?

    Similar change to the Grammar was just merged in bpo-32117. Both bpo-32117 and this issue extend already implemented PEPs (PEP-448 and PEP-525 correspondingly) to the corner case missed in the original PEP.

    Pablo, Yury, could you please start a discussion about this on the Pythod-Dev mailing list?

    @gvanrossum
    Copy link
    Member

    Hm, I think that if Yury still thinks this is a good idea you two should just do this. No need to write a PEP or wait for the Steering Committee. Unless there's a downside? Does it break real existing code?

    @njsmith
    Copy link
    Contributor

    njsmith commented Mar 4, 2019

    There are some tricky subtleties here around the distinction between list/dict/set comprehensions and generator expressions.

    For list/dict/set comprehensions, they're evaluated eagerly, so an async comprehension can only occur in async context. For generator expressions, they're evaluated lazily, so you can write an async generator expression inside a non-async context. *Consuming* the async generator will require an async context, but all the expression itself does is instantiate an object, and that's synchronous.

    (Like Guido, I'm not 100% sure that it's useful to support async generator expressions inside sync contexts, but we've already shipped it, so I'll assume we're going to keep it.)

    So, I would expect the rule to be, precisely: if an async list/dict/set comprehension occurs inside either a list/dict/set comprehension or a generator expression, that should force the enclosing expression to become async.

    So this is a synchronous comprehension, even though it has an async generator expression in it, and must occur inside an 'async def':

    [(i async for i in range(j)) for j in range(n)]

    And this is an async generator expression, which can legally be placed inside a regular 'def' (but can only be consumed inside an 'async def'):

    ([i async for i in range(j)] for j in range(n))

    This might be what Yury/Serhiy/etc. already had in mind, but it's complicated enough that it seemed like it's worth spelling out in detail...

    @1st1
    Copy link
    Member

    1st1 commented Mar 6, 2019

    Yes, I have exactly the same thoughts as Nathaniel about this. The bug should be fixed, and the algorithm should be as follows (quoting Nathaniel):

    So, I would expect the rule to be, precisely: if an async list/dict/set comprehension occurs inside either a list/dict/set comprehension or a generator expression, that should force the enclosing expression to become async.

    @serhiy-storchaka
    Copy link
    Member

    It is what PR 6766 implements, Nathaniel.

    Except that your first example (an asynchronous generator in a synchronous comprehensions) is allowed in the current code and can occur inside a synchronous function.

    @njsmith
    Copy link
    Contributor

    njsmith commented Mar 7, 2019

    Except that your first example (an asynchronous generator in a synchronous comprehensions) is allowed in the current code and can occur inside a synchronous function.

    Oh yeah, you're right of course.

    @pablogsal
    Copy link
    Member Author

    Ping, should we include this in beta1 or as is a "bugfix" this can be backported?

    @serhiy-storchaka
    Copy link
    Member

    What can I do to move this issue forward?

    We already missed 3.7, 3.8 and 3.9.

    @pablogsal
    Copy link
    Member Author

    Yeah, I am +1 on moving this and on the current solution in PR6766

    @pablogsal
    Copy link
    Member Author

    New changeset 054e9c8 by Serhiy Storchaka in branch 'main':
    bpo-33346: Allow async comprehensions inside implicit async comprehensions (GH-6766)
    054e9c8

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 (EOL) end of life topic-asyncio type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants