Added missing 3rd (of 4) arg to jsnums.toRepeatingDecimal() calls #556

[ds26gte]

These occur in ide.js and output-ui.js.

[blerner]

The code change looks reasonable. But do we have a test to make sure this is correct? The original issue definitely had something that triggered this bug, so we should be able to extract a regression test for this...

[schanzer]

@blerner @ds26gte here's the file that exposes the bug (evaluate Cell-TABLE after running). Here's the offending spreadsheet.

[jpolitz]

This seems like much more than a missing errback issue: somehow the parsing of this sheet is leading to a rational with a 0 denominator. The string renderers show it, the toRepeatingDecimal call in the DOM renderer errors:

https://code.pyret.org/editor#share=1a1Pd-fdFlF6rj3QfkDP-St8LNbhzdV8r&v=fee2ecd

The data is:

Molecules	Species	Speed
Na+	ions	5.00E-19

So somehow in the sheets reader we are getting 1298074214633707/0 as a value. In the debugger that value looks like this:

[jpolitz]

Sheet URL: https://docs.google.com/spreadsheets/d/1MDCFhQl2tw-FdALuBS8vXC3Kom11aMocgD1OPVS256c/edit?gid=0#gid=0

[jpolitz]

Note that the effect of the code in this PR is just to raise a nicely rendered DomainError, which is good, but should not happen on this input.

[blerner]

This sounds like something that's easily reproducible as a JS regression test, then? I.e. take the string 5E-19 (or 5.00E-19) and parse it and make sure we don't get a broken number?

[jpolitz]

The constant in a Pyret program is fine. It's something in the sheets reading/parsing.

[blerner]

But I think the field content is actually 5E-19, without the .00 -- I suspect the cell formatting is showing two decimal places. And in CPO,

According to Pyret, the value inside that some has a to-repr of 5e-19, a to-string of 5e-19, and yet renders as nothing at all in the output... So something is very strange with that value.

[jpolitz]

What browser are you on?

Brave Version 1.76.82 Chromium: 134.0.6998.178 (Official Build) (arm64)

[blerner]

Hmmm, I may have had stale state in my browser window somehow; refreshing made the problem go away, so /shrug?

[jpolitz]

OK, the culprit is fromFixnum in js-numbers, which is somehow wrong (IDK how yet) in the logic that was added for handling scientific notation literals:

brownplt/pyret-lang@8047792#diff-34cd626ab11424687bfd071b24342ee9d60c879050d69239ca8cf86df95e2f39R7-R77

fromFixnum is relatively hard to reach from Pyret (most things use makeNumberFromString which uses jsnums.fromString). However, json uses it, so a simple command-line repro is:

→ cat json-5e.arr
import json as J

print(J.read-json("5E-19"))
→ make json-5e.jarr
→ node json-5e.jarr
j-num(1298074214633707/0)

[jpolitz]

@ds26gte if you're still on this: a useful way to proceed would be to figure out how to write tests for fromFixnum (which likely was never tested very thoroughly), perhaps using json, and then make sure it works for many cases with E literals.

[ds26gte]

PR brownplt/pyret-lang#1810 addresses the fact that the error shouldn't happen (as opposed to having a decent error message).

[schanzer]

@jpolitz now that Dorai's PR (mentioned above) is merged into horizon, I can confirm that this issue is resolved. See https://pyret-horizon.herokuapp.com/editor#share=1WqFQ_uZtplDvcbi35ojDWZzOVbREP4un&v=dc8be4e.

@ds26gte if this means your other PR subsumes this one, please close. Otherwise, let us know what unique fixes are in this one?

[blerner]

These fixes are needed also, and are not subsumed by another PR as far as I know. They also are not tested, and I'm not thrilled about that...

Also, while tracing through this code into js-numbers, I found another missing errbacks argument, in the call to equals() on line 3905.

[ds26gte]

PR brownplt/pyret-lang#1810 (accepted) took care of the fact that this bug shouldn't happen.

PR brownplt/pyret-lang#1818 (pending) ensures

