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

PEP 563: Should the behavior change for yield/yield from's #86891

Closed
isidentical opened this issue Dec 23, 2020 · 34 comments
Closed

PEP 563: Should the behavior change for yield/yield from's #86891

isidentical opened this issue Dec 23, 2020 · 34 comments

Comments

@isidentical
Copy link
Sponsor Member

@isidentical isidentical commented Dec 23, 2020

BPO 42725
Nosy @gvanrossum, @larryhastings, @markshannon, @serhiy-storchaka, @gvanrossum, @lysnikolaou, @pablogsal, @miss-islington, @isidentical
PRs
  • #25583
  • #25974
  • #25988
  • 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-05-08.13:03:31.147>
    created_at = <Date 2020-12-23.13:28:51.066>
    labels = []
    title = "PEP 563: Should the behavior change for yield/yield from's"
    updated_at = <Date 2021-05-08.13:03:31.146>
    user = 'https://github.com/isidentical'

    bugs.python.org fields:

    activity = <Date 2021-05-08.13:03:31.146>
    actor = 'BTaskaya'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-05-08.13:03:31.147>
    closer = 'BTaskaya'
    components = []
    creation = <Date 2020-12-23.13:28:51.066>
    creator = 'BTaskaya'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 42725
    keywords = ['patch']
    message_count = 34.0
    messages = ['383647', '383655', '383659', '383660', '383661', '383662', '383686', '383693', '383730', '383812', '384483', '384508', '384580', '384589', '384602', '384617', '384638', '385203', '385210', '385217', '385223', '391723', '391747', '391748', '391750', '391754', '391764', '391780', '391782', '391783', '392777', '392803', '392831', '393252']
    nosy_count = 9.0
    nosy_names = ['gvanrossum', 'larry', 'Mark.Shannon', 'serhiy.storchaka', 'Guido.van.Rossum', 'lys.nikolaou', 'pablogsal', 'miss-islington', 'BTaskaya']
    pr_nums = ['25583', '25974', '25988']
    priority = 'normal'
    resolution = None
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue42725'
    versions = []

    @isidentical
    Copy link
    Sponsor Member Author

    @isidentical isidentical commented Dec 23, 2020

    Since the annotations are processed just like all other expressions in the symbol table, the generated entries for functions etc. This would result with

    def foo():
        for number in range(5):
            foo: (yield number)
        return number
    
    foo()

    returning a generator / coroutine (depending on yield/yield from/await usage). Is this something we want to keep or maybe tweak the symbol table generator to not to handle annotations (since there are also more subtle issues regarding analysis of cell / free vars).

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Dec 23, 2020

    Ouch. I think we should generate a SyntaxError if yield [from] is used in an annotation.

    @isidentical
    Copy link
    Sponsor Member Author

    @isidentical isidentical commented Dec 23, 2020

    This is another side effect of processing annotations (at the symbol table construction stage) (and I would assume there are a few more cases like this);

    def foo():
        outer_var = 1
    
        def bar():
            inner_var: outer_var = T
        
        return bar
    
    inner = foo()
    print(inner.__closure__)

    In theory, there shouldn't be any cells / references to the variables from outer scope, but since we process the entry for the annotation and record outer_var as a free var it is listed here.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Dec 23, 2020

    The difference is that that just causes a slight inefficiency, while processing 'yield' makes the function a generator. So I don't feel as strongly about that.

    @isidentical
    Copy link
    Sponsor Member Author

    @isidentical isidentical commented Dec 23, 2020

    So I don't feel as strongly about that.

    Just to note, since I believe the solution for all this might be the same (not processing annotations at all, since they will be compiled to strings in the later stage). If we go down on that route, it will be simpler but we won't be able to raise SyntaxError's for "a: (yield)" / "b: (await/yield from x)".

    By the way, this is what happens if you try to use get_type_hints on a function where an argument is (yield):

    >>> typing.get_type_hints(a)
    Traceback (most recent call last):
      File "/usr/local/lib/python3.10/typing.py", line 537, in __init__
        code = compile(arg, '<string>', 'eval')
      File "<string>", line 1
    SyntaxError: 'yield' outside function
    
    During handling of the above exception, another exception occurred:
    
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "/usr/local/lib/python3.10/typing.py", line 1490, in get_type_hints
        value = ForwardRef(value)
      File "/usr/local/lib/python3.10/typing.py", line 539, in __init__
        raise SyntaxError(f"Forward reference must be an expression -- got {arg!r}")
    SyntaxError: Forward reference must be an expression -- got '(yield)'

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Dec 23, 2020

    If we simply ignored yield in the annotation, wouldn't we have the problem that

    def f(a: (yield)): pass

    silently changes from being a generator (in 3.9) to not being a generator (in 3.10)? That would be bad. I'd rather make this an error still. (But for nonlocals, not processing sounds fine.)

    @lysnikolaou
    Copy link
    Contributor

    @lysnikolaou lysnikolaou commented Dec 24, 2020

    I concur with Guido. I feel that making this an error is the best alternative we have.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Dec 24, 2020

    Okay, let's do it. It should probably a post-parse check (before we invoke the bytecode compiler but after we have the AST in hand).

    @isidentical
    Copy link
    Sponsor Member Author

    @isidentical isidentical commented Dec 25, 2020

    I have a patch ready to go, but I'd prefer to block this issue until bpo-42737 is resolved/decided.

    @isidentical
    Copy link
    Sponsor Member Author

    @isidentical isidentical commented Dec 26, 2020

    One thing to note here, currently Pablo and I are trying to bring annotation unparsing from the compiler to the parser in bpo-41967. If we do so, the annotations won't cause any side effects on the symbol table generation.

    @markshannon
    Copy link
    Member

    @markshannon markshannon commented Jan 6, 2021

    I've also opened bpo-42837 which is about fixing the symbol table, so that it is correct w.r.t. to current behavior.

    I'd like to fix it ASAP as the compiler should be able to rely on the symbol table being correct.

    Of course, once we have decided what the behavior should be, then it may need to be updated again.

    I'm inclined to agree that 'yield' in an annotation should probably be a syntax error, but I haven't put much thought into to.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jan 6, 2021

    Yield in an annotation should be a syntax error.

    @markshannon
    Copy link
    Member

    @markshannon markshannon commented Jan 7, 2021

    What's the process for making a decision on whether to make 'yield' in an annotation a syntax error?

    As a language change it should have a PEP, IMO.
    The PEP will be short, and shouldn't need a long-winded acceptance process.
    I just think that a PEP is more visible to the community than the bug tracker.

    I'm happy to write the PEP.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jan 7, 2021

    Should not "await" and "async for" (in comprehesions) and ":=" be forbidden too?

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jan 7, 2021

    I wouldn’t have thought you’d need a PEP for this but if you want to write one that sounds like the right thing.

    Re: async and walrus: I think those are different, their presence doesn’t affect the meaning of the function like yield. We can’t hope to prevent side effects syntactically.

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Jan 7, 2021

    Does not walrus affect the meaning of variable? And await affects the meaning of generator expression.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jan 8, 2021

    Oh, you’re right about walrus. And I don’t actually understand async generator expressions.

    This suggests that we definitely need a PEP. :-)

    @markshannon
    Copy link
    Member

    @markshannon markshannon commented Jan 18, 2021

    Draft PEP here
    https://github.com/markshannon/peps/blob/annotation-syntax-errors/pep-9999.rst

    Guido, would you like to be co-author as it was your idea to make these things a syntax error?

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jan 18, 2021

    It'll probably be quicker if you leave me out of the loop (feel free to quote me though). I am currently quite overwhelmed with PEP-sized discussions. I expect that the SC can rule on this quickly (just use their tracker to send them the PEP).

    @isidentical
    Copy link
    Sponsor Member Author

    @isidentical isidentical commented Jan 18, 2021

    The use of nonlocal variables in an annotation will be a syntax error in Python 3.10

    What is the reasoning for forbidding nonlocal variables (https://bugs.python.org/msg383659), can't we just treat them like regular variables and leave the value to whom needed to deal with resolving the scope?

    Also, you should preferably clarify other cases regarding the symbol table interaction of annotations. For example this case:

    Here is one;
    class T:
    def foo(self):
    a: super().bar() = x
    print(T.foo.__closure__)

    And if we are on the road to writing a PEP, maybe we should somehow squeeze bpo-42737 somewhere, since the annotations for complex targets are still evaluated and this makes everything a bit problematic and inconsistent. If you need collaboration on the PEP, just let me know (isidentical@gmail.com) (I have a patch ready to go for the symbol table to both make annotations ineffective and forbid this stuff but was waiting for bpo-42737)

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Jan 18, 2021

    I'm not keen on prohibiting nonlocal variable reference in annotations,
    since I can imagine uses for this (e.g. in tests). I'm not too worried
    about keeping the cell alive "forever", since with Larry's PEP-649 their
    lifetime would end when the object containing the annotation ends.

    Someone should look into how Batuhan's super()-in-annotation example would
    behave under Larry's PEP-649. (Using super() in the magical function Larry
    proposes to generate might not work anyway, since super()'s own magical
    powers don't work in a nested function.)

    Re: bpo-42737, not sure what to do for that, since neither PEP-563 nor PEP-649 gives a way to access those annotations. Then again, that's also true
    for annotations on local variables. So maybe we can treat them the same?
    Type checkers won't care, and runtime uses of annotations (whether as type
    hints or otherwise) won't have access......

    @isidentical
    Copy link
    Sponsor Member Author

    @isidentical isidentical commented Apr 23, 2021

    I've posted an entry on python-dev to collect comments about how we should act (whether just act them as strings on the symbol table or just forbid them completely): https://mail.python.org/archives/list/python-dev@python.org/thread/5NQH5GURAYW6XINPBO6VSF45CWLQOHUQ/

    Since we are almost days away from the cut, I will probably just submit a proper patch this weekend where we can debate more through reviews but first I need to know what kind of action we intend to take. (this all assumes PR 23952 is goes in).

    @larryhastings
    Copy link
    Contributor

    @larryhastings larryhastings commented Apr 23, 2021

    I think stringized annotations should prohibit the same things PEP-649 prohibits: walrus, yield / yield from, and await.

    This was easy in my 649 branch; walrus adds locals, and yield / yield from make it a generator. So the code raises an error if the generated annotations code object has locals or is a generator. I don't think I had to do anything special to prohibit await, because that's only valid in a function declared "async def", which annotations code objects are not.

    @isidentical
    Copy link
    Sponsor Member Author

    @isidentical isidentical commented Apr 23, 2021

    This was easy in my 649 branch; walrus adds locals, and yield / yield from make it a generator. So the code raises an error if the generated annotations code object has locals or is a generator. I don't think I had to do anything special to prohibit await, because that's only valid in a function declared "async def", which annotations code objects are not.

    The implementation I have right now just adds a new state to the symbol table, and raises error in the visitors (like switch Yield, YieldFrom etc).

    @larryhastings
    Copy link
    Contributor

    @larryhastings larryhastings commented Apr 23, 2021

    Wouldn't it be easier to just throw an exception in the stringizing code? I note that there are dedicated functions to generate walrus, yield, yield from, and await in Python/ast_unparse.c. In theory this is a general-purpose facility, but in practice it's not part of the limited API, and it's called from literally one place, the "stringize this annotation" call site inside compile.c.

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Apr 24, 2021

    So shouldn't we just rule out some specific bits of syntax in annotations? That can be done after the PEG grammar has accepted them, otherwise we'd end up having a whole new expression-like grammar section that excludes them. I think this is the list we should exclude, right?

    • yield [from]
    • walrus
    • await (?)

    I agree await technically doesn't need to be in this list, so maybe we shouldn't explicitly exclude it -- it's no different than writing

    def f(x: open(filename).read()):
        ...

    @isidentical
    Copy link
    Sponsor Member Author

    @isidentical isidentical commented Apr 24, 2021

    That can be done after the PEG grammar has accepted them

    But wouldn't we still end up with maintaining a custom flag to see if we are in annotation (e.g a: Something((yield) + 2)) and act upon that which would seem to do a bit messy in the grammar actions rather than just throwing syntax errors from switch/cases inside of symbol table?

    @gvanrossum
    Copy link
    Member

    @gvanrossum gvanrossum commented Apr 24, 2021

    I don’t really care how you do it, I just thought having a whole “expr-without-walrus-or-yield” subgrammar would be tedious.

    If there is a way to have the “in-an-annotation” flag set during parsing that may be better, I don’t know.

    Maybe Lysandros has an idea?

    @serhiy-storchaka
    Copy link
    Member

    @serhiy-storchaka serhiy-storchaka commented Apr 24, 2021

    There is no “expr-without-await-or-async-for” subgrammar, but "await" and asynchronous comprehensions are invalid in synchronous functions. There should be similar flag for annotations in symtable.c.

    @pablogsal
    Copy link
    Member

    @pablogsal pablogsal commented Apr 24, 2021

    I just thought having a whole “expr-without-walrus-or-yield” subgrammar would be tedious.

    Yeah I have experimented with something similar in the past and this becomes out of hand super quickly and leafs to a lot of duplication.

    Similarly, a flag that conditionally accept some rules can be quite tricky to hey right with the backtracking and those require some extra metagrammar so adding the flag is not super unreadable.

    In my humble opinion, I think is far simpler to reject this after the parsing is done, either in the compiler or the symbol table.

    @isidentical
    Copy link
    Sponsor Member Author

    @isidentical isidentical commented May 3, 2021

    New changeset ad106c6 by Batuhan Taskaya in branch 'master':
    bpo-42725: Render annotations effectless on symbol table with PEP-563 (GH-25583)
    ad106c6

    @pablogsal
    Copy link
    Member

    @pablogsal pablogsal commented May 3, 2021

    Batuhan, can this issue be closed?

    @isidentical
    Copy link
    Sponsor Member Author

    @isidentical isidentical commented May 3, 2021

    Batuhan, can this issue be closed?

    No, I still need to take care of what's new entry.

    @miss-islington
    Copy link
    Contributor

    @miss-islington miss-islington commented May 8, 2021

    New changeset 90d584a by Miss Islington (bot) in branch '3.10':
    bpo-42725: mention the changes on what's new (GH-25974)
    90d584a

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    None yet
    Development

    No branches or pull requests

    8 participants