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

Add PEP 673 Self type #933

Merged
merged 14 commits into from Nov 12, 2021
Merged

Add PEP 673 Self type #933

merged 14 commits into from Nov 12, 2021

Conversation

@Gobot1234
Copy link
Contributor

@Gobot1234 Gobot1234 commented Nov 11, 2021

Adds support for PEP 673 and the special form Self, pyright already assumes this is here so it would be good to get runtime support for this.

Co-authored-by: Pradeep Kumar Srinivasan gohanpra@gmail.com

@pradeep90

@the-knights-who-say-ni
Copy link

@the-knights-who-say-ni the-knights-who-say-ni commented Nov 11, 2021

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).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@Gobot1234

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

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

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

@srittau
Copy link
Collaborator

@srittau srittau commented Nov 11, 2021

For reference: @pradeep90 has also signed the CLA according to https://check-python-cla.herokuapp.com/.

Copy link
Collaborator

@srittau srittau left a comment

Thanks. Please also add an entry to typing_extensions/CHANGELOG.

@Gobot1234
Copy link
Contributor Author

@Gobot1234 Gobot1234 commented Nov 11, 2021

What should be done test wise for this?

@srittau
Copy link
Collaborator

@srittau srittau commented Nov 11, 2021

I'd say have a look at the existing tests in src_py3/test_typing_extensions.py, but overall the tests can be fairly lean as there isn't much that can go wrong implementation-wise. Ideally, all code paths should be exercised once as a smoke test.

@Gobot1234 Gobot1234 changed the title Add Self type Add PEP 673 Self type Nov 11, 2021
@Gobot1234
Copy link
Contributor Author

@Gobot1234 Gobot1234 commented Nov 11, 2021

I'm not entirely clear as to why the 3.6/7 tests fail? Is typing extensions being in the venv being preferred over the local version?

@srittau
Copy link
Collaborator

@srittau srittau commented Nov 11, 2021

That's weird indeed. Even if the venv version was preferred, it should fail in Python 3.8+ as well.

Edit: But yes, it's trying to import /opt/hostedtoolcache/Python/3.7.12/x64/lib/python3.7/site-packages/typing_extensions.py, not the local version. Maybe try to import .typing_extensions?

@JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Nov 11, 2021

Is the problem that something else in the venv is also installing typing-extensions? Maybe we can try explicitly uninstalling it before running the tests.

@JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Nov 11, 2021

Yes, it's because importlib-metadata requuires typing-extensions (https://github.com/python/importlib_metadata/blob/main/setup.cfg#L21), and it's a dependency for a few of our test helpers (pytest, flake8). it apparently started doing that sometime this year, which is why we're only noticing now.

@Gobot1234
Copy link
Contributor Author

@Gobot1234 Gobot1234 commented Nov 11, 2021

Maybe we can try explicitly uninstalling it before running the tests.

So put subprocess.run([sys.executable, "-m", "pip", "uninstall", "typing-extensions"]) at the top of the tests?

@JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Nov 11, 2021

Better to just run it in the GitHub action directly.

@srittau
Copy link
Collaborator

@srittau srittau commented Nov 11, 2021

I've just merge #932, so there are conflicts now, but I think they should be easy to fix.

@JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Nov 11, 2021

Hm, that doesn't work either because apparently our copy of typing-extensions isn't on the $PATH yet when we try to import pytest.

Possible ideas:

  • Drop pytest and just use unittest, which is in the stdlib and shouldn't have dependencies. Do we even need pytest features for anything?
  • Wrangle the $PYTHONPATH before we invoke pytest so that it invokes the copy of typing-extensions in the repo. But that's hacky, and ultimately also makes it hard for people to run tests locally.

@JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Nov 11, 2021

I would prefer the first approach but it might be better to do it in a separate PR. I can do that tonight if nobody else gets to it first.

For reference this is the deptree:

 % pipdeptree --python ~/py/venvs/venvtyping/bin/python
