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

Rename typing._collect_parameters #118900

Merged
merged 6 commits into from
May 10, 2024
Merged

Conversation

JelleZijlstra
Copy link
Member

function. Unfortunately, released versions of typing_extensions
monkeypatch this function without the extra parameter, which makes
it so things break badly if current main is used with typing_extensions.

% ~/py/cpython/python.exe test_typing_extensions.py
Traceback (most recent call last):
  File "/Users/jelle/py/typing_extensions/src/test_typing_extensions.py", line 42, in <module>
    from _typed_dict_test_helper import Foo, FooGeneric, VeryAnnotated
  File "/Users/jelle/py/typing_extensions/src/_typed_dict_test_helper.py", line 17, in <module>
    class FooGeneric(TypedDict, Generic[T]):
                                ~~~~~~~^^^
  File "/Users/jelle/py/cpython/Lib/typing.py", line 431, in inner
    return func(*args, **kwds)
  File "/Users/jelle/py/cpython/Lib/typing.py", line 1243, in _generic_class_getitem
    return _GenericAlias(cls, args)
  File "/Users/jelle/py/cpython/Lib/typing.py", line 1420, in __init__
    self.__parameters__ = _collect_parameters(
                          ~~~~~~~~~~~~~~~~~~~^
        args,
        ^^^^^
        enforce_default_ordering=enforce_default_ordering,
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
TypeError: _collect_parameters() got an unexpected keyword argument 'enforce_default_ordering'

Fortunately, the monkeypatching is not needed on Python 3.13, because CPython
now implements PEP 696. By renaming the function, we prevent the monkeypatch
from breaking typing.py internals.

function. Unfortunately, released versions of typing_extensions
monkeypatch this function without the extra parameter, which makes
it so things break badly if current main is used with typing_extensions.

```
% ~/py/cpython/python.exe test_typing_extensions.py
Traceback (most recent call last):
  File "/Users/jelle/py/typing_extensions/src/test_typing_extensions.py", line 42, in <module>
    from _typed_dict_test_helper import Foo, FooGeneric, VeryAnnotated
  File "/Users/jelle/py/typing_extensions/src/_typed_dict_test_helper.py", line 17, in <module>
    class FooGeneric(TypedDict, Generic[T]):
                                ~~~~~~~^^^
  File "/Users/jelle/py/cpython/Lib/typing.py", line 431, in inner
    return func(*args, **kwds)
  File "/Users/jelle/py/cpython/Lib/typing.py", line 1243, in _generic_class_getitem
    return _GenericAlias(cls, args)
  File "/Users/jelle/py/cpython/Lib/typing.py", line 1420, in __init__
    self.__parameters__ = _collect_parameters(
                          ~~~~~~~~~~~~~~~~~~~^
        args,
        ^^^^^
        enforce_default_ordering=enforce_default_ordering,
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    )
    ^
TypeError: _collect_parameters() got an unexpected keyword argument 'enforce_default_ordering'
```

Fortunately, the monkeypatching is not needed on Python 3.13, because CPython
now implements PEP 696. By renaming the function, we prevent the monkeypatch
from breaking typing.py internals.
@AlexWaygood
Copy link
Member

AlexWaygood commented May 10, 2024

There are some uses of this function in the wild, which this change would break... https://github.com/wyfo/apischema/blob/d48cc010c417e9c49db25b7a8683621b3c305b44/apischema/typing.py#L59-L62

And we could also fix the test failure over at typing_extensions with this diff:

--- a/src/typing_extensions.py
+++ b/src/typing_extensions.py
@@ -2836,7 +2836,8 @@ else:
 
         return tuple(parameters)
 
-    typing._collect_parameters = _collect_parameters
+    if not _PEP_696_IMPLEMENTED:
+        typing._collect_parameters = _collect_parameters

It's true that making a change in CPython rather than typing_extensions has the advantage that already-released versions of typing_extensions are fixed as well to-be-released versions of typing_extensions. But I think already-released versions of typing_extensions are badly broken anyway on Python 3.13, due to the error pointed out in python/typing_extensions#377 (comment)

@JelleZijlstra
Copy link
Member Author

But I think already-released versions of typing_extensions are badly broken anyway on Python 3.13, due to the error pointed out in python/typing_extensions#377 (comment)

I think that error shows up only if you use typing_extensions.TypeVar though, while the error this PR fixes breaks any use of Generic after typing_extensions is imported, which feels much worse.

There are some uses of this function in the wild, which this change would break... https://github.com/wyfo/apischema/blob/d48cc010c417e9c49db25b7a8683621b3c305b44/apischema/typing.py#L59-L62

I think we could address this by keeping an alias _collect_parameters = _collect_type_parameters (or a wrapper function that raises DeprecationWarning).

@AlexWaygood
Copy link
Member

Okay, sounds reasonable.

I think we could address this by keeping an alias _collect_parameters = _collect_type_parameters (or a wrapper function that raises DeprecationWarning).

Or we could do this in the __getattr__ method down at the bottom? (With do_the_deprecation_warning() replaced by the appropriate logic to do a deprecation warning)

--- a/Lib/typing.py
+++ b/Lib/typing.py
@@ -3770,6 +3770,9 @@ def __getattr__(attr):
     elif attr in {"ContextManager", "AsyncContextManager"}:
         import contextlib
         obj = _alias(getattr(contextlib, f"Abstract{attr}"), 2, name=attr, defaults=(bool | None,))
+    elif attr == "_collect_parameters":
+        do_the_deprecation_warning()
+        return _collect_type_parameters
     else:
         raise AttributeError(f"module {__name__!r} has no attribute {attr!r}")
     globals()[attr] = obj

Lib/typing.py Outdated Show resolved Hide resolved
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks!

Lib/test/test_typing.py Outdated Show resolved Hide resolved
Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@JelleZijlstra JelleZijlstra enabled auto-merge (squash) May 10, 2024 16:21
@JelleZijlstra JelleZijlstra merged commit ec9d12b into python:main May 10, 2024
31 checks passed
@miss-islington-app
Copy link

Thanks @JelleZijlstra for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 10, 2024
Unfortunately, released versions of typing_extensions
monkeypatch this function without the extra parameter, which makes
it so things break badly if current main is used with typing_extensions.

Fortunately, the monkeypatching is not needed on Python 3.13, because CPython
now implements PEP 696. By renaming the function, we prevent the monkeypatch
from breaking typing.py internals.

We keep the old name (raising a DeprecationWarning) to help other external users who call it.
(cherry picked from commit ec9d12b)

Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented May 10, 2024

GH-118917 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label May 10, 2024
@JelleZijlstra JelleZijlstra deleted the collectparams branch May 10, 2024 17:08
AlexWaygood pushed a commit that referenced this pull request May 10, 2024
Co-authored-by: Jelle Zijlstra <jelle.zijlstra@gmail.com>
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Ubuntu NoGIL 3.13 has failed when building commit b3074f0.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/1422/builds/9) and take a look at the build logs.
  4. Check if the failure is related to this commit (b3074f0) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/1422/builds/9

