Alternate design to implementing & testing jsnums

[blerner]

Alternate design to #1818, where instead of manually placing errbacks parameters everywhere throughout the code, and where even a single missed errbacks argument could be a brittle and hard-to-find failure later, this one changes js-numbers to only expose a module-creating function that closes over the errbacks parameter, and exports the created jsnums instance as a field on runtime -- so that modules that previously imported jsnums directly can now access it off their respective runtimes.

I've pulled in the tests from #1818, and they all pass, and I think I've pulled in any substantive changes to runtime.js as well.

[jpolitz]

Makes me a bit nervous that we have to make sure we account for every client of js-numbers here, because it is no longer exporting a bunch of functions it used to.

Is there a way to do this that is backwards-compatible?

Like, maybe all the existing functions are also exported, with an errbacks argument, and they call MakeNumberLibrary with the given errbacks and then the appropriate function?

Sketch:

const libByErrbacksCache = new Map(); // cache of instantiated NumberLibrary objects by errbacks object identity
function makeBackcompatWrapper(name) {
  NumbersExport[name] = function() {
    if(libByErrbacksCache[name]) { return errbacksCache[name].apply(arguments); }
    const errbacks = arguments[arguments.length - 1];
    const lib = MakeNumberLibrary(errbacks);
    libByErrbacksCache[errbacks] = lib;
    return lib[name].apply(arguments);
  }
}
makeBackcompatWrapper("fromFixnum");
makeBackcompatWrapper("fromString");
...

[jpolitz]

Properties this has:

• If someone was forgetting errbacks arguments, oh well, they still are
• If someone calls over and over again with an errbacks argument, they pay a small cost of the cache check but no more
• We don't break any existing code

[blerner]

I don't think that will quite work, because BigInteger and Rational are generative types -- they're defined within the MakeNumberLibrary() closure, so if a "legacy caller" of the non-closed-over library uses multiple functions in a row (with structured numbers), they'll all fail each others' isPyretNumber checks. We'd have to do something fancier with object-identity of the errbacks objects themselves, I think, to get the cache to not be generative?

Do we have any library authors who write native JS libraries for Pyret that we're not aware of? Because if not, then as of this PR (and the counterpart for CPO), the only mentions of js-numbers are:

blerner@blerner-x1:~/pyret-lang$ grep -rn "pyret-base/js/js-numbers" src/ tests/ --exclude="*.jarr"
src/js/base/js-numbers.js:107:define("pyret-base/js/js-numbers", function() {
src/js/base/runtime.js:3:   "pyret-base/js/js-numbers",
src/scripts/standalone-configB.json:6:    "pyret-base/js/js-numbers": "build/phaseB/js/js-numbers.js",
src/scripts/node_modules-config.json:6:    "pyret-base/js/js-numbers": "$PYRET/js/js-numbers.js",
src/scripts/standalone-configC.json:6:    "pyret-base/js/js-numbers": "build/phaseC/js/js-numbers.js",
src/scripts/standalone-configA.json:6:    "pyret-base/js/js-numbers": "build/phaseA/js/js-numbers.js",
tests/jsnums-test/jsnums-test.js:14:R(["pyret-base/js/js-numbers"], function(JNlib) {

and

blerner@blerner-x1:~/code.pyret.org$ grep -rn "pyret-base/js/js-numbers" src/ --exclude="*.jarr"
blerner@blerner-x1:~/code.pyret.org$ 

[jpolitz]

Re: “it's not hard to find all the uses”.

What about pyret-embed? What about pyret-npm? What about the anchor branch? What about Brown's internal grader or NEU's (and other similar projects we don't know about). I don't know. I could look. I've also started to lose track of all the projects. Some of these things also have issues that are hard to predict with e.g. different versions of runtime files being in caches (esp. with pyret-npm). I've been burned quite a bit recently by being flippant with breaking “internal” API changes.

Thinking more about the design options here.

[jpolitz]

A few ideas that would make me more comfortable:

• Make this new structure be called something else. Call it js-nums, or js-numbers-errbacks, or similar. Then import it into js-numbers, and export from js-numbers something like { ...MakeNumberLibrary({some default errbacks that throw}) }. This isn't a perfect drop-in because if some code is using the errbacks interface it will not respect that, but all the happy paths will keep being happy.
• Make this new structure be called something else. Call it js-nums, or js-numbers-errbacks, or similar. Then leave js-numbers alone except for adding a console.log or console.error saying it is deprecated and will be deleted at $DATE. Then we delete it at $DATE.

[jpolitz]

To be clear this PR is a vast improvement in ergonomics.

I just really, really don't want to have some 5-alarm fire because we broke an interface that we didn't know something relies on (could be something we wrote and forgot about! could be a huge pain of a built file sitting in someone's .pyret directory that I'm not thinking about, etc).

[blerner]

Fair points.

• I didn't consider the anchor branch because there are already so many diverging changes that we'd have to revisit this anyway.
• pyret-npm is a customer of Pyret, not an integrated tool, so it shouldn't import the js-numbers library anyway. (And grepping through the source shows it doesn't.)
• I didn't check pyret-embed because it's not in brownplt (I think?) -- I just looked through your repo now, though, and I think it's the same as pyret-npm in this regard.
• Autograders are a possible concern, yes.

I'm not sure I completely understand your latest suggestion, yet. Will think about it more...

[ds26gte]

Re: “it's not hard to find all the uses”.

What about pyret-embed? What about pyret-npm? What about the anchor branch? What about Brown's internal grader or NEU's (and other similar projects we don't know about). I don't know. I could look. I've also started to lose track of all the projects. Some of these things also have issues that are hard to predict with e.g. different versions of runtime files being in caches (esp. with pyret-npm). I've been burned quite a bit recently by being flippant with breaking “internal” API changes.

Thinking more about the design options here.

In case this is a useful data point: Neither Brown's nor NEU's autograder repos use pyret-lang's js-numbers.js as a JS library.

[jpolitz]

@blerner I think this is good to merge. Thanks for going through my roundabout complaints, I'm happy with where this landed now.

I think that the way this is set up now, the only case this will be backwards incompatible for on expected behavior is if some code was:

1. Using the jsnums library
2. Calling into jsnums methods on created number objects of specific subtypes, not the (intended public) static entrypoints, and calling a method that takes errbacks
3. Providing a custom errbacks to that method

If they were doing all of that, they will get the default errbacks that throw raw JS errors instead of their custom errbacks.

In all other ways this is backwards-compatible if you were using the library correctly.

This does change the expected use of the library a bit (which is generally all internal anyway), to use the instantiated API on runtime configured with the “right” Pyret errbacks. But it shouldn't suddenly break for someone using it the “old” way.

The only other backwards incompatibilities should only apply to uses that were incorrectly not providing errbacks anyway, so it's hard to blame us for changing their error behavior.