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

gh-105834: Add tests for calling issubclass() between two protocols #105835

Merged
merged 3 commits into from Jun 16, 2023

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Jun 15, 2023

Copy link
Member

@carljm carljm left a comment

Choose a reason for hiding this comment

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

Your reasoning on the issue looks right to me.

Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

We need to improve our test coverage instead. This block covers a case like this (add this to test_basic_protocol):

diff --git a/Lib/test/test_typing.py b/Lib/test/test_typing.py
index 3eb0fcad69..84e9f2650f 100644
--- a/Lib/test/test_typing.py
+++ b/Lib/test/test_typing.py
@@ -2474,6 +2474,11 @@ def f():
         self.assertNotIsSubclass(types.FunctionType, P)
         self.assertNotIsInstance(f, P)
 
+        class HasAnno(Protocol):
+            meth: Callable[[Any], Any]
+
+        self.assertIsSubclass(HasAnno, P)
+
     def test_runtime_checkable_generic_non_protocol(self):
         # Make sure this doesn't raise AttributeError
         with self.assertRaisesRegex(

On current main, this test passes, but with your patch applied, it no longer passes.

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Jun 16, 2023

Thanks @JelleZijlstra! I knew I had to be missing something 😖

Here's a complete repro:

>>> from typing import *
>>> @runtime_checkable
... class CallableMembersProto(Protocol):
...     def meth(self): ...
...
>>> class NonCallableMembersProto(Protocol):
...     meth: Callable[[], None]
...
>>> issubclass(NonCallableMembersProto, CallableMembersProto)
True

The last line would be False if we went ahead with this PR as it stands currently. I think you're right that this is definitely intended behaviour, and it seems untested currently.

I can see arguments both ways in terms of whether it's desirable behaviour, and it seems really unlikely to come up in practice. (PEP-544 doesn't seem to discuss this point at all.) But given that this has been established behaviour since Python 3.8, I agree that we should probably keep it. (I also can't find any significant performance improvement from removing this block of code.)

@AlexWaygood AlexWaygood changed the title gh-105834: Remove dead code in typing.py gh-105834: Add tests for calling issubclass() between two protocols Jun 16, 2023
@AlexWaygood
Copy link
Member Author

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

@carljm, @JelleZijlstra: please review the changes made to this pull request.

@AlexWaygood AlexWaygood added tests Tests in the Lib/test dir topic-typing needs backport to 3.11 only security fixes needs backport to 3.12 bug and security fixes labels Jun 16, 2023
@AlexWaygood AlexWaygood enabled auto-merge (squash) June 16, 2023 15:33
@AlexWaygood AlexWaygood merged commit 70c075c into python:main Jun 16, 2023
20 of 21 checks passed
@miss-islington
Copy link
Contributor

Thanks @AlexWaygood for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

Sorry, @AlexWaygood, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 70c075c194d3739ae10ce76265f05fa82ed46487 3.11

@bedevere-bot
Copy link

GH-105859 is a backport of this pull request to the 3.12 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.12 bug and security fixes label Jun 16, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 16, 2023
…tocols (pythonGH-105835)

Some parts of the implementation of `typing.Protocol` had poor test coverage
(cherry picked from commit 70c075c)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@AlexWaygood AlexWaygood deleted the typing-dead-code branch June 16, 2023 15:49
AlexWaygood added a commit to AlexWaygood/cpython that referenced this pull request Jun 16, 2023
…two protocols (python#105835)

Some parts of the implementation of `typing.Protocol` had poor test coverage
@bedevere-bot
Copy link

GH-105860 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jun 16, 2023
JelleZijlstra pushed a commit that referenced this pull request Jun 16, 2023
…otocols (GH-105835) (#105859)

Some parts of the implementation of `typing.Protocol` had poor test coverage
(cherry picked from commit 70c075c)

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
carljm added a commit to carljm/cpython that referenced this pull request Jun 16, 2023
* main:
  pythongh-104799: PEP 695 backward compatibility for ast.unparse (python#105846)
  pythongh-105834: Add tests for calling `issubclass()` between two protocols (python#105835)
  CI: Remove docs build from Azure Pipelines (python#105823)
  pythongh-105844: Consistently use 'minor version' for X.Y versions (python#105851)
  Fix inaccuracies in "Assorted Topics" section of "Defining Extension Types" tutorial (python#104969)
  pythongh-105433: Add `pickle` tests for PEP695 (python#105443)
  bpo-44530: Document the change in MAKE_FUNCTION behavior (python#93189)
  pythonGH-103124: Multiline statement support for pdb (pythonGH-103125)
  pythonGH-105588: Add missing error checks to some obj2ast_* converters (pythonGH-105589)
gvanrossum pushed a commit to gvanrossum/cpython that referenced this pull request Jun 18, 2023
…tocols (python#105835)

Some parts of the implementation of `typing.Protocol` had poor test coverage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir topic-typing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants