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: Fixed a nested class example in #2007

Merged
merged 1 commit into from
Jun 28, 2021
Merged

Conversation

superbobry
Copy link
Contributor

@superbobry superbobry commented Jun 28, 2021

The "Backward Compatibility" section in PEP-563 contains an example which
violates Python scoping rules. This PR updates the PEP to say that the example
is not OK and should fail.

See discussion in typing-sig for details *.

@superbobry superbobry requested a review from ambv as a code owner June 28, 2021 12:17
@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@superbobry

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

The "Backward Compatibility" section in PEP-563 contains an example which
violates Python scoping rules. This PR updates the PEP to say that the example
is not OK and should fail.

See discussion in typing-sig for details [*].

[*]: https://mail.python.org/archives/list/typing-sig@python.org/thread/F2ERQCGB6W6VADR7G6NN4TLMFJECD6EW/
@JelleZijlstra
Copy link
Member

Thanks! You'll have to sign the CLA and I'd also like to see signoff from @ambv.

@superbobry
Copy link
Contributor Author

@JelleZijlstra I have submitted the CLA form, but IIUC the processing will take ~24h. Look forward to hearing from @ambv!

@gvanrossum
Copy link
Member

I'd really like @ambv's review of this.


def method(self) -> field2: # this is OK
...

def method(self) -> field: # this FAILS, class D doesn't
... # see C's attributes, This was
... # see C's attributes. This was
# already true before this PEP.
Copy link
Member

Choose a reason for hiding this comment

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

Why mention this here and not in the previous example? And why not show the correct way (C.field) like you did there?

@ambv
Copy link
Contributor

ambv commented Jun 28, 2021

Ah, this is an example of a bad practice in PEP writing: aspirational truths. At the time reviewers of the PEP were disappointed with nested class scoping not working, so somebody had the idea that typing.get_type_hints() should special-case methods and classes by injecting their name to globalns, essentially making this automatic:

>>> from __future__ import annotations
>>> from typing import *
>>> class C:
...   class D:
...     def meth(self) -> D:
...       ...
...
>>> C.D
<class '__main__.C.D'>
>>> get_type_hints(C.D.meth)
Traceback (most recent call last):
  ...
NameError: name 'D' is not defined
>>> get_type_hints(C.D.meth, globalns=dict(C.__dict__))
{'return': <class '__main__.C.D'>}

In practice implementing this wasn't trivial and I never gotten to fixing this case. That it contradicts scoping rules, sure, but would still be a useful addition to get_type_hints() because there's a more important case which the example in the PEP doesn't mention: functions which are class factories. So, to make the following work:

>>> class B:
...   ...
...
>>> def f() -> B:
...   class A(B):
...     def meth(self) -> A:
...       ...
...   return A
...
>>> A_ = f()
>>> get_type_hints(A_.meth)
Traceback (most recent call last):
  ...
NameError: name 'A' is not defined
>>> g2 = dict(globals())
>>> g2[A_.__name__] = A_
>>> get_type_hints(A_.meth, globalns=g2)
{'return': <class '__main__.f.<locals>.A'>}

WDYT @gvanrossum, is this still worth touching? If so, I'll prepare a fix to 3.10 get_type_hints(). If not, then I guess updating the PEP with this PR here is the way to go.

@gvanrossum
Copy link
Member

I don't think it is worth fixing -- it will just proliferate questions about scopes here, and AFAIK all the static checkers implement the scope rules as used by the runtime, so get_type_hints() would be the odd one out.

@ambv ambv merged commit 9bbc154 into python:master Jun 28, 2021
superbobry added a commit to superbobry/peps that referenced this pull request Jun 29, 2021
This PR fixes the wording in comments for filing examples in the 
"Backwads Compatibility" section as suggested by Guido in python#2007.
superbobry added a commit to superbobry/peps that referenced this pull request Jun 29, 2021
This commit fixes the wording in the comments for failing examples
in the "Backwads Compatibility" section, as suggested by Guido in python#2007.
@superbobry superbobry deleted the patch-1 branch June 29, 2021 12:20
@superbobry
Copy link
Contributor Author

Thanks for the context, Łukasz!

gvanrossum pushed a commit that referenced this pull request Jun 29, 2021
This commit fixes the wording in the comments for failing examples
in the "Backwards Compatibility" section, as suggested by Guido in #2007.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants