Skip to content

Conversation

@TIGirardi
Copy link
Contributor

Currently, the pure python module (map.py) uses None as a marker for child nodes.

However, None is hashable and it is possible to be used as a key both in the builtin dict and in the C Map implementation.

The python version actually accepts setting None, but every subsequent operation that reaches the key mistakes it for a node marker and tries to operate on the value.

__slots__ = ()
__hash__ = None

_NULL = _Unhashable()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd call it NULL, the module isn't imported directly anyways. Also I think you can just use void since this is just a marker that users never see.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Note, void is hashable (object implements __hash__ without passing it down to subclasses), but that doesn’t matter if this is just used as a sentinel object anyway?

>>> foo = object()
>>> hash(foo)
9007199254740991

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I'm being a little paranoid, but I would prefer an immediate error, which void wouldn't do. This is the current behavior:

>>> from immutables.map import Map
>>> m = Map().set(None, 1)
# do a bunch of stuff
>>> None in m
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "$prefix/lib/site-packages/immutables/map.py", line 592, in __contains__
    self.__root.find(0, map_hash(key), key)
  File "$prefix/lib/site-packages/immutables/map.py", line 157, in find
    return val_or_node.find(shift + 5, hash, key)
AttributeError: 'int' object has no attribute 'find'

With the patch (if somehow the user uses _NULL as key) it raises a TypeError immediately.

Whatever you decide, I can change with a button click.

@1st1 1st1 merged commit 913572c into MagicStack:master May 18, 2020
@1st1
Copy link
Member

1st1 commented May 18, 2020

Alright, I merged as is. Thanks so much for help! I'll release tomorrow.

@TIGirardi TIGirardi deleted the none-keys branch May 18, 2020 22:57
@TIGirardi
Copy link
Contributor Author

Thank you!

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.

3 participants