py/obj: Remove mp_generic_unary_op(). #10348
Conversation
|
Code size report: |
|
This looks like a good change (to reduce code size, and simplify things). But, does it now allow hashing of objects that were not previously hashable, and which should not be hashable? |
No. We know this is the case because all of the tests are still passing. This works because the fallback only takes effect when the |
|
Here are some examples of things that now become hashable by the change in this PR, that were not previously hashable: hash(list.pop)
hash([].pop)
# closure
def f(x):
def g():
return x
return g
hash(f(1))
# slice
class A:
def __getitem__(self, i):
return i
hash(A()[1:2])The first 3 are hashable in CPython, so that's an improvement. The last one (slice instance) is not hashable in CPython. |
dlech
force-pushed
the
drop-generic-unary-op
branch
from
db42e09
to
6d174ff
Compare
Good one. I found the reason for this and added a fix and a test. |
dlech
force-pushed
the
drop-generic-unary-op
branch
from
6d174ff
to
b8c6bb4
Compare
Codecov Report
@@ Coverage Diff @@
## master #10348 +/- ##
=======================================
Coverage 98.50% 98.50%
=======================================
Files 155 155
Lines 20537 20541 +4
=======================================
+ Hits 20229 20233 +4
Misses 308 308
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
dlech
force-pushed
the
drop-generic-unary-op
branch
2 times, most recently
from
10a2147
to
0c93940
Compare
|
The remaining comment/concern I have about this change is that now most of the non-core types are hashable, eg:
And maybe other things I didn't think of. Actually, it looks like files are hashable in CPython (but not in uPy without this PR), so that is again an improvement. I think we need to go through all types and see which become hashable and decide whether that's acceptable or not. |
|
This should be the nearly complete list.
|
|
In CPython, any type written in Python will inherit the default So unless there are any MicroPython-specific types (extmod) that are "odd" and should not be able to be used as a dict key, I think we can lump all of those in the "the change is actually an improvement" pile. So that just leaves core types where we should make sure they match the CPython behavior if applicable. Anything in the table above that says "yes" for "has unary_op" is not going to change behavior with the PR. And So that leaves us with the following that could warrant further inspection:
|
|
The following are items that clearly use the default hash in CPython so I've checked these off the list:
Items that need to be fixed in MicroPython: Bound methods are discussed in #5233 This just leaves a few internals like |
|
Broad question: this will effect custom types defined by users as well if I get it correctly; what's like the worst thing which could happen if no action is taken to deal with this? |
|
Yes it will affect custom types implemented in C that don't assign anything to the The worst that could happen (software-wise) is that someone could be depending on their type not being hashable, i.e. trying to add an object to a set and catching the TypeError, and their code would break and they would have to fix it like we did with I can't really think of any practical cases where someone would be depending on this though. |
dlech
force-pushed
the
drop-generic-unary-op
branch
from
0c93940
to
21dfa07
Compare
|
I added a commit for |
|
I also searched the MicroPython code base for cases where we have a |
|
I checked the following types:
In all 5 cases CPython allows hashing the resulting object/instance. And MicroPython currently does not, but with this PR it will (because those types don't have an explicit unary_op handler so will fall back to the default and support hashing). So that is a good improvement for those 5 types. I have marked those 5 off the list above. I'll see about adding tests for them. |
Since converting to variable sized slots in mp_obj_type_t, we can now reduce the code size a bit by removing mp_generic_unary_op() and the corresponding slots where it is used. Instead we just implement the generic `__hash__` operation in the runtime. Signed-off-by: David Lechner <david@pybricks.com>
As per https://bugs.python.org/issue408326, the slice object should not be hashable. Since MicroPython has an implicit fallback when the unary_op slot is empty, we need to fill this slot. Signed-off-by: David Lechner <david@pybricks.com>
This adds a unary_op implementation for the dict_view type that makes the implementation of `hash()` for these types compatible with CPython. Signed-off-by: David Lechner <david@pybricks.com>
dpgeorge
force-pushed
the
drop-generic-unary-op
branch
from
21dfa07
to
32d853e
Compare
Signed-off-by: Damien George <damien@micropython.org>
dpgeorge
force-pushed
the
drop-generic-unary-op
branch
from
32d853e
to
9accb7d
Compare
|
Happily, this PR is still a net decrease in code size. |
|
Merged. Thanks @dlech for your efforts on this. |
Since converting to variable sized slots in
mp_obj_type_t, we can now reduce the code size a bit by removingmp_generic_unary_op()and the corresponding slots where it is used. Instead we just implement the generic__hash__operation in the runtime.