bbcmicrobit / micropython

Port of MicroPython for the BBC micro:bit
https://microbit-micropython.readthedocs.io
Other
608 stars 283 forks source link

__mul__ isn't being called as expected #522

Open ZanderBrown opened 6 years ago

ZanderBrown commented 6 years ago

The __mul__ magic function doesn't seem to be called as expected yet __add__ is

This sample will give the expected result (1.0) when run locally with

But when run on m:b fails with

TypeError: unsupported types for : 'Fraction', 'int'

Build:

MicroPython v1.9.2-34-gd64154c73 on 2017-09-01; micro:bit with nRF51822

Explicitly calling __mul__ does give the expected result so I expect something isn't plummed in somewhere

Also worth noting that:

class A:
    pass

A() + 1

Will generate:

TypeError: unsupported types for __add__: 'A', 'int'

But * just gives unsupported types for nothing

Originally discovered by @romilly

dpgeorge commented 6 years ago

To save space, not all special methods are enabled in the micro:bit build. Only __add__ and __sub__ are supported, along with comparison (<, >, =, <=, >=).

ZanderBrown commented 6 years ago

Ah okay, perhaps the error should reflect this?

romilly commented 6 years ago

And maybe the docs should also warn somewhere? I can do a PR for the docs - maybe a note to that effect in the Developer FAQ, along with a link to the 'differences' page on the wiki https://github.com/micropython/micropython/wiki/Differences?

On 6 June 2018 at 15:14, Zander notifications@github.com wrote:

Ah okay, perhaps the error should reflect this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bbcmicrobit/micropython/issues/522#issuecomment-395083706, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJHPieo6EX2cZdhf_hJZ75dCQYi9bcdks5t5-O1gaJpZM4UckUp .

-- personal:@romillyc work:@rareblog skype:romilly.cocking web: http://blog.rareschool.com/

dpgeorge commented 6 years ago

Ah okay, perhaps the error should reflect this?

Adding such checks and errors would probably increase code more than enabling the feature :)

Is there a compelling use case (from a teaching point of view perhaps) for these special operators? I can check how much they cost to enable, maybe they are worth adding after all.

And maybe the docs should also warn somewhere?

Yes that would be a good solution, if this feature ends up staying disabled.

romilly commented 6 years ago

I'd like to get Fractions working, as they are a great example of Python's extensibility and useful in quite a few projects.

(Code is in https://github.com/romilly/fractions)

The current code relies on dunders for add, sub, radd, rsub, mul, rmul, div, rdiv.

I could live without radd, rmul rdiv, though they are attractive from a teaching perspective.

On 6 June 2018 at 15:29, Damien George notifications@github.com wrote:

Ah okay, perhaps the error should reflect this?

Adding such checks and errors would probably increase code more than enabling the feature :)

Is there a compelling use case (from a teaching point of view perhaps) for these special operators? I can check how much they cost to enable, maybe they are worth adding after all.

And maybe the docs should also warn somewhere?

Yes that would be a good solution, if this feature ends up staying disabled.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bbcmicrobit/micropython/issues/522#issuecomment-395088724, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJHPqqf6J-0k0diPj925PSler4JMtAZks5t5-cygaJpZM4UckUp .

-- personal:@romillyc work:@rareblog skype:romilly.cocking web: http://blog.rareschool.com/

ZanderBrown commented 6 years ago

Yes I think the Fractions class is a very good case as it's education both in how it's implemented and that it's fractions (and is just plain cool)

As for changing the error would it not just need to check if mp_binary_op_method_name[op] is actually defined?

https://github.com/bbcmicrobit/micropython/blob/e26d7c89d4a96de0fa0a1dd5aec024b31fc4816e/source/py/runtime.c#L564-L566

dpgeorge commented 6 years ago

Enabling the config option MICROPY_PY_ALL_SPECIAL_METHODS to get __mul__ and other support costs 160 bytes of code space. This doesn't include support for reverse special methods (eg __rmul__) because that requires an upgrade to the latest upstream MicroPython to get it working.

romilly commented 6 years ago

Thanks for that. Blog post coming real soon now(tm)

It would be great to get the reverse specials at some point, but I can use their absence to explain what you;d get from them.

On 12 June 2018 at 05:42, Damien George notifications@github.com wrote:

Enabling the config option MICROPY_PY_ALL_SPECIAL_METHODS to get mul and other support costs 160 bytes of code space. This doesn't include support for reverse special methods (eg rmul) because that requires an upgrade to the latest upstream MicroPython to get it working.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bbcmicrobit/micropython/issues/522#issuecomment-396463827, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJHPjGtS8At55VHqL8wd4jg5MbTuyWCks5t70acgaJpZM4UckUp .

-- personal:@romillyc work:@rareblog skype:romilly.cocking web: http://blog.rareschool.com/

dpgeorge commented 6 years ago

Thanks for that. Blog post coming real soon now(tm)

Sorry, maybe it was misunderstood, but I didn't actually enable the option, just checked how much it cost :) Probably it's small enough that we can enable it. And with the latest version of upstream MicroPython there are space savings in other places that would counteract the addition.

romilly commented 6 years ago

I was going to enable and re-build, but it sounds as if it would be better to wait for the new version.

I have plenty to keep me busy atm as I am working on a testing harness for a multi-microbit application.

On 12 June 2018 at 07:10, Damien George notifications@github.com wrote:

Thanks for that. Blog post coming real soon now(tm)

Sorry, maybe it was misunderstood, but I didn't actually enable the option, just checked how much it cost :) Probably it's small enough that we can enable it. And with the latest version of upstream MicroPython there are space savings in other places that would counteract the addition.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bbcmicrobit/micropython/issues/522#issuecomment-396476578, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJHPox3RUIkMZSjaOypaZZVQ43IzqeSks5t71tmgaJpZM4UckUp .

-- personal:@romillyc work:@rareblog skype:romilly.cocking web: http://blog.rareschool.com/