Opened 8 years ago

Closed 7 years ago

#4065 enhancement closed invalid (invalid)

Replace 'callable(foo)' with 'hasattr(foo, "__call__")'

Reported by: Carlos Valiente Owned by:
Priority: normal Milestone: Python-3.x
Component: core Keywords:
Cc: Antoine Pitrou, Jean-Paul Calderone, Thijs Triemstra Branch:
Author:

Description

This patch eliminates the DeprecationWarnings thrown when running the test suite with "python2.6 -3"

Attachments (2)

callable-not-supported.diff (9.1 KB) - added by Carlos Valiente 8 years ago.
callable-in-twisted-compat.diff (7.3 KB) - added by Carlos Valiente 8 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 8 years ago by Jean-Paul Calderone

Keywords: review added

comment:2 Changed 8 years ago by Antoine Pitrou

Cc: Antoine Pitrou added

Please note there are corner cases where the two are not equivalent, but hopefully they don't really matter in the real world:

>>> class P(property):
...   def __get__(self, obj, cls):
...     1/0
... 
>>> class C(object):
...   __call__ = P()
... 
>>> c = C()
>>> callable(c)
True
>>> hasattr(c, "__call__")
False
>>> c.__call__
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 3, in __get__
ZeroDivisionError: integer division or modulo by zero

The reason is that hasattr() really calls getattr() and then discards the result / catches any exception, while callable() only examines the type's structure (callable() is lower level).

comment:3 in reply to:  2 Changed 8 years ago by Carlos Valiente

Replying to antoine:

The reason is that hasattr() really calls getattr() and then discards the result / catches any exception, while callable() only examines the type's structure (callable() is lower level).

I see. What about defining callable in twisted.python.compat3 like this:

$ python3
Python 3.1.1 (r311:74480, Oct 29 2009, 08:31:28) 
[GCC 4.2.1] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> def callable(obj):
...    try:
...        return getattr(obj, "__call__", None) is not None
...    except:
...        return True
...
>>> class P(property):
...     def __get__(self, obj, cls):
...         1/0
... 
>>> class C(object):
...     __call__ = P()
... 
>>> c = C()
>>> callable(c)
True

comment:4 Changed 8 years ago by Antoine Pitrou

It's still not ideal:

>>> class C: pass
... 
>>> c = C()
>>> c.__call__ = lambda: print("__call__!")
>>> getattr(c, "__call__", None)
<function <lambda> at 0x7fb4e58b5b78>
>>> c()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'C' object is not callable

comment:5 Changed 8 years ago by Antoine Pitrou

Ok, so Benjamin Peterson answered me the following on IRC, about the ideal replacement:

isinstance(x, collections.Callable), but I think hasattr(x, "__call__") is fine

comment:6 in reply to:  5 Changed 8 years ago by Carlos Valiente

Replying to antoine:

Ok, so Benjamin Peterson answered me the following on IRC, about the ideal replacement:

isinstance(x, collections.Callable), but I think hasattr(x, "__call__") is fine

Since that option seems to be better, shall I implement callable() in twisted.python.compat3, together with some test cases like the samples you provided here, Antoine?

comment:7 Changed 8 years ago by Glyph

Shouldn't this just be in twisted.python.compat? Why a new compat3 module?

comment:8 in reply to:  7 Changed 8 years ago by Carlos Valiente

Replying to glyph:

Shouldn't this just be in twisted.python.compat? Why a new compat3 module?

According to the description of #2484, compatibility code for Python 3.x should go in twisted.python.compat3. Does that still hold, or should I just keep it simple and do this in twisted.python.compat:

try:
    callable = callable
except NameError:
    def callable(obj):
        # ...

comment:9 Changed 8 years ago by Antoine Pitrou

Please note that, looking at the kind of purposes callable() is used for here, hasattr(XXX, "__call__") is probably good enough a replacement (and faster than the correct approach). But the final decision is not my call :-)

By the way, there's a typo in your patch in test_assertions: "__call_" instead of "__call__".

Changed 8 years ago by Carlos Valiente

Attachment: callable-not-supported.diff added

comment:10 in reply to:  9 Changed 8 years ago by Carlos Valiente

By the way, there's a typo in your patch in test_assertions: "__call_" instead of "__call__".

Oops, just fixed that . Thanks, Antoine!

comment:11 Changed 8 years ago by Carlos Valiente

Owner: Glyph deleted

comment:12 Changed 8 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added
Keywords: review removed
Owner: set to Carlos Valiente

hasattr is a Python mis-function:

   >>> class X:
   ...     def __getattr__(self, name):
   ...             1 / 0
   ... 
   >>> hasattr(X(), 'foo')
   False
   >>> getattr(X(), 'foo', None)
   Traceback (most recent call last):
     File "<stdin>", line 1, in <module>
     File "<stdin>", line 3, in __getattr__
   ZeroDivisionError: integer division or modulo by zero
   >>> 

I think the twisted.python.compat idea makes the most sense.

Changed 8 years ago by Carlos Valiente

comment:13 in reply to:  12 Changed 8 years ago by Carlos Valiente

Keywords: review added
Owner: Carlos Valiente deleted

Replying to exarkun:

I think the twisted.python.compat idea makes the most sense.

Patch callable-in-twisted-compat.diff implements callable in twisted.python.compat, as suggested.

comment:14 Changed 8 years ago by Jean-Paul Calderone

Keywords: review removed

Thanks for the updated patch cvaliente.

The terrible implementation of callable in this patch motivated me to complain on #python-dev. Someone there pointed out http://bugs.python.org/issue7006 which makes it anything but clear what callable is actually supposed to be replaced with. I think that waiting for that issue to be resolved before changing anything in Twisted makes sense.

comment:15 Changed 8 years ago by jknight

Many (most? I didn't count.) uses of callable in twisted seem to be in "assert" expressions. These seem superfluous and should probably just be removed. Even if something is callable, it might still fail to be called, and that error must be handled properly, in any case.

comment:16 Changed 8 years ago by Jean-Paul Calderone

True. Some of the places do it so that the error happens early, near the code which is bogusly passing a non-callable in, though. I think these make it easier for people doing something dumb to recognize their mistake, maybe.

comment:17 Changed 7 years ago by Thijs Triemstra

Milestone: Python-2.6

comment:18 Changed 7 years ago by Jean-Paul Calderone

Keywords: py3k removed
Milestone: Python-2.6Python-3.x

comment:19 Changed 7 years ago by Jean-Paul Calderone

Additional news, http://bugs.python.org/issue7624 (isinstance(foo, collections.Callable) is presently broken for classic classes).

comment:20 in reply to:  19 Changed 7 years ago by Thijs Triemstra

Cc: Thijs Triemstra added

Replying to exarkun:

Additional news, http://bugs.python.org/issue7624 (isinstance(foo, collections.Callable) is presently broken for classic classes).

Which is fixed now in Python trunk.

comment:21 Changed 7 years ago by Jean-Paul Calderone

Resolution: invalid
Status: newclosed

Python 3.2 is re-introducing the callable builtin: http://bugs.python.org/issue10518

comment:22 Changed 6 years ago by <automation>

Note: See TracTickets for help on using tickets.