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

ObjectProxy does not play well with GenericAlias, such as isinstance(proxy, Dict) #245

Open
champialex opened this issue Aug 28, 2023 · 12 comments

Comments

@champialex
Copy link

Thanks for the great library!

Just ran into an issue with instance checks on typing.py aliases.

def test_isinstance_proxy():
    p = wrapt.ObjectProxy({1: 2})

    assert isinstance(p, dict) # Passes just fine
    assert isinstance(p, Dict) # Fails

This is due to GenericAlias / _BaseGenericAlias being implemented as:

def __instancecheck__(self, obj):
    return self.__subclasscheck__(type(obj)) # Should be obj.__class__

I imagine this is a python bug? Is this something you have run into before?

@GrahamDumpleton
Copy link
Owner

What is the Dict type and how does it relate to GenericAlias and _BaseGenericAlias?

I am not familiar with typing.py aliases. Can you provide URL links to docs and the code you are quoting?

Even so, the code self.__subclasscheck__(type(obj)) could well be problematic when one considers duck typing and the ability to override __class__ attribute of Python objects.

@pbryan
Copy link

pbryan commented Aug 28, 2023

In general, you can't check if values are instances of generic aliases. It should raise TypeError, in part (by design) because of type erasure. If you want to perform an equivalent check, you can check if it is an instance of the class provided by typing.get_origin(alias).

@champialex
Copy link
Author

Hey Graham,

So, for Dict, it is coming from typing import Dict. The code I'm referring to is here in typing.py, L1164

I also just found this ticket in CPython: python/cpython#89949

@GrahamDumpleton
Copy link
Owner

To better understand implication of Paul's comment about typing.get_origin(alias) can you provide a better overall code example of when the original problem arises rather than a very narrow test case. But then this could be a stupid ask on my part since could well be obvious if I understood typing.py aliases.

@pbryan
Copy link

pbryan commented Aug 28, 2023

Some code to help illustrate:

pbryan@sesame ~]$ python
Python 3.11.3 (main, Jun  5 2023, 09:32:32) [GCC 13.1.1 20230429] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import typing
>>> alias = dict[str, int]
>>> value = alias(a=1, b=2)
>>> value
{'a': 1, 'b': 2}
>>> isinstance(value, alias)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: isinstance() argument 2 cannot be a parameterized generic
>>> isinstance(value, typing.get_origin(alias))
True
>>> 

@GrahamDumpleton
Copy link
Owner

Yeah, was hoping for better example from Alexandre to see whether that sort of change you were suggesting made sense.

FWIW, I didn't even know you could write something like alias = dict[str, int]. Shows how rusty I am on Python coding.

@pbryan
Copy link

pbryan commented Aug 28, 2023

Generic aliases from base types is relatively new. I believe circa 3.10. Prior to that, we had to use generics defined in typing like typing.Dict[str, int]. Those are now deprecated in favour of the base type generics.

@champialex
Copy link
Author

champialex commented Aug 28, 2023

I think @pbryan argument is fair enough. I think as long as this happens in code I own / have control over this is pretty easy to fix / handle by moving to checks against the proper classes, rather than the aliases. Less so if it happens in libraries.

I am checking a few corner cases before committing on using the proxy more widely.

@GrahamDumpleton
Copy link
Owner

As far as sniff test on the code:

def __instancecheck__(self, obj):
    return self.__subclasscheck__(type(obj))

I still feel it isn't favourable to duck typing and perhaps could be rewritten as:

def __instancecheck__(self, obj):
    return self.__subclasscheck__(getattr(obj, "__class__", type(obj)))

Obviously this would help with the weird goals of wrapt, but whether it is a sensible change I don't know.

With the demise of Twitter I don't know of an easy way to reach out to any Python folks who may have an opinion. :-(

@GrahamDumpleton
Copy link
Owner

Oh, and could well be the getattr() isn't needed there and could just say obj.__class__. My knowledge is based on really old Python versions where not sure a __class__ attribute was always guaranteed.

@champialex
Copy link
Author

champialex commented Aug 28, 2023

...... Twitter....

Some devs seem to agree there if I understand correctly python/cpython#89949 .

Also not sure how much it actually matters, and it may be a dangerous change

@champialex
Copy link
Author

Thanks for the help.

Looks like I will have to be careful using the proxy.

It seems that CPython's PyTuple_Check checks the type and not the class.

This causes issue with C implementations, even in the standard lib. For example, when trying to Json-serialize a proxy:

def test_can_json_serialize_proxy():
    import json
    
    p = wrapt.ObjectProxy((1, 2, 3))

    serialized = json.loads(json.dumps(p))

    assert serialized == (1, 2, 3)

This will fail here, when CPython relies on the C library

Interestingly enough the python implementation of it is fine and uses isinstance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants