Comments (8)
+1 for removing subclasshook.
from bidict.
I think I found the issue. It is in https://github.com/jab/bidict/blob/master/bidict/_abc.py in the subclasshook method.
NotImplemented is somehow not None so the check on line 108 fails to do what it should be doing and any class with an inverse method defined on it becomes a subclass of collections.abc.Mapping.
import bidict
import collections.abc as cabc
class DummyWithInverse(object):
def inverse(self, x):
return x
issubclass(DummyWithInverse,cabc.Mapping)
True
from bidict.
Sorry for the bug and thanks so much for the helpful repro! Pushed a fix to the abcfix branch. Would you be willing to test your code with this version to confirm the fix works for you?
from bidict.
While this does fix the issue for me, I think the fix is potentially dangerous because we never actually explicitly check if C is a subclass of collections.abc.Mapping so it might rear it's ugly head again?(not sure)
I tried
If NotImplemented==Mapping.__subclasshook__(C):
instead of what's in the original code
if not Mapping.__subclasshook__(C):
and that also seems to work for me but ofc may be even that isn't real fix since I don't have the whole bidict project running with unit tests etc.
from bidict.
Thanks for confirming my fix!
The alternative changes you're proposing[1] are insufficient. Consider the following:
>>> from collections.abc import Mapping
>>> class MyMapping(Mapping):
... pass
...
>>> Mapping.__subclasshook__(MyMapping)
NotImplemented
>>> issubclass(MyMapping, Mapping)
True
So we can't bail early if the Mapping.__subclasshook__(C)
check returns NotImplemented
. (And we can't actually call issubclass(C, Mapping)
there because it triggers RecursionError
for several cases.)
In particular, this would break the following case:
>>> class VirtualBidirectionalMapping(collections.abc.Mapping):
... def inverse(self):
... return ...
...
>>> issubclass(VirtualBidirectionalMapping, BidirectionalMapping)
True
In other words, if we want to preserve the existing behavior, where any Mapping
with an inverse
attribute is considered a BidirectionalMapping
, we have to take the approach in my fix: check for all the required attributes of the Mapping
interface, plus an inverse
attribute.
This is the same approach taken by Iterator.__subclasshoook__()
. See:
https://github.com/python/cpython/blob/v3.8.5/Lib/_collections_abc.py#L243-L275
Note that Iterator
inherits from Iterable
. But rather than calling Iterable.__subclasshook__()
from Iterator.__subclasshook__()
, Iterator.__subclasshook__()
checks for the required attributes of Iterable
(namely, __iter__
), plus the additional required attributes of Iterator
(namely, __next__
).
I have lots of test cases exercising this fixed BidirectionalMapping.__subclasshook__()
implementation, and added more to cover this bug. Can you please take a look at the following test cases and let me know if you can think of any I'm missing: https://github.com/jab/bidict/blob/abcfix/tests/test_class_relationships.py
(Note all those tests pass with the fix in my branch.)
Thanks!
[1] I think a complete version of what you proposed would actually be something like this:
diff --git a/bidict/_abc.py b/bidict/_abc.py
index 8f1b690..ce2dd98 100644
--- a/bidict/_abc.py
+++ b/bidict/_abc.py
@@ -105,8 +105,9 @@ class BidirectionalMapping(Mapping): # pylint: disable=abstract-method,no-init
"""
if cls is not BidirectionalMapping: # lgtm [py/comparison-using-is]
return NotImplemented
- if not Mapping.__subclasshook__(C):
- return NotImplemented
+ super_subclasshook = super().__subclasshook__(C)
+ if super_subclasshook in (NotImplemented, False):
+ return super_subclasshook
mro = C.__mro__
if not any(B.__dict__.get('inverse') for B in mro):
return NotImplemented
return True
from bidict.
Makes sense. Thanks for fixing this fast.
from bidict.
My pleasure, and thank you again for the helpful bug report.
Another option I’m considering here is to just remove the subclasshook. From polling users in the bidict Gitter chat, as well as a code search on GitHub, so far it appears to be unused, and so is just pure maintenance burden (especially since it requires such fiddly, subtle code to implement properly, and causes crazy bugs like this one if you don’t get it quite right).
I’m still thinking about it, but in the meantime, any objection from your side to dropping support for virtual BidirectionalMapping subclasses?
from bidict.
I just released v0.20.0 to PyPI with a fix for this issue (removing BidirectionalMapping.__subclasshook__
altogether). Please update and let me know if you have any issues.
from bidict.
Related Issues (20)
- global name 'BidirectionalMapping' is not defined HOT 21
- github git tags out of sync since 0.18.4 HOT 3
- Missing __all__ in bidict/__init__.py leads to implicit reexport error with mypy in strict mode. HOT 9
- logo HOT 5
- Dependency Dashboard
- Type hints for Bidict type HOT 2
- Try slipcover HOT 1
- Maybe use nix for devcontainer and GHA “tests” workflow
- Why it returns None when I use bidict?
- Will you add a persistence method? HOT 1
- Improve OpenSSF Scorecard HOT 6
- Swap syntax does not work HOT 4
- Automate upgrading dev dependencies HOT 1
- Type alias definition for `MISSING` results in type violations HOT 1
- test fails with python3.12 HOT 3
- Remove dead batteries
- 0.22.1: temp directory in source tree trashes build HOT 6
- 0.22.1: sphinx warnings `reference target not found` HOT 6
- mypy type inference change in 0.23.0 HOT 12
- Recommended performant alt for `namedbidict` HOT 2
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from bidict.