py/obj: Add support for int special method. #4088

pfalcon · 2018-08-31T09:06:24Z

Depends on existing MICROPY_PY_ALL_SPECIAL_METHODS.

pfalcon · 2018-08-31T09:07:44Z

Based on the considerations in #4084

pfalcon · 2018-09-11T05:00:38Z

This patch as it is has issues. It was written to add minimal code, in particular it doesn't patch mp_obj_int_make_new(). So, it relies solely on mp_obj_get_int() to do the __int__ calling, but then the culprit is here in mp_obj_int_make_new().

https://github.com/micropython/micropython/blob/master/py/objint.c#L66

// try to convert to small int (eg from bool)
return MP_OBJ_NEW_SMALL_INT(mp_obj_get_int(args[0]));

pfalcon · 2018-09-11T05:05:51Z

So, this calls MP_OBJ_NEW_SMALL_INT() because it was intended to work with bools, and that's probably it. But when extended to work with arbitrary types, it will lose the highest bit of mp_int_t on converting it to small int.

Replacing it with mp_obj_new_int() is still not ideal. Let's trace how works

return mp_obj_new_int(mp_obj_get_int(args[0]));

It will:

__int__ called on object by mp_obj_get_int(). That returns mp_obj_t, so if value doesn't fit into small int, a long int (like MPZ) will be created.
mp_obj_get_int() will then extract mp_int_t from it.
And then mp_obj_new_int() will create another long int.

I.e., long int will be created 2 times, one time for garbage. And if object value doesn't fit into mp_int_t, it won't work at all. So, mp_obj_int_make_new() should directly call __int__.

This shows that adding such special methods bit by bit adds more code. I still think it's good idea, as int is a universal type to which various other could be coerced. But that's also why stopping at supporting just __int__ is a good idea. ;-)

I'll submit update patch soon.

dpgeorge · 2018-09-11T05:06:12Z

py/obj.c

@@ -235,12 +235,17 @@ mp_int_t mp_obj_get_int(mp_const_obj_t arg) {
    } else if (MP_OBJ_IS_TYPE(arg, &mp_type_int)) {
        return mp_obj_int_get_checked(arg);
    } else {
+        #if MICROPY_PY_ALL_SPECIAL_METHODS
+        mp_obj_t res = mp_unary_op(MP_UNARY_OP_INT, (mp_obj_t)arg);
+        return mp_obj_int_get_checked(res);


If the return value of mp_unary_op is not an integer (small or big) then this will segfault.

If the return value of mp_unary_op is not an integer (small or big) then this will segfault.

So, do we have similar checks everywhere by now? I can add that check to mp_unary_op(), too bad we don't have a define to disable such checks. (MICROPY_FULL_CHECKS is described to be crash-safe.)

Addressed as described in the main comment stream.

dpgeorge · 2018-09-11T05:07:40Z

py/obj.c

        if (MICROPY_ERROR_REPORTING == MICROPY_ERROR_REPORTING_TERSE) {
            mp_raise_TypeError("can't convert to int");
        } else {
            nlr_raise(mp_obj_new_exception_msg_varg(&mp_type_TypeError,
                "can't convert %s to int", mp_obj_get_type_str(arg)));
        }
+        #endif


This changes the error message of this function if an integer-like object is not passed in. It now relies on mp_unary_op to raise the error message which is more generic (too generic) than the error message that is here. Eg when you pass something that's not an int into a function expecting an int, it should say "can't convert to int".

This changes the error message

Yes, it does. But the new message is not false nor unclear:

>>> int([]) Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: unsupported type for __int__: 'list'

That can be seen as the price of supporting special methods like __int__: users now should know that operators are implemented in terms of them. And the error message like above offers a good opportunity to learn! ;-)

Beyond that, uPy's error message was different from CPy's already:

>>> int([]) Traceback (most recent call last): File "<stdin>", line 1, in <module> TypeError: int() argument must be a string, a bytes-like object or a number, not 'list'

So, no much harm if it's different again.

As you imagine, trying to do anything else about error messages would be rather cumbersome.

