Repository navigation
copy.copy() does not properly copy os.environment #59578
Description
| BPO | 15373 |
|---|---|
| Nosy | @rhettinger, @gpshead, @pitrou, @bitdancer, @methane, @serhiy-storchaka, @eryksun, @MojoVampire, @sblondon, @akulakov, @kamilturek, @nightlark |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = None
closed_at = None
created_at = <Date 2012-07-16.16:08:57.208>
labels = ['type-feature', 'library', '3.11']
title = 'copy.copy() does not properly copy os.environment'
updated_at = <Date 2022-03-02.00:42:01.639>
user = 'https://bugs.python.org/htgoebel'bugs.python.org fields:
activity = <Date 2022-03-02.00:42:01.639>
actor = 'eryksun'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2012-07-16.16:08:57.208>
creator = 'htgoebel'
dependencies = []
files = ['26403', '26404']
hgrepos = []
issue_num = 15373
keywords = ['patch', 'needs review']
message_count = 18.0
messages = ['165631', '165637', '165647', '165652', '165691', '165712', '286384', '286392', '286473', '286513', '286526', '384993', '396922', '399654', '402364', '402366', '414318', '414319']
nosy_count = 14.0
nosy_names = ['rhettinger', 'gregory.p.smith', 'htgoebel', 'pitrou', 'r.david.murray', 'methane', 'serhiy.storchaka', 'eryksun', 'josh.r', 'sblondon', 'andrei.avk', 'kamilturek', 'rmast', 'mkatsev']
pr_nums = []
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue15373'
versions = ['Python 3.11']Reacted by Finite State Machine
Activity
htgoebel commentedon Jul 16, 2012
Wehn copying os.environ usinf copy.copy(), any manipulation on the copied object will change os.environment, too.
Strictly speaking, this is correct, as the os.environment class is meant to manipulate the environment in the background. But user's expectation is different: he thinks, manipulating the copied object is save and does not effect the environment.
bitdancer commentedon Jul 16, 2012
os.environ is not a dictionary, so it isn't all that surprising that a shallow copy doesn't behave like a shallow copy of a dictionary. deepcopy does what you'd expect, as does os.environ.copy().
Perhaps it is worth improving this by adding a __copy__ method to os.environ. Someone would need to propose a patch, and it would be an enhancement.
swarmer commentedon Jul 16, 2012
Here's a patch.
swarmer commentedon Jul 16, 2012
A new patch with tests.
pitrou commentedon Jul 17, 2012
I agree with David that copy(os.environ) is rather ambiguous. I think the preferred idiom should be to call dict(os.environ).
As for __copy__, I don't know what it should do: perhaps simply raise an error when copying is attempted?
bitdancer commentedon Jul 17, 2012
Well, I think the fact that os.environ.copy is explicitly supported (and does indeed do dict(os.environ)) is an argument in favor of giving that same meaning to copy(os.environ). I think that follows the principle of least surprise.
serhiy-storchaka commentedon Jan 27, 2017
If add support for the copy protocol, consider also adding support for deepcopying and pickling.
rhettinger commentedon Jan 28, 2017
[Antoine]
FWIW, I concur with Antoine on every point. Also, raising an error when copying seems reasonable ("refuse the temptation to guess" at what the user intended).
methane commentedon Jan 30, 2017
patch looks OK.
But I prefer
__deepcopy__ = __copy__ = copy.I don't know how to support pickling. (should unpickled object
reference os.environ, or copied dict?)
I feel it is different issue.
rhettinger commentedon Jan 31, 2017
That patch would be fine except that I really agree with Antoine that an error should be raised.
methane commentedon Jan 31, 2017
I agree.
os.environ is not dict, it's a proxy of "current" environment which
is not copyable.
So I agree to copy.copy() shouldn't copy os.environ implicitly.
MojoVampire commentedon Jan 13, 2021
Would we remove the functionality of os.environ.copy()? It seems very odd for types to have a .copy() method that works, while not supporting copy.copy, especially when there is zero documentation, on the web or the docstring, to even hint at the difference.
I'm strongly in favor of silently doing the right thing and behaving the same way the .copy() method already behaves; if you want a "copy" of os.environ that still modifies the environment, that's just aliasing it (envalias = os.environ), not copying at all. If you're trying to make a shallow copy, not an alias, you're trying to separate it from the parent, which every other dict-like thing does (assuming immutable values), where os.environ is a very weird exception (for copy.copy, but not the .copy() method).
Can someone give an example where you'd want copy.copy to produce a "shallow copy" that acts like an alias, not an actual independent copy?
akulakov commentedon Jul 3, 2021
I think it may be good to deprecate and discourage use of
os.environ.copy(), and add a new methodos.environ.asdict(). And possibly have__copy__raise an error pointing users toos.environ.asdict().I'm not sure what to do about pickling.
sblondon commentedon Aug 16, 2021
I think an asdict() method is not necessary. Instanciating a dict from os.environ is enough:
With 'dict()', it's obvious there is no side effect.
I think os._Environ should not be pickable too because there is an ambiguity too when it will be unpicked: should we set the unpickled data to a basic dict (or another _Environ instance) or to the environment variables?
It seems there are more maintainers in favour of the removal. Removing the copy is a breaking change so it should be done on several releases.
Are you interested by a PR which adds DeprecationWarning when copy.copy(os.environ) or os.environ.copy() are called? It seems to be the first step to implement.
akulakov commentedon Sep 21, 2021
I think the advantage of asdict() method is it's more discoverable and it doesn't leave any uncertainty of whether returned value will update environment or not. If a user sees
dict(os.environ)in code, they may wonder if it does or does not; and there's no obvious way to confirm.If they see os.environ.asdict(), it will be safe to assume that the method is provided exactly for this purpose, and the way to confirm is obvious - checking the docstring.
akulakov commentedon Sep 21, 2021
I guess the easy way to test it would be to modify the copy and check os.environ, but still there would be some uncertainty if it's consistent across platforms and python versions, if it's expected to still work in future python versions, etc. asdict() docstring is much more explicit.
mkatsev commentedon Mar 1, 2022
Note that deepcopy doesn't work either, even though it looks like it does at the first glance (which is arguably worse since it's harder to notice):
eryksun commentedon Mar 2, 2022
In bpo-28824, I suggested preserving the case of environment variables in Windows by using a case-insensitive subclass of str in the encodekey() function. This is self-contained by the use of the encodekey() and decodekey() functions in the mapping methods such as __iter__(). The reason for this is mostly about aesthetics, but also about faithfully displaying the actual environment variable names. Opinions vary, but to me "WindowsSdkVerBinPath" is both easier on my eyes and easier to read than "WINDOWSSDKVERBINPATH". The eyesore factor gets amplified when it's a wall of all upper-cased names 'screaming' at me.
A copy via dict(os.environ) would use regular str keys and lose the case-insensitive, case-preserving property. It would be more useful if os.environ.copy() were implemented to return a copy that keeps the case-insensitive property for keys but disables updating the process environment. This could be implemented by making putenv and unsetenv parameters in the constructor. If self.putenv or self.unsetenv is None, then __setitem__() or __delitem__() woud have no effect on the process environment. The copy() method would return a new _Environ instance for self._data.copy(), one which of course disables updating the process environment.