classification
Title: os.environ should inherit dict
Type: enhancement Stage:
Components: Versions: Python 2.6
process
Status: closed Resolution: rejected
Dependencies: Superseder:
Assigned To: Nosy List: belopolsky, benjamin.peterson, georg.brandl, loewis, rhettinger, vdupras
Priority: normal Keywords: patch

Created on 2008-02-19 21:56 by benjamin.peterson, last changed 2008-02-24 04:24 by belopolsky. This issue is now closed.

Files
File name Uploaded Description Edit
environ-modern.diff benjamin.peterson, 2008-02-19 21:56
environ-modern2.diff benjamin.peterson, 2008-02-23 22:43
environ-modern3.diff benjamin.peterson, 2008-02-23 22:49
Messages (22)
msg62571 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-02-19 21:56
This patch changes os.environ to inherit builtin dict rather than UserDict.
msg62572 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-02-19 22:11
What's the rationale for this change?
msg62573 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-02-19 22:37
PEP 390. It's cleaner and faster.
msg62582 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-02-20 03:51
I still must be missing something. There is no PEP 390, AFAICT.
msg62584 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-02-20 04:04
Forgive me. I meant 290.
msg62585 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-02-20 04:12
Sorry, it still makes no sense. Up to r56113, PEP 290 doesn't seem to
talk about UserDict, os.environ, or inheritance from new-style classes.
What section are you specifically referring to?
msg62593 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-02-20 12:55
I know that it doesn't mention inheriting dict specifically, but you
asked why, so I was referring to the Rationale: "Modernization options
arise when new versions of Python add features that allow improved
clarity or higher performance than previously available." Sorry for the
confusion.
msg62594 - (view) Author: Virgil Dupras (vdupras) (Python triager) Date: 2008-02-20 16:36
The performance gain is rather good:

hsoft-dev:python hsoft$ ./python.exe -m "timeit" -s "import os" 
"os.environ['HOME']"
1000000 loops, best of 3: 1.16 usec per loop
hsoft-dev:python hsoft$ patch -p0 < environ-modern.diff 
patching file Lib/os.py
hsoft-dev:python hsoft$ ./python.exe -m "timeit" -s "import os" 
"os.environ['HOME']"
1000000 loops, best of 3: 0.428 usec per loop

The regression tests still pass, and modernization of the code is a nice 
thing.

+1
msg62824 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-02-23 22:33
Small comment on the patch:

                 def clear(self):
-                    for key in self.data.keys():
+                    for key in self.keys():
                         unsetenv(key)
-                        del self.data[key]
+                        del self[key]

It looks like the patched version will unsetenv twice, Change

del self[key]

to

dict.__delitem__(self, key)


+1
msg62827 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-02-23 22:43
Here's a corrected version.
msg62828 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2008-02-23 22:46
See #1367711 for a discussion whether this is really the way to go.
msg62829 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-02-23 22:49
And another.
msg62852 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-02-24 00:19
> dict doesn't dynamically bind any calls; to
> properly replace the semantics of dict, you
> have to implement *all* API.

What was the rationale for this decision?  To me it looks like a hold-
over from the time when dicts were not subclassable.

I agree, this is a topic for python-ideas rather than bug-track, but if 
you could point me to any prior discussions on this issue, it would help 
me to either formulate a specific proposal or drop the issue before more 
electrons are sacrificed.
msg62854 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2008-02-24 00:27
> What was the rationale for this decision?  To me it looks like a hold-
> over from the time when dicts were not subclassable.

I reckon it's faster this way, and you want basic datatypes like dict to
be fast.
msg62857 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-02-24 00:38
> What was the rationale for this decision?  To me it looks like a hold-
> over from the time when dicts were not subclassable.
> 
> I agree, this is a topic for python-ideas rather than bug-track, but if 
> you could point me to any prior discussions on this issue, it would help 
> me to either formulate a specific proposal or drop the issue before more 
> electrons are sacrificed.

Can't find the original discussion right now, but one statement is at

http://www.mailinglistarchive.com/python-dev@python.org/msg01728.html

In essence, inheriting from dict is intentionally unspecified: if you
only change some methods, but not all, the behavior of the
non-overridden methods is unspecified. This is necessary because
a) specifying it would be a lot of work, and
b) specifying it might unreasonable restrict future implementations, and
c) specifying late binding for some of the methods would unreasonably
    slow down their implementation, even if the common case that dict is
    not subclassed.
msg62858 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2008-02-24 00:51
The builtins make direct calls to their own internal methods.  This has 
existed for a long time. It would be hard to change without crushing 
performance. Changing it violates Meyer's open-closed principle. And 
changing it also introduces weird, unexpected behaviors (i.e. a 
seemingly innocent method override can break pickling for an object -- 
we had a bug report on this once for dicts and sets).  Also, making 
these kind of changes makes it *much* harder for a builtin to guarantee 
that it invariants are maintained and opening the door for segfaults.
msg62859 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-02-24 01:13
First, if the new thread on dict behavior does not make sense, please 
see discussion at issue1367711 where it started.  (My mistake following 
up here.)

Second, the ability to subclass built-in type is such a great feature, 
that it is a shame that doing so comes with so many caveats.  The way I 
understand objections to issue1367711 patch is that (1) it is hard to 
derive from dict properly and (2) derived classes may be broken by 
future changes to dict.  The second objection applies to UserDict 
derivation as much as dict derivation, only UserDict is unlikely to be 
touched ever in the future.  As to the first objection, if the burden is 
that all methods have to be overriden, let os.environ lead by example 
and do it.  UserDict is a kludge from the XX century, stdlib should stop 
promoting its use.

Since there is so much resistance to making dict more subclass-friendly, 
what would you say to reimplementing DictMixin in C as say 'dict.mixin'?
msg62860 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-02-24 01:20
> The builtins make direct calls to their own internal methods.

Raymond, I guess issue2067 escaped your review. :-)
msg62861 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2008-02-24 01:35
Py3.0 updte:  UserDict is going to survive into Py3.0 and will be moved 
into the collections module.  DictMixin is replaced by the abstract 
base classes for mappings.

I think the discussion here has grown far beyond the original bug 
report.  I recommend this one be closed and that you start a thread on 
python-3000 or python-dev if you want to propose altering basic APIs or 
moving the ABC code into C.

I can't say that I support your "UserDict is a kludge ..." comment.  
The class has its uses and the standard library will continue to use it 
where appropriate.  

FWIW, at one time I also thought all uses of UserDict would ultimately 
be supplanted by dict subclassing, but I found that it was somewhat 
difficult to remove in some circumstances and that its API had some 
advantages (i.e. it's easier to write subclass code like self.data
[computedkey]=computedvalue than to dispatch to dict.__setitem__(self, 
computedkey, computedvalue) -- the latter form is somewhat errorprone 
when the subclass methods become more complex).

Will look at 1367711 as you suggest.
msg62864 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-02-24 02:00
Let's get back on-topic.

I assume you are recommending to close this issue by rejecting the 
patch.  I disagree.  The patch can be fixed to properly override all 
methods and a unit test can be added to guarantee that all dict methods 
are overridden.

The patch has two distinct benefits: (1) UserDict is not loaded on 
startup; and (2) os.environ is faster.  IMHO, these two reasons make the 
patch worth pursuing.

BTW, one of the objections based on exec .. in os.environ behavior (see http://bugs.python.org/msg49131) has become invalid since then.

Specific issues should of course be addressed.
msg62873 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2008-02-24 04:18
I also disagree that UserDict is a clutch, and that inheriting from dict
is in any way more modern or cleaner than inheriting from UserDict.

Hence I'm rejecting this patch. To "appeal", please discuss on python-dev.
msg62875 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2008-02-24 04:24
Did anyone mention "clutch"? :-)

Oh, well, please close issue1367711 as a duplicate.
History
Date User Action Args
2008-02-24 04:24:17belopolskysetmessages: + msg62875
2008-02-24 04:18:40loewissetstatus: open -> closed
resolution: rejected
messages: + msg62873
2008-02-24 02:00:07belopolskysetmessages: + msg62864
2008-02-24 01:35:01rhettingersetmessages: + msg62861
2008-02-24 01:20:57belopolskysetmessages: + msg62860
2008-02-24 01:13:05belopolskysetmessages: + msg62859
2008-02-24 00:51:25rhettingersetnosy: + rhettinger
messages: + msg62858
2008-02-24 00:38:07loewissetmessages: + msg62857
2008-02-24 00:27:20georg.brandlsetmessages: + msg62854
2008-02-24 00:19:00belopolskysetmessages: + msg62852
2008-02-23 22:49:35benjamin.petersonsetfiles: + environ-modern3.diff
messages: + msg62829
2008-02-23 22:46:41georg.brandlsetnosy: + georg.brandl
messages: + msg62828
2008-02-23 22:43:23benjamin.petersonsetfiles: + environ-modern2.diff
keywords: + patch
messages: + msg62827
2008-02-23 22:33:58belopolskysetnosy: + belopolsky
messages: + msg62824
2008-02-20 16:36:39vduprassetnosy: + vdupras
messages: + msg62594
2008-02-20 12:55:07benjamin.petersonsetmessages: + msg62593
2008-02-20 04:12:34loewissetmessages: + msg62585
2008-02-20 04:04:23benjamin.petersonsetmessages: + msg62584
2008-02-20 03:51:09loewissetmessages: + msg62582
2008-02-19 22:37:22benjamin.petersonsetmessages: + msg62573
2008-02-19 22:11:33loewissetnosy: + loewis
messages: + msg62572
2008-02-19 21:56:51benjamin.petersoncreate