• proper error message on invalid args to toRepeatingDecimal(), with JS-tests (the only tests possible now, now that the bug can't happen via Pyret commandline or CPO)
• all calls to equals(), lessThan(), lessThanOrEqual() take errbacks. (greaterThan() and greaterThanOrEqual() were already clean)

[schanzer]

@ds26gte are you saying that this PR should be merged after #1818? Or that it's obviated by #1818 and should be closed?

[ds26gte]

@ds26gte are you saying that this PR should be merged after #1818? Or that it's obviated by #1818 and should be closed?

It could have been merged any time in the past and now. It fixes a call to toRepeatingDecimal() in CPO that was using a bad set of arguments, where an errbacks argument wasn't really an errbacks.

PR #1818 did not change the argument signature of toRepeatingDecimal() at all, so the fix introduced by this PR is still valid. However, an entirely different bug that had to do with misreading VERYSMALL rational numbers was fixed via PR #1810, so this bug is no longer exercised.

PR #1818 has added tests that ensure that if the original bug were still unfixed, a proper error message would have been raised. It does this by simulating (via JS) a situation where the original bug could still happen.

[ds26gte]

P.S. If you're asking me if this PR should be merged or abandoned, it should be merged!

[schanzer]

@blerner does #1818 add the tests you wanted, to allow this PR to be merged?

[blerner]

I'm not thrilled with the behavior of undefined-- <= 0: I had to look that one up, twice, to remind myself what was supposed to happen. (Undefined turns to NaN, which isn't comparable with anything -- two subtle type coercions). Could you add a line at the top of toRepeatingDecimal that says limit = limit ?? Infinity;, to make the behavior clearer?

[blerner]

I don't see any regression tests in #1818 that confirm that the missing argument would cause a problem. If I'm reading correctly, you've tested that toRepeatingDecimal when given four validly typed arguments will throw an errbacks error. A regression test for this ought to test that omitting the third (optional) argument causes a method-not-found js error, I think...?

Or, better, since errbacks is now a required argument, we should make limit be a required argument, and replace undefined with +Infinity. Then it's a trivial check for if (typeof limit !== "number"), to throw a contract error as needed. (We shouldn't have an "optional" argument before a required argument, and errbacks needs to be required.)

[ds26gte]

I'm not thrilled with the behavior of undefined-- <= 0: I had to look that one up, twice, to remind myself what was supposed to happen. (Undefined turns to NaN, which isn't comparable with anything -- two subtle type coercions). Could you add a line at the top of toRepeatingDecimal that says limit = limit ?? Infinity;, to make the behavior clearer?

This — i.e., relying an JS comparison quirk — is not what's happening though.

toRepeatingDecimal() does not directly take a limit argument at all. It rather takes an options argument, which can legitimately be undefined, which is indeed taken care of correctly. In order to specify a non-default limit, this options should be an object with field limit. Otherwise, the limit is set to 512, a very valid number. It is never undefined, and there is no reliance on undefined-- <= 0.

Using Infinity as an explicit 3rd argument to toRepeatingDecimal() would actually be a type error.

[ds26gte]

I don't see any regression tests in #1818 that confirm that the missing argument would cause a problem. If I'm reading correctly, you've tested that toRepeatingDecimal when given four validly typed arguments will throw an errbacks error. A regression test for this ought to test that omitting the third (optional) argument causes a method-not-found js error, I think...?

Or, better, since errbacks is now a required argument, we should make limit be a required argument, and replace undefined with +Infinity. Then it's a trivial check for if (typeof limit !== "number"), to throw a contract error as needed. (We shouldn't have an "optional" argument before a required argument, and errbacks needs to be required.)

Commit brownplt/pyret-lang@1ec7a95 adds a test for when toRepeatingDecimal() is called with errbacks as a 3rd rather than 4th argument.

For 3rd argument being explicitly +Infinity, please see other comment for why this, rather than undefined, would be a type error.

[blerner]

Oh... Ok, so my problem then was that the docstring comment was misleading, but not inaccurate:

   // An optional limit on the decimal expansion can be provided, in which
   // case the search cuts off if we go past the limit.
   // If this happens, the third argument returned becomes '...' to indicate
   // that the search was prematurely cut off.
   var toRepeatingDecimal = (function() {
     var getResidue = function(r, d, limit, errbacks) {

That proximity sure makes it look like the third argument to this function is "an optional limit"!
It might be helpful just to add a comment above getResidue that explains that limit is determined from options, below. Or better yet, extract getResidue out of this function, since it doesn't need to be closed over within toRepeatingDecimal, and since it's complicated enough that it deserves its own explanation.

[ds26gte]

Oh... Ok, so my problem then was that the docstring comment was misleading, but not inaccurate:

   // An optional limit on the decimal expansion can be provided, in which
   // case the search cuts off if we go past the limit.
   // If this happens, the third argument returned becomes '...' to indicate
   // that the search was prematurely cut off.
   var toRepeatingDecimal = (function() {
     var getResidue = function(r, d, limit, errbacks) {

That proximity sure makes it look like the third argument to this function is "an optional limit"! It might be helpful just to add a comment above getResidue that explains that limit is determined from options, below. Or better yet, extract getResidue out of this function, since it doesn't need to be closed over within toRepeatingDecimal, and since it's complicated enough that it deserves its own explanation.

Fixed in commit brownplt/pyret-lang@7b63e0f

• getResidue() definition moved out of toRepeatingDecimal()
• both functions now have better comments
• jsnums-test.js now tests both functions for a too-small limit