FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-105144: abc: Suppress errors raised by unrelated other subclasses by JelleZijlstra · Pull Request #105159 · python/cpython · GitHub

/ cpython Public

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

Closed
JelleZijlstra wants to merge 1 commit into
python:mainfrom
JelleZijlstra:abcsub
Closed

gh-105144: abc: Suppress errors raised by unrelated other subclasses#105159
JelleZijlstra wants to merge 1 commit into
python:mainfrom
JelleZijlstra:abcsub

Conversation

JelleZijlstra commented May 31, 2023
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

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

…asses

A more invasive solution for python#105144; perhaps better kept for 3.13 only.
Comment thread Lib/_py_abc.py
if issubclass(subclass, rcls):
cls._abc_cache.add(subclass)
return True
except Exception:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Honestly, I feel pretty uncomfortable about suppressing all subclasses of Exception -- could we just do TypeError? If I had a typo in a custom __subclasscheck__ method, I'm not sure I'd want e.g. the resulting AttributeError to be silenced because of the fact that ABCMeta was being used somewhere

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Copy link
Copy Markdown
Member

Looks like there's still lots of test_typing failures if I apply this diff to your PR branch:

diff --git a/Lib/typing.py b/Lib/typing.py
index 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}

Copy link
Copy Markdown
Member

The trouble is that these __subclasscheck__ calls call into _ProtocolMeta.__subclasscheck__, which might raise:

cpython/Lib/_py_abc.py

Lines 105 to 106 in ec0082c

return cls.__subclasscheck__(subclass)
return any(cls.__subclasscheck__(c) for c in (subclass, subtype))

But we don't want it to raise if the __subclasscheck__ call is coming from an __instancecheck__ call.

AlexWaygood commented Jun 1, 2023
edited
Loading

Copy link
Copy Markdown
Member

Ultimately, it does sort of feel like typing is the module that's "in the wrong" here. The abc module makes the assumption that issubclass() calls will never raise, providing both arguments are instances of type. 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's typing that should have to do the awkward workaround. It's unfortunate how fragile that makes some of the code in typing.py, but I think PEP-544's semantics are too well-established now to change the behaviour.

Copy link
Copy Markdown
Member Author

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL