gh-152636: Add a note about frozendict to the types.MappingProxyType#152637
gh-152636: Add a note about frozendict to the types.MappingProxyType#152637sobolevn wants to merge 3 commits into
frozendict to the types.MappingProxyType#152637Conversation
…pingProxytype`
Documentation build overview
34 files changed ·
|
|
This warning feels redundant to me. MappingProxyType is ultimately just a proxy; it is sometimes used as a substitute for frozendict, but that is not its main purpose. MappingProxyType is not a well-known class, so people will probably find frozendict more easily. |
|
I agree with Raymond (on issue) and Inada (above), the docs should stay neutral, rather than recommend one over the other. The two types do two different things, claiming either is universally better is false. |
| ``MappingProxyType`` can expose its internal mapping | ||
| in some rare cases on some versions of Python. | ||
| Starting from Python 3.15 it is recommeneded | ||
| to use truly immutable :class:`frozendict` instead. |
There was a problem hiding this comment.
I don't think that a "warning" is justified here, a "note" would be enough.
For the second sentence, maybe only recommend frozendict to "store sensitive information".
|
My concern is that multiple bugs exposing the internal mapping have been fixed so far, but there might be more bugs awaiting. Instead of trying to fix all bugs, I suggested adding a note in MappingProxyType doc explaining that the internal mapping can be leaked, it's not 100% "secure", and using a frozendict avoids these issues. |
frozendict doesn't replace MappingProxyType in all cases, but it's safer if you want to store sensitive information. |
|
An example accessing and modifying the internal mapping: import gc, types
# the internal mapping is not stored in a variable
# to make sure that it cannot be modified
private = types.MappingProxyType({'password': 82349281})
for ref in gc.get_referents(private):
if isinstance(ref, dict):
ref['password'] = 123
print(private)Output: I don't think that we can fix this bug or "vulnerability", the GC must track the |
frozendict is preferable than MappingProxyTypefrozendict to the types.MappingProxyType
|
Updated :) For me, it explains the difference perfectly :) |
|
I still feel that this note is redundant and reduces the S/N ratio of the documentation, but I don't strongly object to it. |
I asked several Python people from my community, how many of them can explain such difference. Not many of them could :) So, I think that this is a not really a noise, but an important difference between two stdlib containers that people should be aware of and understand their desired use-case :) If there are no other objections / specific rephrase ideas, I will merge this PRs in a week or so :) |
| ``MappingProxyType`` can expose its internal mapping in some rare cases. | ||
|
|
||
| While ``frozendict`` is a concrete container type | ||
| that actually holds data that cannot be changed in any way. |
There was a problem hiding this comment.
I can envision people creating issues because the following doesn't hold:
x = {1: [1,2]}
y = frozendict(x)
x[1].append(2)
assert y[1] == [1,2]
I don't know if we make a difference between data in the sense (key, value), namely you can't change the assignment (key) to (value) (in terms of pointers, you don't change that) vs what the value actually is (it's still some "data" inside that frozendict as it's the value of y[1] in my example). But by saying "cannot be changed in any way" is too strong. It may be understood as making a deep-copy of the dict content.
Also, if you start your sentence with While, there should a subject afterwards, e.g, While X, Y. If you want to drop the Y part, I'd say "On the other hand, X".
For the average Python user, I think This note matters only for Python users who would consider |
| without affecting the underlying container. | ||
| ``MappingProxyType`` can expose its internal mutable mapping in some rare cases. | ||
|
|
||
| On the other hand, ``frozendict`` is a concrete immutable mapping. |
There was a problem hiding this comment.
AFAIK both MappingProxyType and frozendict are (immutable) mappings if we consider collections.abc.Mapping. So... I don't know whether there is really a need for that note in the end. "concrete immutable mapping" applies to both of them. I'm sorry for being nitpicky though but I agree with Inada here that this note could be more confusing depending on who reads it.
We already say
Read-only proxy of a mapping. It provides a dynamic view on the mapping's
entries, which means that when the mapping changes, the view reflects these
changes.
I would just say (if I were to change the docs, but that's not even sure)
Read-only proxy of a mapping. It provides a dynamic view on the mapping's
entries, which means that when the mapping changes, the view reflects these
changes.
Use :class:`frozendict` if you want an immutable shallow copy of a mapping.
|
Ok then, let's not touch this :) |
I didn't know provide links to the exact issue on github, but I can if others think that it is needed.
Refs #152405
Refs #152483
frozendictinstead ofMappingProxyType#152636