flake8-bugbear==21.9.2
  - attrs [required: >=19.2.0, installed: 21.2.0]
  - flake8 [required: >=3.0.0, installed: 4.0.1]
    - importlib-metadata [required: <4.3, installed: 4.2.0]
      - typing-extensions [required: >=3.6.4, installed: 3.10.0.2]
      - zipp [required: >=0.5, installed: 3.6.0]
    - mccabe [required: >=0.6.0,<0.7.0, installed: 0.6.1]
    - pycodestyle [required: >=2.8.0,<2.9.0, installed: 2.8.0]
    - pyflakes [required: >=2.4.0,<2.5.0, installed: 2.4.0]
flake8-pyi==20.10.0
  - attrs [required: Any, installed: 21.2.0]
  - flake8 [required: >=3.2.1, installed: 4.0.1]
    - importlib-metadata [required: <4.3, installed: 4.2.0]
      - typing-extensions [required: >=3.6.4, installed: 3.10.0.2]
      - zipp [required: >=0.5, installed: 3.6.0]
    - mccabe [required: >=0.6.0,<0.7.0, installed: 0.6.1]
    - pycodestyle [required: >=2.8.0,<2.9.0, installed: 2.8.0]
    - pyflakes [required: >=2.4.0,<2.5.0, installed: 2.4.0]
  - pyflakes [required: >=2.1.1, installed: 2.4.0]
pipdeptree==2.2.0
  - pip [required: >=6.0.0, installed: 18.1]
pytest==6.2.5
  - attrs [required: >=19.2.0, installed: 21.2.0]
  - importlib-metadata [required: >=0.12, installed: 4.2.0]
    - typing-extensions [required: >=3.6.4, installed: 3.10.0.2]
    - zipp [required: >=0.5, installed: 3.6.0]
  - iniconfig [required: Any, installed: 1.1.1]
  - packaging [required: Any, installed: 21.2]
    - pyparsing [required: >=2.0.2,<3, installed: 2.4.7]
  - pluggy [required: >=0.12,<2.0, installed: 1.0.0]
    - importlib-metadata [required: >=0.12, installed: 4.2.0]
      - typing-extensions [required: >=3.6.4, installed: 3.10.0.2]
      - zipp [required: >=0.5, installed: 3.6.0]
  - py [required: >=1.8.2, installed: 1.11.0]
  - toml [required: Any, installed: 0.10.2]
setuptools==40.6.2

We'll also have to get rid of flake8, probably by running linting in a different venv from testing.

@srittau
Copy link
Collaborator

@srittau srittau commented Nov 11, 2021

Drop pytest and just use unittest, which is in the stdlib and shouldn't have dependencies. Do we even need pytest features for anything?

I think this is the best idea. We can run flake8 in a separate job, so that it installing typing-extensions shouldn't matter.

@hauntsaninja
Copy link
Collaborator

@hauntsaninja hauntsaninja commented Nov 11, 2021

typing-extensions is widely enough used that it seems really easy for this to be reintroduced. We can leave a big warning, but another approach to consider is putting --no-deps in test-requirements.txt and just listing out the exact set of all dependencies we need (e.g. from pip freeze).

@JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Nov 11, 2021

I'm hoping that we just won't have any dependencies in the tests. This seems achievable because that's what the stdlib does, and typing-extensions is essentially a back/forwardported piece of stdlib.

@srittau
Copy link
Collaborator

@srittau srittau commented Nov 11, 2021

#935 hopefully fixes CI.

@JelleZijlstra JelleZijlstra merged commit 60aa1e2 into python:master Nov 12, 2021
7 checks passed
@mimi89999
Copy link

@mimi89999 mimi89999 commented Dec 7, 2021

Hello,

Would it be possible to use this for nested classes?

class Parent:
    class A:
        b: int
        c: int
    class Z:
        z: Self.A

@Gobot1234
Copy link
Contributor Author

@Gobot1234 Gobot1234 commented Dec 7, 2021

This isn't the right place to ask questions about the PEP (see the python-dev mailing list for that).

But for what it's worth the PEP states Self should be evaluated in the enclosing class's (Z) namespace so Self.A would attribute error if this were possible but to me this doesn't make much sense and it would be a big change to both the PEP and the implementation to add support for this at runtime.

NB. I don't mind the idea of allowing attribute access on types/type aliases in the class namespace. This is something that rust does although you might just be better off making your class generic to work around this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants