Skip to content

[mypyc] feat: true_dict_rprimitive for optimized dict fastpath usage #19499

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

BobTheBuidler
Copy link
Contributor

@BobTheBuidler BobTheBuidler commented Jul 24, 2025

This draft PR adds a true_dict_rprimitive which allows mypyc to differentiate between an actual dict instance and a subclass of dict.

Since this impacts any function that builds kwargs, plus dict comprehensions and other things, the special casing seems reasonable.

I chose not to map builtins.dict to the new rprimitive because we cannot guarantee types of input arguments. I intended to ONLY use it in instances where the dict type is certain. I'm not sure if this decision is correct or not, looking for opinions.

There is one test failure I still need to figure out, but I'd appreciate any feedback on the direction.

Copy link
Contributor

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@brianschubert
Copy link
Member

(Just a passing remark, I haven't looked at the actual change)

I think "exact dict" would be a better term than "true dict" to describe instances of dict not including instances of subtypes of dict. Similar terminology is used in Python's C API, e.g. PyDict_CheckExact.

@BobTheBuidler
Copy link
Contributor Author

Should we maybe just rename dict_rprimitive to dict_subclass_rprimitive?

I noticed while coding this PR that it isn't mapped the same way as the other rprimitive types, and this is not clear from the definition in rtypes, so maybe renaming that var and then changing true_dict_rprimitive to dict_rprimitive is the optimal choice.

@BobTheBuidler
Copy link
Contributor Author

BobTheBuidler commented Jul 26, 2025

I decided to go ahead and map builtins.dict to the new primitive, and updated the naming as per @brianschubert recommendation. Everything seems to be working except for some issue with defaultdict. For some reason, defaultdict is now also mapped to the new dict primitive. but my if check is very clearly name== "builtins.dict" which should not be the case for defaultdict?

If I remove my new line from the mapper, which leaves "builtins.dict" mapped to the existing primitive, all tests turn green.

If we could decide we want the check, we need to figure out why mypy is treating defaultdict like a non-subclass dict. One of you guys might already know why. Maybe mypy handles defaultdict uniquely. It might be the case that this is not possible to workaround in a reasonable fashion.

I need an opinion from a maintainer on the best way to proceed here. I could remove the new check from the mapper and we could merge this in basically now.

@BobTheBuidler BobTheBuidler marked this pull request as ready for review July 26, 2025 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants