Feat/function call fixes

[albertjan]

this does two things:

• it handles **kwargs for javascript functions like this

Sk. object.__init__ = function __init__() {
    // arguments will contain *args and as last element the dict with **kwargs
};
Sk.object.__init__.co_kwargs = 1;

• it moves all external functions, stuff called from skulpt, to builtin to the end of the compile so we have access to function
• it adds internal.js with all the functions that are called from javascript

This fixes datetime.py and reassigning abs @prescod

[prescod]

The patch is too big for me to review and understand, but I can confirm that it fixes my test-case.

# This code works with Python and skulpt-master

def foo():
  print pyabs == abs

pyabs = abs
print pyabs == abs

foo()

print "Success"

[albertjan]

@prescod The reassigning of functions failed for a few pretty obscure reasons. I'll try and explain a little.

• a hack we did in loadname to have top level properties for processing. For example there is a mouseX property this changes when the mouse moves, but because it doesn't live in a class it cannot be a python property.
• these builtin functions were not real skulpt function
• because internal object rely on some of these builtins the load order caused us not to be able to make them Sk.builtin.func's

loadname tries to get the name from the current scope and when it doesn't find it it looks in the builtins. If it does find it and it sees it's a javascript function it assumes it's this property hack for processing and calls the function to get the value. This works fine when abs lives in builtins but when you reassign it to somewhere in the local scope it goes down the wrong path. Because it's not a skulpt function.

To make them skulpt functions I had to move them down in the load order so Sk.builtin.func is available when we make these functions. This gave me a good opportunity to split out internal api and external api functions. So internal.js now contains the functions you call from javascript and builtin.js contains the functions you call from skulpt, and these are now real functions, they don't go down the property hack path anymore making them reassignable. 🎉

[eah13]

This fixes bugs as advertised.

but changing Sk.builtins to Sk.internal is a major API change that breaks some custom modules, including our tests/checks suite, which apparently calls things like Sk.builtins.asnum$ directly:
http://elliott-dev.trinket.io/library/trinkets/da6cfde875

We could of course change our module and ship, provided other third party modules we use like numpy, pygame.events, or sense_hat don't have hidden direct calls. But an unknown amount of third party module breakage would be a major impediment to getting this upstream, and with something this big I think we need it upstream if we're going to be able to accept future PRs to builtins, etc. And I don't think it's time for Skulpt 2.0.

So at first glance I believe the solution would be to give Sk.builtins aliases to relevant parts of Sk.internals for backwards compatibility? Or perhaps there's a benefit to leaving the Sk.builtins API intact and going with something like Sk.builtin_exports for the new and improved stuff? You probably have some better ideas.

Butterfly effects at every turn this far down. 🦋

[eah13]

I believe we'll need to preserve the Sk.builtins API.

[albertjan]

@eah13 Eek you're right ... this sucks. Back to the drawing board.

[albertjan]

Holy crap @eah13 this was a special one. 😄 but the end solution is a lot better. I wonder why we never thought of this. The changes to calling javascript functions from skulpt are a bit suspect still I will try and write some unittests around this.

[eah13]

One small typo I think in fabs (which shouldn't be in the builtins, but hey it's Skulpt).

You mentioned wanting to add tests as well? I think that would be good, especially ones that give kwargs for builtins a workout. Maybe also some Sk.builtin API tests to head off any third party module breakage.

[eah13]

Should this be .fabs?

Maybe this could be refactored into an array of names and a for each? Might make it more maintainable if builtindict.js could run off of that array as well.

[albertjan]

@eah13 do you have an idea of how often it's used in trinkets? I'm pretty sure we can get this removed upstream. We could throw an Exception saying this function is deprecated and moved to the math module.