[BUG] `frozendict` isn't considered a `dict` on older py3 versions

[Lex-DRL]

OS version (https://www.google.com/search?channel=fs&q=check+os+version&ie=utf-8&oe=utf-8):
Windows 11 25H2 (build 26200.7623)

Python3 version (python3 -V -V):
Python 3.10.11 (tags/v3.10.11:7d4cc5a, Apr 5 2023, 00:38:17) [MSC v.1929 64 bit (AMD64)]

Steps to reproduce:

1. use python 3.10
2. isinstance(frozendict({}), dict)

Actual result (with the python stack trace if present):

False

[Marco-Sulla]

This is not a bug. It's the intended behavior of the C extension, that is available for Python 3.10- only for now.

You can force the pure py version, but probably it's not what you really want ;)
What's exactly your problem?

[Lex-DRL]

If it's intended behavior, then no problem... I guess.

But it's odd. There's a common use case where your function in some shared package expects a dict as an argument, and you might want to type-check it to be sure. I know it's not pythonic, but sometimes you just need to enforce the type strictly. Thus, someone who uses any variant of a dict (including frozendict) would reasonably expect that it could be passed to such function. But it would fail.

I discovered this issue from a bug report on my custom node for ComfyUI, which could be used in various environments with various python versions. For my case, I fixed it to be less strict about the type. But I believe there's a ton of legacy code which still does "dumb" isinstance(arg, dict) check instead of "the right one" isinstance(arg, typing.Mapping).

[Marco-Sulla]

But it's odd.

Yeah, it is. But there are people that find odd the contrary: that frozendict inherits from dict in the pure py version. And probably they are right.
The only reason frozendict inherits from dict in pure py is for improving its speed, nothing else.

I know it's not pythonic, but sometimes you just need to enforce the type strictly

Not in this case ;) Just take my considerations as suggestions:

I just took a look to this code. Here, isinstance is not needed, since in the code there's a input_dict.keys() call. So duck typing is sufficient here.

Notice also that the type of input_dict in the signature of the function should be updated to accept any map and None. But since typing in Python is boring and it's an internal function, who cares.

Here, on the contrary the isistance is ok, but I fail to see the usefulness of this function... ?_? Furthermore, since both dict and frozendict "inherit" from Mapping, isinstance(v, _t.Mapping) is enough.

Thus, someone who uses any variant of a dict (including frozendict) would reasonably expect that it could be passed to such function. But it would fail.

There's nothing we can do here. A lot of people can easily inherit from collections.abc.Mapping. And that's good! So unfortunately people can't use nothing other than dicts there, until there's someone patient as you that will fix the code :)

instead of "the right one" isinstance(arg, typing.Mapping)

IMHO for 99% of cases, the "right one" is no check at all. But I'm not Dutch ;)

[Lex-DRL]

I could respond to your takes questioning why I added this type check at all, but let's save time to both of us and I'll just accept the rejection. No is no. OK. 🤷🏻‍♂️

TL;DR of it: improving compatibility with other ComfyUI nodes I'm not even aware of.

Type hints - yeah, I should update those.

[Marco-Sulla]

let's save time to both of us and I'll just accept the rejection

You can simply accept my nope. I'm just trying to explain the why. You can skip it if you want :)

The reasons is simple: if frozendict will inherit from dict in the C extension too, you can mutate it.

That's all. The C extension makes frozendict "really" immutable. I use the quotes since it's written in C, so...