Skip to content

Third-party tests failed on Fri Jun 02 2023 #213

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

Closed
github-actions bot opened this issue Jun 2, 2023 · 11 comments · Fixed by #218
Closed

Third-party tests failed on Fri Jun 02 2023 #213

github-actions bot opened this issue Jun 2, 2023 · 11 comments · Fixed by #218

Comments

@github-actions
Copy link

github-actions bot commented Jun 2, 2023

Runs listed here: https://github.com/python/typing_extensions/actions/workflows/third_party.yml

@AlexWaygood
Copy link
Member

AlexWaygood commented Jun 2, 2023

cattrs's tests are failing on Python 3.7 with the latest typing_extensions: https://github.com/python/typing_extensions/actions/runs/5151301185/jobs/9276336770#step:9:106

I can reproduce the failures locally in a clean virtual environment on 3.7. The specific test that's failing is test_unstructure_protocol here: https://github.com/python-attrs/cattrs/blob/8beacd7ae557511f1ec9da8ab2c1bcdf9f20247f/tests/test_generics.py#L240-L259

I've bisected the failure to this typing_extensions commit: f9d21b1

Haven't yet done any investigation to figure out whether this is a typing_extensions regression, or whether cattrs is making some incorrect assumptions about typing_extensions.Protocol.

AlexWaygood added a commit to AlexWaygood/typing_extensions that referenced this issue Jun 2, 2023
Temporary workaround for python#213 until the tests pass on 3.7 again
@AlexWaygood
Copy link
Member

#214 skips cattrs tests on 3.7 until we can figure out what's going wrong.

@AlexWaygood
Copy link
Member

AlexWaygood commented Jun 2, 2023

The reason why cattrs's tests are only failing on Python 3.7, is that cattrs assumes typing_extensions.Protocol will be the same object as typing.Protocol on Python 3.8+: https://github.com/python-attrs/cattrs/blob/8beacd7ae557511f1ec9da8ab2c1bcdf9f20247f/src/cattrs/_compat.py#L52-L63 1.

So cattrs doesn't test with typing_extensions.Protocol on Python 3.8+.

Footnotes

  1. This is an incorrect assumption as of typing_extensions==4.6.0, in which we backported lots of bugfixes and performance improvements from Python 3.12 to typing_extensions.Protocol. This means that typing_extensions.Protocol is now a different object to typing.Protocol on all Python versions <3.12.

@AlexWaygood
Copy link
Member

Cc. @Tinche for cattrs expertise -- curious if you can seen an obvious reason why f9d21b1 might be breaking cattrs's tests on Python 3.7? :-)

I'm not too familiar with the architecture of cattrs, unfortunately. This was picked up by a new daily workflow we've added to detect when changes in typing_extensions break the CI for third-party projects that use typing_extensions: https://github.com/python/typing_extensions/blob/main/.github/workflows/third_party.yml

@Tinche
Copy link

Tinche commented Jun 2, 2023

Thanks @AlexWaygood , I'll take a look!

@Tinche
Copy link

Tinche commented Jun 2, 2023

Here's a helper function from cattrs:

def is_protocol(type: Any) -> bool:
    return issubclass(type, Protocol) and getattr(type, "_is_protocol", False)

On typing_extensions==4.6.3, this raises, but on 4.6.2, it works. That's to be expected, right?

The failing test is trying to match on

class Proto(Protocol):
    a: int

but that should also now be marked runtime_checkable, right?

@AlexWaygood
Copy link
Member

Thanks @Tinche, that helps me diagnose the problem! And I think it is a regression in typing_extensions (and on Python 3.12...).

On Python 3.11, you can do this:

>>> import typing
>>> issubclass(object, typing.Protocol)
False

But on Python 3.12, you can't anymore:

>>> import typing
>>> issubclass(object, typing.Protocol)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Users\alexw\coding\cpython\Lib\typing.py", line 1797, in __subclasscheck__
    raise TypeError(
TypeError: Instance and class checks can only be used with @runtime_checkable protocols

The new behaviour doesn't make much sense to me. That TypeError should only be triggered if you're trying to call issubclass() against subclasses of Protocol, not if you're calling issubclass() against Protocol itself.

So I think this needs to be fixed in CPython and typing_extensions.

@AlexWaygood
Copy link
Member

@Tinche
Copy link

Tinche commented Jun 2, 2023

I think that's unrelated to my test though.

Here's a minimal repro:

from typing_extensions import Protocol


class Proto(Protocol):
    a: int


def is_protocol(type) -> bool:
    return issubclass(type, Protocol) and getattr(type, "_is_protocol", False)


is_protocol(Proto)

(so I am calling it against a subclass of protocol.)

On 4.6.3 this raises, on 4.6.2 it returns True. That's the expected behavior now, since it's not runtime_checkable?

@AlexWaygood
Copy link
Member

AlexWaygood commented Jun 2, 2023

It raises because of your issubclass(type, Protocol) call inside the is_protocol function rather than because Proto is not decorated with @runtime_checkable. You're never actually calling isinstance() or issubclass() with Proto as the second argument, so whether or not Proto is decorated with @runtime_checkable should be irrelevant.

@Tinche
Copy link

Tinche commented Jun 2, 2023

Ah right I get it, runtime_checkable is for checking instances against Proto, not Proto against Protocol.

JelleZijlstra pushed a commit that referenced this issue Jun 3, 2023
Temporary workaround for #213 until the tests pass on 3.7 again
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 a pull request may close this issue.

2 participants