pfalcon · 2018-09-12T05:03:32Z

I'll submit update patch soon.

So, posted an update to address problem described in that comment. Will look into addressing value type checking/error message concerns next.

pfalcon · 2018-09-12T05:21:12Z

If the return value of mp_unary_op is not an integer (small or big) then this will segfault.

This is now addressed. But it's done on the level of instance_unary_op(), where wrong-typed value can be easily returned by user. This is consistent with handling special requirements for __hash__. Native code, we trust. Because otherwise it has too many opportunities to do something wrong anyway.

pfalcon · 2018-09-12T21:33:58Z

This changes the error message

This also has been addressed. It's done in a separate commit so far, and I guess it's better if it stays that way.

pfalcon · 2018-09-12T21:36:50Z

Note that #ifdef'ing required (location of the "detailed" error gets swapped depending on whether MICROPY_PY_ALL_SPECIAL_METHODS is defined) makes to wonder if it's cleaner to just enable __int__ unconditionally.

pfalcon · 2018-09-16T04:39:32Z

This should be ready for review, there's just random minor coveralls coverage decrease.

pfalcon · 2018-11-09T06:27:10Z

@dpgeorge, Ping.

dpgeorge · 2018-12-06T11:15:44Z

Sorry for the delay here.

Note that #ifdef'ing required (location of the "detailed" error gets swapped depending on whether MICROPY_PY_ALL_SPECIAL_METHODS is defined) makes to wonder if it's cleaner to just enable __int__ unconditionally.

Yes, I agree. After having a good look at this code again I see that it spends a lot of time trying to keep the old behaviour when the feature is disabled, which makes the overall code harder to understand and maintain in the future. I'd be happy to just have this unconditionally enabled. After all, __hash__ is unconditionally enabled and arguably __int__ is just as fundamental, if not more, than hashing. Making it unconditionally enabled shouldn't actually add that much code: the main addition is in objtype.c, the changes to the other files are just reorganising the flow of the logic.

Re tests: the new test seems to never run. I think the feature test should be int(cud1), not +cud1.

pfalcon · 2018-12-06T17:11:52Z

I'd be happy to just have this unconditionally enabled. After all, hash is unconditionally enabled and arguably int is just as fundamental, if not more, than hashing.

Well, hashing is surely more fundamental, as it allows to use objects as keys in dict, etc.

Otherwise the reason to make it conditional was to remain fair re: -1'ing patches by others which unconditionally add new features. How can I complain about that, if I don't do that myself. So, if there's understanding that it's an exception to enable __int__ unconditionally right away, sounds good. Because otherwise there's slippery slope for __index__, __float__, etc.

Re tests: the new test seems to never run. I think the feature test should be int(cud1), not +cud1.

Will have a look, but it's copied from special_methods2.py, and the idea is that it runs in the coverage build. For normal build, it's enabled in some other patch.

Based on the discussion, this special method is available unconditionally, as converting to int is a common operation.

pfalcon · 2018-12-06T20:33:52Z

Ok, reworked to make it unconditional. A test then added to existing special_methods.py.

dpgeorge · 2018-12-07T02:10:48Z

Thanks for updating.

Will have a look, but it's copied from special_methods2.py, and the idea is that it runs in the coverage build.

The standard unix build has all-special-methods enabled too, so it'll run there. What happened with the special_methods3.py test is that the __pos__ special method was removed from the test class, which meant the feature test didn't work anymore.

dpgeorge · 2018-12-07T02:11:44Z

The failure in Travis is only due to increase in minimal firmware size.

dpgeorge · 2018-12-07T06:33:35Z

Merged in b1d0872 and d690c2e, with minor change to error message handling, to add back mp_raise_TypeError("can't convert to int") for case when error reporting is configured to be terse.

coveralls · 2018-12-08T08:37:46Z

Pull Request Test Coverage Report for Build 9953

11 of 12 (91.67%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.005%) to 98.064%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
py/obj.c	1	2	50.0%

Totals
Change from base Build 9952:	-0.005%
Covered Lines:	18238
Relevant Lines:	18598

