Open sorawee opened 5 years ago
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......)
https://github.com/brownplt/pyret-lang/commit/2bc5e6643c814209feb4c62e78c2c5cbb1c23c2e is a first attempt at fixing this, but it's not thoroughly tested.
@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?
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...)
Are we still pushing fixes into the horizon
branch these days?
Yes.
On Thu, Jul 25, 2019, 4:11 PM ds26gte notifications@github.com 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 https://github.com/brownplt/pyret-lang/issues/1470?email_source=notifications&email_token=AAHAHQDSGMJOB4E37HK7P3LQBICIFA5CNFSM4IG4F2KKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD22ULSA#issuecomment-515196360, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHAHQC7XBQZIOMRZ3PLHNDQBICIFANCNFSM4IG4F2KA .
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 notifications@github.com 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 https://github.com/brownplt/pyret-lang/issues/1470?email_source=notifications&email_token=AAHAHQDSGMJOB4E37HK7P3LQBICIFA5CNFSM4IG4F2KKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD22ULSA#issuecomment-515196360, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHAHQC7XBQZIOMRZ3PLHNDQBICIFANCNFSM4IG4F2KA .
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?
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...
results in:
This is the same problem as #819.