GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-105834: Add tests for calling `issubclass()` between two protocols by AlexWaygood · Pull Request #105835 · python/cpython

Conversation

@AlexWaygood

@AlexWaygood AlexWaygood commented Jun 15, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member
  • Issue: Untested code in typing.py #105834

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@AlexWaygood
Remove dead code in typing.py
6ac4e7d
@AlexWaygood
AlexWaygood requested review from Fidget-Spinner, JelleZijlstra and gvanrossum as code owners June 15, 2023 18:25
@bedevere-bot bedevere-bot added the awaiting core review label Jun 15, 2023
@bedevere-bot bedevere-bot mentioned this pull request Jun 15, 2023
Untested code in typing.py #105834
Closed
@AlexWaygood AlexWaygood added the skip news label Jun 15, 2023
carljm
carljm approved these changes Jun 15, 2023
View reviewed changes

@carljm carljm left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
AlexWaygood reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Jun 15, 2023
JelleZijlstra
JelleZijlstra requested changes Jun 16, 2023
View reviewed changes

@JelleZijlstra JelleZijlstra left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting changes and removed awaiting merge labels Jun 16, 2023
@bedevere-bot

bedevere-bot commented Jun 16, 2023

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood

AlexWaygood commented Jun 16, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood
Fix
dd75fb3
@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

AlexWaygood commented Jun 16, 2023

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Jun 16, 2023
@bedevere-bot

bedevere-bot commented Jun 16, 2023

Copy link
Copy Markdown

Thanks for making the requested changes!

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot
bedevere-bot requested review from JelleZijlstra and carljm June 16, 2023 11:38
@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 only security fixes labels Jun 16, 2023
JelleZijlstra
JelleZijlstra reviewed Jun 16, 2023
View reviewed changes
Comment thread Lib/test/test_typing.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@AlexWaygood
Moar assertions
aad6502
@AlexWaygood
AlexWaygood requested a review from JelleZijlstra June 16, 2023 15:20
JelleZijlstra
JelleZijlstra approved these changes Jun 16, 2023
View reviewed changes
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting change review labels Jun 16, 2023
@AlexWaygood AlexWaygood mentioned this pull request Jun 16, 2023
Release 4.7.0 planning python/typing_extensions#241
Closed
@AlexWaygood
AlexWaygood enabled auto-merge (squash) June 16, 2023 15:33
@AlexWaygood
AlexWaygood merged commit 70c075c into python:main Jun 16, 2023
@miss-islington

miss-islington commented Jun 16, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting merge label Jun 16, 2023
@miss-islington

miss-islington commented Jun 16, 2023

Copy link
Copy Markdown
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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@miss-islington miss-islington assigned AlexWaygood Jun 16, 2023
@bedevere-bot

bedevere-bot commented Jun 16, 2023

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the needs backport to 3.12 only security fixes label Jun 16, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 16, 2023
@AlexWaygood @miss-islington
pythongh-105834: Add tests for calling issubclass() between two pro…
2aa26aa 
…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 <[email protected]>
@AlexWaygood
AlexWaygood deleted the typing-dead-code branch June 16, 2023 15:49
@AlexWaygood AlexWaygood mentioned this pull request Jun 16, 2023
Fix edge-case Protocol bug on Python 3.7 python/typing_extensions#242
Merged
AlexWaygood added a commit to AlexWaygood/cpython that referenced this pull request Jun 16, 2023
@AlexWaygood
[3.11] pythongh-105834: Add tests for calling issubclass() between …
7a3b537 
…two protocols (python#105835)

Some parts of the implementation of `typing.Protocol` had poor test coverage
@AlexWaygood AlexWaygood mentioned this pull request Jun 16, 2023
[3.11] gh-105834: Backport new tests for typing.Protocol (#105835) #105860
Merged
@bedevere-bot

bedevere-bot commented Jun 16, 2023

Copy link
Copy Markdown

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
@miss-islington @AlexWaygood
[3.12] gh-105834: Add tests for calling issubclass() between two pr…
e6982c5 
…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 <[email protected]>
JelleZijlstra pushed a commit that referenced this pull request Jun 16, 2023
@AlexWaygood
[3.11] gh-105834: Backport new tests for typing.Protocol (#105835) (#…
e590338 
…105860)
carljm added a commit to carljm/cpython that referenced this pull request Jun 16, 2023
@carljm
Merge branch 'main' into modulelocals
b096651 
* 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
@AlexWaygood @gvanrossum
pythongh-105834: Add tests for calling issubclass() between two pro…
ebd9bd1 
…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

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra approved these changes
@gvanrossum gvanrossum Awaiting requested review from gvanrossum
@Fidget-Spinner Fidget-Spinner Awaiting requested review from Fidget-Spinner
@carljm carljm Awaiting requested review from carljm

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@AlexWaygood AlexWaygood

Labels

skip news tests Tests in the Lib/test dir topic-typing

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@AlexWaygood @bedevere-bot @miss-islington @carljm @JelleZijlstra
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.