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

Cast from decimal to uint8, uint16, uint32, uint, uint64, int, etc. don't follow .NET behaviour #4049

Closed
MangelMaxime opened this issue Feb 17, 2025 · 1 comment · Fixed by #4052
Assignees
Labels

Comments

@MangelMaxime
Copy link
Member

Running this code in .NET will throw exceptions:

uint8 -1m |> printfn "%A"
uint16 -1m |> printfn "%A"
uint32 -1m |> printfn "%A"
uint -1m |> printfn "%A"
uint64 -1m |> printfn "%A"
int -18446744073709551615m |> printfn "%A"

Exceptions looks like Value was either too large or too small for a UInt16.

On Fable, the code succeeds and output:

255
65535
4294967295
4294967295
18446744073709551615
0

I am not sure how to approach fixing this.

My first idea was to apply a fix in toLong function here:

| Number(fromKind, _), Number(toKind, _) ->
let fromMeth = "from" + fromKind.ToString()
Helper.LibCall(com, "BigInt", fromMeth, BigInt.Number, args, ?loc = r)
|> wrapLong com ctx r targetType

But I think this would be to wide because in some scenario .NET allows to do that:

let value = 4294967295UL
let c = uint8 value
printfn "%A" c
// No exception thrown it actually prints: 255uy

I believe we should only fix portion of the code when we call Decimal.op_explicit at:

| "op_Explicit", _ ->
match t with
| Number(kind, _) ->
match kind with
| BigIntegers _ -> toLong com ctx r t args |> Some
| Integers _ -> toInt com ctx r t args |> Some
| Floats _ -> toFloat com ctx r t args |> Some
| Decimal -> toDecimal com ctx r t args |> Some
| _ -> None
| _ -> None

Should we add function like that to Decimal.ts ?

export function op_Explicit_ToChar(d: Decimal) {
  const n = toNumber(d);
  if (n < 0 || n > 65535 || isNaN(n)) {
    throw new Error("Value was either too large or too small for a character.");
  }

  return String.fromCharCode(n);
}

export function op_Explicit_ToUint64(d: Decimal) {
  const n = toUInt64(fromDecimal(d));
  if (n < 0 || n > 18446744073709551615n) {
    throw new Error("Value was either too large or too small for a UInt64.");
  }

  return n;
}

Is it the right approach / place to store them? @ncave @alfonsogarciacaro

@ncave ncave self-assigned this Feb 17, 2025
@ncave
Copy link
Collaborator

ncave commented Feb 17, 2025

@MangelMaxime Thanks, I'll take a look.

ncave added a commit to ncave/Fable that referenced this issue Feb 18, 2025
ncave added a commit that referenced this issue Feb 18, 2025
* Updated fable-library-rust dependencies

* Added decimal conversion tests

* [JS/TS] Fix #4049: decimal/bigint to integer conversion checks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants