GrahamDumpleton / wrapt

A Python module for decorators, wrappers and monkey patching.
BSD 2-Clause "Simplified" License
2.03k stars 231 forks source link

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

Open champialex opened 1 year ago

champialex commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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: https://github.com/python/cpython/issues/89949

GrahamDumpleton commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

...... Twitter....

Some devs seem to agree there if I understand correctly https://github.com/python/cpython/issues/89949 .

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

champialex commented 1 year ago

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.