Roughnum overflow results in internal error

[sorawee]

~1e+308 + ~1e+308

results in:

Non Pyret value: TypeError: Cannot read property 'throwDomainError' of undefined

This is the same problem as #819.

[blerner]

Jeeeeeeez. Now that I'm looking at js-numbers again, there are a lot more places that need errbacks. (And because JS allows function calls of incorrect arity, we never noticed......)

[blerner]

2bc5e66 is a first attempt at fixing this, but it's not thoroughly tested.

[sorawee]

@blerner in the chart library I use a lot of these functions that you just add an extra argument. Should they be changed as well?

[blerner]

That's quite possible, yes. I think any of these functions could theoretically throw a numeric exception, so we need to start from all the exception-raising sites, and work outward to their callers, and outward from there, etc. until we get to either runtime.js or to another direct client of js-numbers. It looks like: ide.js, image-lib.js, world.js, the updated image libraries, d3-lib.js, d3-lib-list.js, plot-lib.js, plot-lib-list.js, chart-lib.js, and output-ui.js all potentially need fixing. (The worst culprit is toFixnum, which calls _integerDivideToFixnum, which is a makeIntegerBinop, which could call numerator and denominator and Roughnum.makeInstance, which each potentially use errbacks. I think none of those uses are live code when starting from toFixnum, but nevertheless we don't want mixed-arity calls...)

[ds26gte]

Are we still pushing fixes into the horizon branch these days?

[blerner]

Yes.

…

On Thu, Jul 25, 2019, 4:11 PM ds26gte ***@***.***> wrote: Are we still pushing fixes into the horizon branch these days? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1470?email_source=notifications&email_token=AAHAHQDSGMJOB4E37HK7P3LQBICIFA5CNFSM4IG4F2KKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD22ULSA#issuecomment-515196360>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHAHQC7XBQZIOMRZ3PLHNDQBICIFANCNFSM4IG4F2KA> .

[blerner]

Well, actually, let's make them PRs based on horizon. There's a *lot* of stuff in flux and I don't want horizon itself to churn too heavily without Joe or me right now.

…

On Thu, Jul 25, 2019, 4:11 PM ds26gte ***@***.***> wrote: Are we still pushing fixes into the horizon branch these days? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1470?email_source=notifications&email_token=AAHAHQDSGMJOB4E37HK7P3LQBICIFA5CNFSM4IG4F2KKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD22ULSA#issuecomment-515196360>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHAHQC7XBQZIOMRZ3PLHNDQBICIFANCNFSM4IG4F2KA> .

[ds26gte]

Just saw your PR and tested it for the particular bug reported by @sorawee. You may ignore my other PR as yours subsumes it.

Here are my initial notes (which are easily fixable):

• Make sure all calls to Roughnum.makeInstance use errbacks, (except for trivial stuff like Roughnum.makeInstance(1) which are known not to error).

• BigInteger.prototype.toFixnum should call errbacks if returning JS Infinity.

• To make integerDivideToFixnum work fault-tolerantly: makeIntegerBinop’s args should be allowed to take errbacks.

• Some makeBignum calls could fail because we’re implicitly assuming fixnum-ness of the exponent part. While this may be reasonable assumption, should allow it take errbacks when it fails.

(I essentially zeroed in on all cases that use JS to produce a fixnum, either explicitly via a toFixnum or implicitly when supplying a JS result to Roughnum.makeInstance. The + and - failure brought up by @sorawee is an instance of the latter.)

I notice there seems to be a Pyret function called num-to-fixnum, but I couldn't actually use it in the IDE. Are we really supplying this to the user? It is apparently dropped (intentionally?) when populating the Pyret environment, but all the set-up (including Pyret-ish name) is present.

Is it possible to tag along my mods to your commit (is it a PR, I can't see it), or do I just create a separate branch based off your commit?

[blerner]

• I think I just made a branch, so feel free to keep pushing onto that branch with additional changes.

• I think we want to avoid any variadic function calls, so even trivial calls to makeInstance should take errbacks.

• I'm pretty sure makeIntegerBinop's arguments can and do take errbacks, but double check that...