[Python-Dev] Patch to use dict subclasses in eval(), exec

[Guido van Rossum]

> > I don't see the extra DECREF here (and below) needed to compensate for
> > the fact that PyObject_GetItem() returns the object with incremented
> > refcount but PyDict_GetItem() doesn't.
> 
> oops.  I believe this can be fixed, if only by a DECREF in all paths
> and an INCREF in the fast path.  However, this will worsen the "fast
> case" performance figures at least a tiny bit.

I'm already unhappy about the performance hit of your patch.  We're
trying very hard to make global and built-in access as fast as
possible.  This patch adds extra local variable references to the
"fast path".  More INCREFS and DECREFS will slow it down even more.
Not just because there's more executed code, but also because there's
more code, period, which may reduce the I-cache effectiveness.

> [guido again]
> > What are you going to do with all the *other* places where
> > PyDict_GetItem() is used?
> 
> Which places do you have in mind?  The common feature request seems
> to be for globals() only, and I can't immediately think of a way of
> accessing globals() that my patch doesn't (attempt to) cover.

There are lots of other places where PyDict_*Item is used for an
object that could be a dict subclass.  Unless you've tracked all these
down I think it wouldn't be wise to apply this patch.  We either say
(as we say now) "you can't override __getitem__ in a dict passed into
the Python interpreter and expect it to work" or we say "you can
override __getitem__ and it will work".

It would be really bad to say "you can override __getitem__ and it
will usually work but not in these twelve situations, because we
didn't bother to change legacy code."

> Unfortunately, I'm not particularly excited about this capability
> myself.

Me neither, to say the least.  I still feel hostile, both because of
the performance hit, and because I worry that it will be used to
create obfuscated code that *looks* like Python but isn't Python.

> What I think this does show, however, is that using a sequence like
> (not quite C code)
>     inline PyObject *PyObject_GetItem_FastForDicts(o, k) {
>         if(PyDict_CheckExact(o)) {
>             ret = PyDict_GetItem(o, k);
>             Py_INCREF(ret);
>             return ret;
>         }
>         return PyObject_GetItem(o, k);
>     }
> will impose in-the-noise penalties for 'dict' instances, modest
> penalties for 'dict' subclass instances without __getitem__, and
> provide the full power for general objects.

I'd like to see more benchmarking of this (as well as more motivation).

> On Tue, Oct 29, 2002 at 08:57:01AM +0100, M.-A. Lemburg wrote:
> > If all you want is a way to define a default value for a failing
> > dictionary lookup, wouldn't it be better to simply add this feature
> > to the standard dictionary implementation ?
> 
> It doesn't seem to be quite this simple, because you almost certainly
> want to have items in the builtin namespace appear even if you want to
> give a default on what would eventually be a NameError for a
> LOAD_GLOBAL/LOAD_NAME.  You end up with something like
>     class D(dict):
>         def __getitem__(self, item):
>             try:
>                 return dict.__getitem__(self, item)
>             except KeyError:
>                 try:
>                     return getattr(__builtins__, item)
>                 except AttributeError:
>                     return "XX"
> which is more complicated that a "simple" defaulting dictionary.
> 
> Unfortunately, this is another time when I am not a strong proponent of a 
> feature, but merely wrote the patch to see if the "common wisdom" on which
> the rejection was based is valid.  (common wisdom's been right in earlier
> cases, but I'm tempted to call the speed penalties for this change
> entirely tolerable if the purposes of people who do want this patch are
> good ones)
> 
> 
> On Tue, Oct 29, 2002 at 12:11:12AM -0500, Raymond Hettinger wrote
> [in a private message]:
> > Do you know why the following snippet doesn't work with your patch?
> >
> > class SpreadSheet(dict):
> >     def __getitem__(self, key):
> >         return eval(dict.__getitem__(self,key), self)
> >
> > ss = SpreadSheet()
> > ss['a1'] = '5'
> > ss['a2'] = 'a1*6'
> > ss['a3'] = 'a2*a1'
> > print ss['a3']
> 
> It took me awhile to figure this out...
> 
> LOAD_NAME first tries to load from the locals dict, where I did not
> change the way that name resolution functions.

Another deficiency of the patch.

> eval() uses
> the same dict for locals and globals if the locals (third arg) are not
> directly specified.
> 
> The following works:
>     class SpreadSheet(dict):
>         def __getitem__(self, key):
>             return eval(dict.__getitem__(self,key), self, {})
> 
> Jeff

--Guido van Rossum (home page: http://www.python.org/~guido/)