bpo-34939: Allow annotated global names in module namespace

[pablogsal]

https://bugs.python.org/issue34939

[tirkarthi]

@ilevkivskyi Is there any documentation update needed with respect to https://bugs.python.org/issue34939#msg327386 ?

Thanks

[ilevkivskyi]

Thanks for working on this! Here are some comments.

[ilevkivskyi]

Adding a new symbol table flag to solve this looks like an overkill (plus TBH I don't fully understand this solution). Why not just skip emitting this error at global scope? For example check that st->st_cur->ste_symbols != st->st_global?

[ilevkivskyi]

I would probably also add a comment here explaining that annotation forces a variable to be local for current scope, while global forces it to be global, so these two "directives" are conflicting, unless we are currently at the global scope.

[pablogsal]

Thanks for the review @ilevkivskyi. This is the first time I touch the symtable so sorry if the original proposed solution was a bit convoluted. I have simplified it in 701bffe.

[ilevkivskyi]

I would reserve this test file only for basic things. I propose to add this test to either test_symtable.py or test_syntax.py.

Also there is even a simpler repro for the issue:

global x
x: int  # SyntaxError

plus I would add test cases for nonlocal as well (even though it already works).

[pablogsal]

It seems that test_syntax tests all raise SyntaxErrors, but in this case, we want to test that the solution is valid. On the other hand, test_symtable seems to test the symtable properties directly, as opposed to check that something is a valid Syntax or not. I am a bit unsure on which file to put the reproducer (along with the "nonlocal" one).

[pablogsal]

I have moved the tests to test_symtable in 0e21eba.

[pablogsal]

I have openend #9872 as there are no current way to test for nonlocal declarations in the symtable python module.

[ilevkivskyi]

@tirkarthi I don't have ideas for better wording. Maybe just note that this is an error only for non-global scope (because annotation forces a variable to be local for current scope)?

[ilevkivskyi]

Thanks! I have one additional suggestion. Feel free to merge after you address it.

[ilevkivskyi]

Complex multi-line tests are easier to read if you split them like this:

        st6 = symtable.symtable('def g():\n'
                                '    x = 2\n'
                                '    def f():\n'
                                '        nonlocal x\n'
                                '    x: int',
                                'test', 'exec')

(Also I would stick to PEP 8 even in tests, see other test above. In this case space around = and space after : in annotation.)

[pablogsal]

Done in 547dbba. I would like to add an extra nonlocal check to st6 after if #9872 is merged.

[pablogsal]

@ilevkivskyi Thank you very much for the review and the helpful advice :)