PyPy compatibility #327

Closed
antocuni opened this Issue Mar 31, 2011 · 8 comments

Projects

None yet

3 participants

@antocuni

The current version of IPython crashes on PyPy because of an infinite recursion inside Config.getitem. The different behavior of PyPy and CPython in these corner cases is documented here:
http://pypy.readthedocs.org/en/latest/cpython_differences.html#subclasses-of-built-in-types

The following patch fixes the bug and make IPython working on PyPy:

diff --git a/IPython/config/loader.py b/IPython/config/loader.py
index 652d321..d000b3a 100644
--- a/IPython/config/loader.py
+++ b/IPython/config/loader.py
@@ -118,6 +118,11 @@ class Config(dict):
         return type(self)(copy.deepcopy(self.items()))

     def __getitem__(self, key):
+        # we cannot use directly self._is_section_key, because it triggers
+        # infinite recursion on top of PyPy. Instead, we manually fish the
+        # bound method.
+        _is_section_key = self.__class__._is_section_key.__get__(self)
+        #
         # Because we use this for an exec namespace, we need to delegate
         # the lookup of names in __builtin__ to itself.  This means
         # that you can't have section or attribute names that are 
@@ -126,7 +131,7 @@ class Config(dict):
             return getattr(__builtin__, key)
         except AttributeError:
             pass
-        if self._is_section_key(key):
+        if _is_section_key(key):
             try:
                 return dict.__getitem__(self, key)
             except KeyError:
@takluyver
Member

Out of interest, is this using a released version of PyPy, or a development version? PyPy 1.4 says it supports Python 2.5, and I believe we're using some 2.6 features. Is PyPy 1.4 kind of 2.5+, or was this tested on the development version?

Otherwise, this looks broadly sensible. Is that the best way to get the attribute? I would use object.__getattribute__(self, "is_section_key"). Thanks!

@antocuni

Yes, pypy 1.4 implements python 2.5.
I tested the patch with the latest version from trunk, which implements python 2.7: we should release pypy 1.5 with full 2.7 compatibility very soon.

Using object.getattribute does not work, because it has the very same behavior of using self._is_section_key directly. The "problem" is that in cpython, the builtin _getattribute does not respect the overridden methods of dict, and thus calls the original getitem, which works fine. However, pypy's getattribute does respect the overridden getitem, hence the problem.

@takluyver
Member

Looking at the module, I'm not actually sure that I see the problem: we're calling back into __getitem__ from __getattr__, but the spec says that __getattr__ should only be called if the attribute isn't found via normal means (and it can be). So unless I'm missing something, self._is_section_key should work on any implementation.

I look forward to giving PyPy 1.5 a spin!

@antocuni

So, this is what happens when you do self._is_section_key inside the getitem:

  • self.class.getattribute(self, '_is_section_key') (i.e.,
    object.getattribute, the normal one)

  • getattribute does self.dict['_is_section_key']; remember that
    self.dict is of type Config

    a) CPython ignores Config.getitem and directly calls dict.getitem,
    which raises KeyError and then the lookup continues on the class

    b) PyPy correctly calls Config.getitem, which tries to do
    "self._is_section_key", which calls Config.getitem, etc.

I hope it's clearer now.

@takluyver
Member

Ah, that makes sense. I'd overlooked that we were replacing __dict__ as well. So is doing self.__class__ special cased so that it doesn't trigger a lookup in __dict__?

@antocuni
antocuni commented Apr 1, 2011

Yes, exactly.

In both PyPy and CPython, class is implemented as a "GetSetProperty",
which takes precedence over the normal dict lookup and just returns
type(self).

See for PyPy:
http://bitbucket.org/pypy/pypy/src/a623e9cc3648/pypy/objspace/std/objecttype.py#cl-215

and for CPython:
http://hg.python.org/cpython/file/214d0608fb84/Objects/typeobject.c#l3059

@takluyver
Member

OK then, this looks sensible.

@takluyver
Member

I've turned your patch into a pull request, #331. Thanks again.

@minrk minrk closed this Apr 4, 2011
@markvoorhies markvoorhies pushed a commit to markvoorhies/ipython that referenced this issue Apr 21, 2011
Antonio Cuni Tweak config loader for PyPy compatibility.
closes gh-327
7eb58d3
@mattvonrocketstein mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this issue Nov 3, 2014
Antonio Cuni Tweak config loader for PyPy compatibility.
closes gh-327
0615bde
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment