Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Generic int64 match does not generate correct literal on JS #24233

Closed
metagn opened this issue Oct 4, 2024 · 0 comments · Fixed by #24234
Closed

Generic int64 match does not generate correct literal on JS #24233

metagn opened this issue Oct 4, 2024 · 0 comments · Fixed by #24234
Assignees

Comments

@metagn
Copy link
Collaborator

metagn commented Oct 4, 2024

Description

Found when trying to remove these workarounds from times

proc foo[T: SomeInteger](a, b: T) =
  let x = a div b

const bar = 123

let x: int64 = 456
foo(x, bar)

bar is passed as a regular integer 123 to foo, when it should be 123n with --jsbigint64:on

Nim Version

presumably anything after #21613

Current Output

(node output)

a.js:357
    return a_p0 / b_p1;
                ^

TypeError: Cannot mix BigInt and other types, use explicit conversions
    at divInt64 (a.js:357:17)
    at foo__a_u8 (a.js:662:23)
    at Object.<anonymous> (a.js:673:1)
    at Module._compile (node:internal/modules/cjs/loader:1469:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1548:10)
    at Module.load (node:internal/modules/cjs/loader:1288:32)
    at Module._load (node:internal/modules/cjs/loader:1104:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:174:12)
    at node:internal/main/run_main_module:28:49

Expected Output

no error

Known Workarounds

No response

Additional Information

No response

@metagn metagn self-assigned this Oct 4, 2024
metagn added a commit to metagn/Nim that referenced this issue Oct 4, 2024
Araq pushed a commit that referenced this issue Oct 7, 2024
fixes #24233

Integer literals with type `int` can match `int64` with a generic match.
Normally this would generate an conversion via `isFromIntLit`, but when
it matches with a generic match (`isGeneric`) the node is left alone and
continues to have type `int` (related to #4858, but separate; since
`isFromIntLit > isGeneric` it doesn't propagate). This did not cause
problems on the C backend up to this point because either the compiler
generated a cast when generating the C code or it was implicitly casted
in the C code itself. On the JS backend however, we need to generate
`int64` and `int` values differently, so we copy the integer literal and
give it the matched type now instead.

This is somewhat risky even if CI passes but it's required to make the
times module work without [this
workaround](https://github.com/nim-lang/Nim/blob/7dfadb8b4e95d09981fbeb01d85b12f23946c3e7/lib/pure/times.nim#L219-L238)
on `--jsbigint64:on` (the default).

CI exposed an issue: When matching an int literal to a generic parameter
in a generic instantiation, the literal is only treated like a value if
it has `int literal` type, but if it has the type `int`, it gets
transformed into literally the type `int` (#12664, #13906), which breaks
the tests t14193 and t12938. To deal with this, we don't give it the
type `int` if we are in a generic instantiation and preserve the `int
literal` type.
Araq pushed a commit that referenced this issue Oct 19, 2024
refs #6978, refs #6752, refs #21613, refs #24234

The `jsNoInt64`, `whenHasBigInt64`, `whenJsNoBigInt64` templates are
replaced with bool constants to use with `when`. Weird that I didn't do
this in the first place.

The `whenJsNoBigInt64` template was also slightly misleading. The first
branch was compiled for both no bigint64 on JS as well as on C/C++. It
seems only `trandom` depended on this by mistake.

The workaround for #6752 added in #6978 to `times` is also removed with
`--jsbigint64:on`, but #24233 was also encountered with this, so this PR
depends on #24234.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant