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

bpo-38605: Revert making 'from __future__ import annotations' the default #25490

Merged
merged 5 commits into from
Apr 21, 2021

Conversation

pablogsal
Copy link
Member

@pablogsal pablogsal commented Apr 20, 2021

…ault

This reverts commit 044a104, adapting
the code to changes that happened after it.
@gousaiyang
Copy link
Contributor

Hi @pablogsal, you may also want to revert my previous PR #25236 for this.

@vstinner
Copy link
Member

vstinner commented Apr 20, 2021

Could you give a little bit context to explain the revert? I guess that it's related to the many threads on python-dev about it?

@pablogsal
Copy link
Member Author

Is explained in the NEWs entry. I plan to explain it also on the final commit :)

@vstinner
Copy link
Member

Oh ok, thanks. Copy of the NEWS entry:

Revert making 'from future import annotations' the default. This follows
the Steering Council decision to postpone PEP 563 changes to at least Python
3.11. See the original email for more information regarding the decision:
https://mail.python.org/archives/list/python-dev@python.org/thread/CLVXXPQ2T2LQ5MP2Y53VVQFCXYWQJHKZ/.
Patch by Pablo Galindo.

@isidentical
Copy link
Sponsor Member

isidentical commented Apr 20, 2021

@pablogsal also please bump the magic number (cached pycs might cause some issues) (it wasn't done in the initial PR, but rather get picked up on #22630)

@pablogsal
Copy link
Member Author

@pablogsal also please bump the magic number (cached pycs might cause some issues) (it wasn't done in the initial PR, but rather get picked up on #22630)

We should be able to go back a version, I think. Cached pycs for alpha versions should not take lots of consideration

@pablogsal
Copy link
Member Author

We should be able to go back a version, I think. Cached pycs for alpha versions should not take lots of consideration

Crap, there has been quite a lot of changes since then. Ok, bumping it is!

@pablogsal
Copy link
Member Author

@pablogsal also please bump the magic number (cached pycs might cause some issues) (it wasn't done in the initial PR, but rather get picked up on #22630)

Done in ef38777a1f

@vstinner
Copy link
Member

Oops sorry, I wanted to remove my comment rather posting it, but I closed the PR instead... I reopened the PR.

@gousaiyang
Copy link
Contributor

gousaiyang commented Apr 20, 2021

We can also update the doc of __future__ module and Lib/__future__.py to mention that annotations feature is postponed to be "mandatory in 3.11".

@pablogsal pablogsal closed this Apr 21, 2021
@pablogsal pablogsal reopened this Apr 21, 2021
@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 21, 2021
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 21, 2021
@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 21, 2021
@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 21, 2021
@methane

This comment has been minimized.

@pablogsal
Copy link
Member Author

cpython/Python/ceval.c

Lines 4363 to 4366 in d35eef3

if (oparg & 0x04) {
assert(PyTuple_CheckExact(TOP()));
func->func_annotations = POP();
}

This line should be changed to assert(PyTuple_CheckExact(TOP()) || PyDict_CheckExact(TOP()));.
I don't know why this pull request passes the test...

Are you taking #23316 into consideration?

@methane
Copy link
Member

methane commented Apr 21, 2021

Are you taking #23316 into consideration?

Yes. And I know understand why this assrtion passes.

def add(a: int, b: int) -> int:
    return a + b
  3           0 LOAD_CONST               0 ('a')
              2 LOAD_NAME                0 (int)
              4 LOAD_CONST               1 ('b')
              6 LOAD_NAME                0 (int)
              8 LOAD_CONST               2 ('return')
             10 LOAD_NAME                0 (int)
             12 BUILD_TUPLE              6
             14 LOAD_CONST               3 (<code object add at 0x7f30fc37b050, file "a.py", line 3>)
             16 LOAD_CONST               4 ('add')
             18 MAKE_FUNCTION            4 (annotations)
             20 STORE_NAME               1 (add)

Annotation is tuple even when PEP 563 is disabled. That's nice.

@python python deleted a comment from bedevere-bot Apr 21, 2021
@python python deleted a comment from bedevere-bot Apr 21, 2021
@pablogsal pablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 21, 2021
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 51a6f52234cf416f33a8f8ce625502dedd2bf0fd 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Apr 21, 2021
…05.9eeCNZ.rst

Co-authored-by: Inada Naoki <songofacandy@gmail.com>
@pablogsal
Copy link
Member Author

I still would like if someone can formally approve the PR :)

Copy link
Member

@methane methane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not reviewed tests yet. In other parts, LGTM.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but please address first the comment in Lib/test/test_dataclasses.py (I concur that it looks a revert of a bugfix, so it reintroduces a typo).

I suggest to first revert the change, and then write a second PR to announce that annotations are going to change again (in Python 3.11 or later).

@pablogsal
Copy link
Member Author

pablogsal commented Apr 21, 2021

I suggest to first revert the change, and then write a second PR to announce that annotations are going to change again (in Python 3.11 or later).

@vstinner What do you mean when you say "announce"? Announce where? In the What's new? A news entry? python-dev?

I suggest to first revert the change

Notice that this was certainly not a clean revert

@vstinner
Copy link
Member

@vstinner What do you mean when you say "announce"? Announce where? In the What's new? A news entry? python-dev?

Add a What's New in Python 3.10 entry to explain that the behavior changed but was then revert, and that it will change again in a future Python version.

Something like this announcement of future collections incompatible changes (collections.Mapping alias):
https://docs.python.org/dev/whatsnew/3.9.html#you-should-check-for-deprecationwarning-in-your-code

This announce was correct: the aliases were removed again in Python 3.10.

python-dev?

Any additional communication would be good, but the minimum would be a note in the What's New in Python 3.10.

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@pablogsal pablogsal merged commit b0544ba into python:master Apr 21, 2021
@pablogsal pablogsal deleted the revert-annotations branch April 21, 2021 11:41
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

Successfully merging this pull request may close these issues.

8 participants