classification
Title: Del on class __annotations__ regressed, failing test
Type: behavior Stage: resolved
Components: Tests Versions: Python 3.8, Python 3.7, Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Mark.Shannon, gvanrossum, kayhayen, levkivskyi, serhiy.storchaka, terry.reedy
Priority: normal Keywords: patch

Created on 2018-07-17 08:15 by kayhayen, last changed 2018-07-24 12:11 by serhiy.storchaka. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 8364 merged serhiy.storchaka, 2018-07-21 03:40
PR 8365 merged miss-islington, 2018-07-21 04:44
PR 8366 merged miss-islington, 2018-07-21 04:45
PR 8437 merged serhiy.storchaka, 2018-07-24 11:02
Messages (20)
msg321807 - (view) Author: Kay Hayen (kayhayen) Date: 2018-07-17 08:15
I am getting this:

PYTHONPATH=`pwd` /c/Python37_32/python test/test_opcodes.py
.F......
======================================================================
FAIL: test_do_not_recreate_annotations (__main__.OpcodeTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "test/test_opcodes.py", line 45, in test_do_not_recreate_annotations
    class C:
  File "test/test_opcodes.py", line 48, in C
    x: int
AssertionError: NameError not raised

I have seen this on Linux as well. I first notices that as a regression of Nuitka in the CPython36 test suite. It actually took me a while to implement support for "del __annotations__" to make later references not fall back to the module "__annotation__", for 3.6 compatibility. 

However, now with 3.7 behavior is back to what 3.5 I think would have done, while the test is not updated to match.

I am confused now, which is the intended way for this to work? Should I follow this change, or will it be fixed, or am I doing something wrong in running something wrong here?

Yours,
Kay
msg321808 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-07-17 08:28
See issue32550.
msg321817 - (view) Author: Kay Hayen (kayhayen) Date: 2018-07-17 12:19
Thanks for pointing out, where it comes from, Serhiy.

So, should the test case be removed then. I still am not so sure about
the bug nature.

Because using the standard mechanism will do this:

x : int

class C:
    del __annotations__
    x : float
    y : int

print(__annotations__)

This will give float for x, and int for y, both of which are wrong for the module.

I do agree that "del" on "__annotations__" might not have a use case, or does it? I think
it's optimized away if not used for classes anyway, isn't it?

Maybe you want make "del" on __annotations__ a syntax error then?

Yours,
Kay
msg321833 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-07-17 14:47
Serhiy, what question do you want us to answer?

On Tue, Jul 17, 2018 at 5:22 AM Serhiy Storchaka <report@bugs.python.org>
wrote:

>
> Change by Serhiy Storchaka <storchaka+cpython@gmail.com>:
>
>
> ----------
> nosy: +Mark.Shannon, gvanrossum, levkivskyi
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue34136>
> _______________________________________
>
-- 
--Guido (mobile)
msg321838 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-07-17 15:14
Sorry. I thought the discussion issue32550 shows that this CPython behavior change was intended, but it is not a part of Python language, and alternate implementations can have different behavior. But I expected that someone of participants with better English can confirm this more clearly, and may be even suggest what to do with Nuitka's tests. (I think that Nuitka can do what is simpler for it.) I am wondering if it is worth to document this behavior change explicitly in the "What's New in 3.8" document.
msg321840 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-07-17 15:53
The test fails on CPython master, so shouldn't it be removed? Don't we have a policy that all tests must pass on master?

This is what I see:

cpython38$ ./python.exe Lib/test/test_opcodes.py 
./python.exe Lib/test/test_opcodes.py 
.F......
======================================================================
FAIL: test_do_not_recreate_annotations (__main__.OpcodeTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "Lib/test/test_opcodes.py", line 45, in test_do_not_recreate_annotations
    class C:
  File "Lib/test/test_opcodes.py", line 48, in C
    x: int
AssertionError: NameError not raised

----------------------------------------------------------------------
Ran 8 tests in 0.003s

FAILED (failures=1)
cpython38$
msg322050 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2018-07-21 00:38
Yes, the test should be removed, commented out, skipped, or made to work, depending on intentions.  Test first is for private branches.
msg322066 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-07-21 02:53
Oh, I missed that this issue is about the test from the CPython testsuite. I thought it is about the Numba specific tests.

It is strange that this test is passed when run as

    ./python -m test test_opcodes

but is failed when run as

    ./python -m test.test_opcodes

or

    ./python Lib/test/test_opcodes.py
msg322069 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-07-21 03:14
(Serhiy: I think you meant Nuitka, not numba -- Nuitka is a Python-to-C++ compiler, and Kay is its author. Somehow he managed to confuse us all. :-)

I suspect that the way `python3 -m test` runs the tests whose name you pass on the command line causes there to be no `__annotations__` global, while the way `python3 -m test.test_opcodes` runs its argument causes there to be one.  It seems that when a module is run as `__main__` it always is endowed with an `__annotations__` global, but when it is an imported module, `__annotations__` is only created when there is at least one top-level annotation.

I don't know why the global is always created for a `__main__`, but I suspect there's a good reason; the test will have to cope, because I think it should be able to run the tests either way.
msg322070 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-07-21 03:27
Sorry for my mistake, yes, I meant Nuitka.
msg322072 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-07-21 04:44
New changeset 06ca3f0c09d017b9d741553818459cca2d5da587 by Serhiy Storchaka in branch 'master':
bpo-34136: Make test_do_not_recreate_annotations more reliable. (GH-8364)
https://github.com/python/cpython/commit/06ca3f0c09d017b9d741553818459cca2d5da587
msg322084 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-07-21 08:12
In the edition of PR 8364 the test is failed on 3.6.

    def test_do_not_recreate_annotations(self):
        annotations = {}
        # Don't rely on the existence of the '__annotations__' global.
        with support.swap_item(globals(), '__annotations__', annotations):
            class C:
                del __annotations__
                x: int  # Updates the '__annotations__' global.
        self.assertIn('x', annotations)
        self.assertIs(annotations['x'], int)

It is possible to write it in less strong form which will be passed on 3.6 too.

    def test_do_not_recreate_annotations(self):
        # Don't rely on the existence of the '__annotations__' global.
        with support.swap_item(globals(), '__annotations__', {}):
            del globals()['__annotations__']
            class C:
                del __annotations__
                with self.assertRaises(NameError):
                    x: int

In 3.6 the annotation declaration fails immediately if __annotations__ is deleted. In 3.7 it falls back to the global __annotations__.

The question is whether the difference in the behavior between 3.6 and 3.7 is intended and is a part of the language specification, or is an implementation detail of CPython. In the former case we can keep the stronger test. In the latter case we should make it weaker.
msg322105 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-07-21 16:27
Thanks for the research. At this point I'm wondering what this test is testing. There is no prescribed behavior once you delete __annotations__ from the scope -- it is not a supported operation. At the same time I don't want to add any code enforcing that it fails. Unless you disagree, I think in 3.6 we should just delete the test.
msg322106 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-07-21 16:33
Shouldn't it be deleted in 3.7+ too? The behavior tested after PR 8364 (falling back to the global __annotations__) looks to me even more questionable than NameError.
msg322114 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-07-21 17:52
I wouldn’t object. Of course my opinion no longer matters that much.

On Sat, Jul 21, 2018 at 9:33 AM Serhiy Storchaka <report@bugs.python.org>
wrote:

>
> Serhiy Storchaka <storchaka+cpython@gmail.com> added the comment:
>
> Shouldn't it be deleted in 3.7+ too? The behavior tested after PR 8364
> (falling back to the global __annotations__) looks to me even more
> questionable than NameError.
>
> ----------
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <https://bugs.python.org/issue34136>
> _______________________________________
>
-- 
--Guido (mobile)
msg322134 - (view) Author: Kay Hayen (kayhayen) Date: 2018-07-22 07:13
As somebody whose opinion is even less important: Did you consider my suggestion to make it a "SyntaxError" for "del __annotations__" on a class level or even module level or at all? Or does this go too far?

Yours,
Kay
msg322169 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-07-22 21:57
> Did you consider my suggestion to make it a "SyntaxError" for
> "del __annotations__" on a class level or even module level or at all?
> Or does this go too far?

That's not reasonable. __annotations__ is just an identifier.
There are lots of other things that shouldn't be deleted --
should we try to enumerate them all and make them all syntax errors?
Surely not.
msg322291 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-07-24 12:05
New changeset c206f0d1375fab7b58c19a6be3e68e316f718c66 by Serhiy Storchaka in branch 'master':
bpo-34136: Make test_do_not_recreate_annotations more lenient. (GH-8437)
https://github.com/python/cpython/commit/c206f0d1375fab7b58c19a6be3e68e316f718c66
msg322292 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-07-24 12:06
New changeset 23a3297ff1076d91ca6d70caadf9606f1fee0776 by Serhiy Storchaka (Miss Islington (bot)) in branch '3.7':
[3.7] bpo-34136: Make test_do_not_recreate_annotations more reliable. (GH-8364) (GH-8365)
https://github.com/python/cpython/commit/23a3297ff1076d91ca6d70caadf9606f1fee0776
msg322293 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2018-07-24 12:06
New changeset 9b33ca0f4d459c165d711778734b6e1bdc3ec35d by Serhiy Storchaka (Miss Islington (bot)) in branch '3.6':
[3.6] bpo-34136: Make test_do_not_recreate_annotations more reliable. (GH-8364) (GH-8366)
https://github.com/python/cpython/commit/9b33ca0f4d459c165d711778734b6e1bdc3ec35d
History
Date User Action Args
2018-07-24 12:11:22serhiy.storchakasetstatus: open -> closed
stage: patch review -> resolved
resolution: fixed
components: + Tests, - Interpreter Core
versions: + Python 3.6, Python 3.8
2018-07-24 12:06:21serhiy.storchakasetmessages: + msg322293
2018-07-24 12:06:00serhiy.storchakasetmessages: + msg322292
2018-07-24 12:05:31serhiy.storchakasetmessages: + msg322291
2018-07-24 11:02:38serhiy.storchakasetpull_requests: + pull_request7962
2018-07-22 21:57:35gvanrossumsetmessages: + msg322169
2018-07-22 07:13:58kayhayensetmessages: + msg322134
2018-07-21 17:52:57gvanrossumsetmessages: + msg322114
2018-07-21 16:33:54serhiy.storchakasetmessages: + msg322106
2018-07-21 16:27:33gvanrossumsetmessages: + msg322105
2018-07-21 08:12:18serhiy.storchakasetmessages: + msg322084
2018-07-21 04:45:26miss-islingtonsetpull_requests: + pull_request7900
2018-07-21 04:44:14miss-islingtonsetpull_requests: + pull_request7899
2018-07-21 04:44:07serhiy.storchakasetmessages: + msg322072
2018-07-21 03:40:04serhiy.storchakasetkeywords: + patch
stage: patch review
pull_requests: + pull_request7898
2018-07-21 03:27:37serhiy.storchakasetmessages: + msg322070
2018-07-21 03:14:05gvanrossumsetmessages: + msg322069
2018-07-21 02:53:03serhiy.storchakasetmessages: + msg322066
2018-07-21 00:38:48terry.reedysetnosy: + terry.reedy
messages: + msg322050
2018-07-17 15:53:10gvanrossumsetmessages: + msg321840
2018-07-17 15:14:31serhiy.storchakasetmessages: + msg321838
2018-07-17 14:47:34gvanrossumsetmessages: + msg321833
2018-07-17 12:22:11serhiy.storchakasetnosy: + gvanrossum, Mark.Shannon, levkivskyi
2018-07-17 12:19:47kayhayensetmessages: + msg321817
2018-07-17 08:28:20serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg321808
2018-07-17 08:15:16kayhayencreate