Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-105144: abc: Suppress errors raised by unrelated other subclasses#105159

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
JelleZijlstra wants to merge1 commit intopython:mainfromJelleZijlstra:abcsub

Conversation

@JelleZijlstra
Copy link
Member

@JelleZijlstraJelleZijlstra commentedMay 31, 2023
edited by bedevere-bot
Loading

A more invasive solution for#105144; perhaps better kept for 3.13 only.

…assesA more invasive solution forpython#105144; perhaps better kept for 3.13 only.
ifissubclass(subclass,rcls):
cls._abc_cache.add(subclass)
returnTrue
exceptException:
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

In the C code I actually suppress all exceptions, not just Exception. Not sure what the right thing to do is.

Copy link
Member

Choose a reason for hiding this comment

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

I'd say suppressing all exceptions would definitely be bad -- that would mean that we'd be suppressing e.g.KeyboardInterrupt, andSystemExit.

Honestly, I feel pretty uncomfortable about suppressing all subclasses ofException -- could we just doTypeError? If I had a typo in a custom__subclasscheck__ method, I'm not sure I'd want e.g. the resultingAttributeError to be silenced because of the fact thatABCMeta was being used somewhere

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

You'd still see the error if you use your broken class directly. What this PR changes is that you'll no longer see the error if you use issubclass() checks on a separate class that inherits from the same ABC.

@AlexWaygood
Copy link
Member

Looks like there's stilllots oftest_typing failures if I apply this diff to your PR branch:

diff --git a/Lib/typing.py b/Lib/typing.pyindex 8c874797d2..8caeb915d8 100644--- a/Lib/typing.py+++ b/Lib/typing.py@@ -1733,7 +1733,7 @@ def _allow_reckless_class_checks(depth=3):     The abc and functools modules indiscriminately call isinstance() and     issubclass() on the whole MRO of a user class, which may contain protocols.     """-    return _caller(depth) in {'abc', 'functools', None}+    return _caller(depth) in {'functools', None}

@AlexWaygood
Copy link
Member

The trouble is that these__subclasscheck__ calls call into_ProtocolMeta.__subclasscheck__, which might raise:

cpython/Lib/_py_abc.py

Lines 105 to 106 inec0082c

returncls.__subclasscheck__(subclass)
returnany(cls.__subclasscheck__(c)forcin (subclass,subtype))

But we don't want it to raise if the__subclasscheck__ call is coming from an__instancecheck__ call.

@AlexWaygood
Copy link
Member

AlexWaygood commentedJun 1, 2023
edited
Loading

Ultimately, it does sort of feel liketyping is the module that's "in the wrong" here. Theabc module makes the assumption thatissubclass() calls will never raise, providing both arguments are instances oftype. That's true for the vast majority of classes, so I think it's a reasonable assumption to make.typing is the module that's doing a surprising/unusual thing here, so it seems reasonable to say that it'styping that should have to do the awkward workaround. It's unfortunate how fragile that makes some of the code intyping.py, but I thinkPEP-544's semantics are too well-established now to change the behaviour.

@JelleZijlstra
Copy link
MemberAuthor

I feel like this change isn't worth the churn.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@AlexWaygoodAlexWaygoodAwaiting requested review from AlexWaygood

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JelleZijlstra@AlexWaygood@bedevere-bot

[8]ページ先頭

©2009-2025 Movatter.jp