gh-152405: Fix crash of 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?

[vstinner]

IMO it's a good approach to fix the issue.

[vstinner]

LGTM. Creating a copy to call __eq__() is a good fix, and the test coverage is good.

I'm fine with not fixing this issue for OrderedDict for performance reasons.

Maybe we should document that MappingProxyType doesn't fully hide the internal mapping in https://docs.python.org/dev/library/types.html#types.MappingProxyType. We can now suggest replacing MappingProxyType with frozendict which doesn't have this issue.

[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 :)