gh-152405: Do not expose the internal mapping when rich-comparing MappingProxyType objects

[sobolevn]

This is an alternative to #152449

Now I send a copy of the dict if it is under a mapping, so it can't be mutated.
Credit for this idea goes to @da-woods in this comment

• Issue: vars's mapping proxy can expose internal dictionary even for built-in types #152405

[da-woods]

Would it be worth optimizing the common case where an exact (any?)dict is ultimately being compared with another exact (any?)dict. Because if it's just going to go through the dict comparison operator then it should be safe?

[da-woods]

The other variation that I was thinking about (but is untested so I might have missed a detail), is to do exact (any?)dict <-> exact (any?)dict specifically*, and then return not implemented for anything else.

I think the result of that is that it would then fall back to the other classes' comparison operator, which would presumably be able to use the mapping interface of the proxy type to do the comparison.

[*] possibly also with a a check for the comparison operator because the collections.OrderDict just uses the dict comparison operator

[da-woods]

Perf results

That does suggest it might be get you a huge amount of benefit. Ah well...

[da-woods]

On more comment (then I stop thinking about this): if you're only concerned about the crash and not the leak then the only case you're really worried about is MappingProxy(dict()) == something_with_custom_operator (because that's what lets you change builtin types).

[sobolevn]

@da-woods thanks a lot for your help and ideas!

I am not sure that something_with_custom_operator is easily representable in C 🤔
Because mappingproxy can compare a lot of things: dict, frozendict, ordereddict, itself (with all possible combinations), etc.

[da-woods]

I am not sure that something_with_custom_operator is easily representable in C

I think it's just "not the dict comparison operator".

---

I've put an alternative (draft) proposal in #152489 mostly to see if it works and passes everything. It intentionally isn't complete though and is just a quick C code suggestion right now.

[sobolevn]

@da-woods please, check my new approach :)
It passes all tests, does not crash, and does not have any behavior changes.

[da-woods]

Yes - I believe that will work. It may be worth special casing not making the copy when you know the right-hand side is another exact anydict. But that's a minor implementation detail.

If I was starting from scratch with no existing tests or behaviour then I think I'd prefer my approach. But obviously Python is very much not starting from scratch.

So yeah - I'd agree that this fixes the important problem in the least modifying way.

[vstinner]

Would you mind to add tests on frozendict?

[sobolevn]

Maybe we should document that MappingProxyType doesn't fully hide the internal mapping

Sure! I will open a new PR for that :)
Thanks a lot for the helpful review.

I would love to hear other opinions before merging, let's give some time for @serhiy-storchaka or @picnixz or @methane to review this PR as well.

I will keep this open for a week or so :)

[sobolevn]

I reliazed that I forgot Py_TYPE(w) == &PyDictProxy_Type check and added it, so we won't copy any data on MappingProxyType({}) == MappingProxyType({})

[picnixz]

I wonder whether we really need a copy. It will undoubtly make the comparisons slower for a case that is honestly not realistic. I don't know how often people use MappingProxyType on their own types though so it may also be a niche performance drop.

I'm more concerned with the behavior change. Is it still a behavior change? Because that behavior change may still break code that was previously not crashing (albeit incorrect). If I were to compare two mappings like that it's hard to decide whether the == test above is expected to be true or false. In some sense, the contents are the same but not their types. Should MappingProxyType carry the type itself? I don't know. It's a view so it's already of different type... but at the same time it can be considered a view constrained on its input type. In this case, I would expect == to fail.

[sobolevn]

I'm more concerned with the behavior change

I think that I confused you with the older outdated description.
In the last version - there's no behavior change.

It will undoubtly make the comparisons slower for a case that is honestly not realistic.

I will only make things noticably slower for cases where we compare MappingProxyType with objects other than: dict, frozendict, OrderedDict, MappingProxyType. I don't think that there are lots of realistic cases where this is actually used. So, I would not be as worried :)

[methane]

I don't think "Fix crash" is accurate. This PR only reduces one case where the underlying dict leaks. Python is a very dynamic language, and it does not guarantee that users cannot access the built-in type dict. Being able to access the underlying dict is not a bug, nor is it a cause of crashes.

What we should consider is whether we can safely make changes to the built-in type dict, and if not, whether we should reduce Python's dynamic nature a bit and make it a frozendict.

[sobolevn]

I don't think "Fix crash" is accurate.

How would you rephrase it? :)

What we should consider is whether we can safely make changes to the built-in type dict, and if not, whether we should reduce Python's dynamic nature a bit and make it a frozendict.

Sounds like a good idea! But, it is clearly out of scope of this PR.

[sobolevn]

I changed the wording to be not about the crash :)

I will keep this open for a week or so :)

I will soon merge the PR if there's no other feedback to address.
Thanks everyone for your help! 👍

[miss-islington-app]

Thanks @sobolevn for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14, 3.15.
🐍🍒⛏🤖

[miss-islington-app]

Sorry, @sobolevn, I could not cleanly backport this to 3.14 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 701a7c5408f689b26c46ed7ed13879375fa82959 3.14

[bedevere-app]

GH-153670 is a backport of this pull request to the 3.15 branch.

[miss-islington-app]

Sorry, @sobolevn, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 701a7c5408f689b26c46ed7ed13879375fa82959 3.13

[sobolevn]

#136204 was not backported to 3.13 and 3.14, so I propose not to backport this as well.