💛 - Coveralls

dpgeorge reviewed Sep 11, 2018

View reviewed changes

pfalcon force-pushed the unary-op-__int__ branch from 6e8ce16 to cee2a7b Compare September 12, 2018 05:02

pfalcon force-pushed the unary-op-__int__ branch from cee2a7b to 9772de3 Compare September 12, 2018 05:18

pfalcon force-pushed the unary-op-__int__ branch from 9772de3 to 6caeff6 Compare September 12, 2018 21:31

pfalcon force-pushed the unary-op-__int__ branch from 6caeff6 to e0fc938 Compare October 11, 2018 22:28

pfalcon force-pushed the unary-op-__int__ branch 2 times, most recently from 8a2325e to 350032c Compare December 6, 2018 20:31

Paul Sokolovsky added 2 commits December 6, 2018 23:32

py/obj: Add support for __int__ special method.

84d3143

Based on the discussion, this special method is available unconditionally, as converting to int is a common operation.

tests/basics/special_methods: Add testcases for __int__.

9302a0d

pfalcon force-pushed the unary-op-__int__ branch from 350032c to 9302a0d Compare December 6, 2018 20:33

dpgeorge closed this Dec 7, 2018

dpgeorge mentioned this pull request May 21, 2019

No int conversion for int-derived types available #1782

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

py/obj: Add support for int special method. #4088

py/obj: Add support for int special method. #4088

pfalcon commented Aug 31, 2018

pfalcon commented Aug 31, 2018

pfalcon commented Sep 11, 2018

pfalcon commented Sep 11, 2018 •

edited

Loading

dpgeorge Sep 11, 2018

pfalcon Sep 11, 2018

pfalcon Sep 12, 2018

dpgeorge Sep 11, 2018

pfalcon Sep 11, 2018 •

edited

Loading

pfalcon commented Sep 12, 2018

pfalcon commented Sep 12, 2018 •

edited

Loading

pfalcon commented Sep 12, 2018

pfalcon commented Sep 12, 2018

pfalcon commented Sep 16, 2018

pfalcon commented Nov 9, 2018

dpgeorge commented Dec 6, 2018

pfalcon commented Dec 6, 2018

pfalcon commented Dec 6, 2018

dpgeorge commented Dec 7, 2018

dpgeorge commented Dec 7, 2018

dpgeorge commented Dec 7, 2018

coveralls commented Dec 8, 2018 •

edited

Loading

py/obj: Add support for __int__ special method. #4088

py/obj: Add support for __int__ special method. #4088

Conversation

pfalcon commented Aug 31, 2018

pfalcon commented Aug 31, 2018

pfalcon commented Sep 11, 2018

pfalcon commented Sep 11, 2018 • edited Loading

dpgeorge Sep 11, 2018

Choose a reason for hiding this comment

pfalcon Sep 11, 2018

Choose a reason for hiding this comment

pfalcon Sep 12, 2018

Choose a reason for hiding this comment

dpgeorge Sep 11, 2018

Choose a reason for hiding this comment

pfalcon Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

pfalcon commented Sep 12, 2018

pfalcon commented Sep 12, 2018 • edited Loading

pfalcon commented Sep 12, 2018

pfalcon commented Sep 12, 2018

pfalcon commented Sep 16, 2018

pfalcon commented Nov 9, 2018

dpgeorge commented Dec 6, 2018

pfalcon commented Dec 6, 2018

pfalcon commented Dec 6, 2018

dpgeorge commented Dec 7, 2018

dpgeorge commented Dec 7, 2018

dpgeorge commented Dec 7, 2018

coveralls commented Dec 8, 2018 • edited Loading

Pull Request Test Coverage Report for Build 9953

💛 - Coveralls

py/obj: Add support for int special method. #4088

py/obj: Add support for int special method. #4088

pfalcon commented Sep 11, 2018 •

edited

Loading

pfalcon Sep 11, 2018 •

edited

Loading

pfalcon commented Sep 12, 2018 •

edited

Loading

coveralls commented Dec 8, 2018 •

edited

Loading