Failed tests:

  • test_eintr
  • test_math

Failed subtests:

  • test_flock - main.FNTLEINTRTest.test_flock
  • test_all - test.test_eintr.EINTRTests.test_all
  • test_wait_integer - test.test_multiprocessing_spawn.test_misc.TestWait.test_wait_integer
  • test_map_timeout - test.test_concurrent_futures.test_process_pool.ProcessPoolSpawnProcessPoolExecutorTest.test_map_timeout
  • test_lockf - main.FNTLEINTRTest.test_lockf

Summary of the results of the build (if available):

==

Click to see traceback logs
Traceback (most recent call last):
  File "/home/ubuntu/buildarea/3.13.itamaro-ubuntu-aws.nogil/build/Lib/test/_test_eintr.py", line 532, in test_lockf
    self._lock(fcntl.lockf, "lockf")
    ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ubuntu/buildarea/3.13.itamaro-ubuntu-aws.nogil/build/Lib/test/_test_eintr.py", line 517, in _lock
    raise Exception("failed to sync child in %.1f sec" % dt)
Exception: failed to sync child in 300.5 sec


Traceback (most recent call last):
  File "/home/ubuntu/buildarea/3.13.itamaro-ubuntu-aws.nogil/build/Lib/test/test_eintr.py", line 17, in test_all
    script_helper.run_test_script(script)
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^
  File "/home/ubuntu/buildarea/3.13.itamaro-ubuntu-aws.nogil/build/Lib/test/support/script_helper.py", line 316, in run_test_script
    raise AssertionError(f"{name} failed")
AssertionError: script _test_eintr.py failed


Traceback (most recent call last):
  File "/home/ubuntu/buildarea/3.13.itamaro-ubuntu-aws.nogil/build/Lib/test/_test_multiprocessing.py", line 5176, in test_wait_integer
    self.assertLess(delta, expected + 2)
    ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^
AssertionError: 5.133446088992059 not less than 5


Traceback (most recent call last):
  File "/home/ubuntu/buildarea/3.13.itamaro-ubuntu-aws.nogil/build/Lib/test/_test_eintr.py", line 535, in test_flock
    self._lock(fcntl.flock, "flock")
    ~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ubuntu/buildarea/3.13.itamaro-ubuntu-aws.nogil/build/Lib/test/_test_eintr.py", line 517, in _lock
    raise Exception("failed to sync child in %.1f sec" % dt)
Exception: failed to sync child in 300.5 sec


Traceback (test.test_zipimport.CompressedZipImportTestCase.testTraceback) ... ok


Traceback (test.test_zipimport.UncompressedZipImportTestCase.testTraceback) ... ok


Traceback (most recent call last):
  File "/home/ubuntu/buildarea/3.13.itamaro-ubuntu-aws.nogil/build/Lib/test/test_concurrent_futures/executor.py", line 71, in test_map_timeout
    self.assertEqual([None, None], results)
    ~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: Lists differ: [None, None] != []

estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
Unfortunately, released versions of typing_extensions
monkeypatch this function without the extra parameter, which makes
it so things break badly if current main is used with typing_extensions.

Fortunately, the monkeypatching is not needed on Python 3.13, because CPython
now implements PEP 696. By renaming the function, we prevent the monkeypatch
from breaking typing.py internals.

We keep the old name (raising a DeprecationWarning) to help other external users who call it.
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.

3 participants