Opened 5 years ago

Closed 4 years ago

#4065 enhancement closed invalid (invalid)

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

Reported by: cvaliente Owned by:
Priority: normal Milestone: Python-3.x
Component: core Keywords:
Cc: antoine, exarkun, thijs Branch:
Author: Launchpad Bug:

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 cvaliente 5 years ago.
callable-in-twisted-compat.diff (7.3 KB) - added by cvaliente 5 years ago.

Download all attachments as: .zip

Change History (24)

comment:1 Changed 5 years ago by exarkun

  • Keywords review added

comment:2 follow-up: Changed 5 years ago by antoine

  • Cc antoine 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 5 years ago by cvaliente

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 5 years ago by antoine

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 follow-up: Changed 5 years ago by 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

comment:6 in reply to: ↑ 5 Changed 5 years ago by cvaliente

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 follow-up: Changed 5 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 5 years ago by cvaliente

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 follow-up: Changed 5 years ago by antoine

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 5 years ago by cvaliente

comment:10 in reply to: ↑ 9 Changed 5 years ago by cvaliente

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 5 years ago by cvaliente

  • Owner glyph deleted

comment:12 follow-up: Changed 5 years ago by exarkun

  • Cc exarkun added
  • Keywords review removed
  • Owner set to cvaliente

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 5 years ago by cvaliente

comment:13 in reply to: ↑ 12 Changed 5 years ago by cvaliente

  • Keywords review added
  • Owner cvaliente 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 5 years ago by exarkun

  • 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 5 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 5 years ago by exarkun

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 5 years ago by thijs

  • Milestone set to Python-2.6

comment:18 Changed 5 years ago by exarkun

  • Keywords py3k removed
  • Milestone changed from Python-2.6 to Python-3.x

comment:19 follow-up: Changed 5 years ago by exarkun

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

comment:20 in reply to: ↑ 19 Changed 5 years ago by thijs

  • Cc thijs 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 4 years ago by exarkun

  • Resolution set to invalid
  • Status changed from new to closed

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

comment:22 Changed 4 years ago by <automation>

Note: See TracTickets for help on using tickets.