Skip to content

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

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
wants to merge 1 commit into
base: horizon
Choose a base branch
from

Conversation

ds26gte
Copy link
Contributor

@ds26gte ds26gte commented Mar 25, 2025

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

@schanzer schanzer requested a review from blerner March 25, 2025 19:19
@blerner
Copy link
Member

blerner commented Mar 25, 2025

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
Copy link

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

@jpolitz
Copy link
Member

jpolitz commented Apr 10, 2025

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:

image

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:

image

@jpolitz
Copy link
Member

jpolitz commented Apr 10, 2025

@jpolitz
Copy link
Member

jpolitz commented Apr 10, 2025

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
Copy link
Member

blerner commented Apr 10, 2025

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
Copy link
Member

jpolitz commented Apr 10, 2025

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

image

@blerner
Copy link
Member

blerner commented Apr 10, 2025

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,

image

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
Copy link
Member

jpolitz commented Apr 10, 2025

What browser are you on?

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

image

@blerner
Copy link
Member

blerner commented Apr 10, 2025

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

@jpolitz
Copy link
Member

jpolitz commented Apr 10, 2025

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
Copy link
Member

jpolitz commented Apr 10, 2025

@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.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants