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

usize and isize should be typed as bigint on 64-bit architectures #311

Open
lucasvanmol opened this issue Apr 19, 2024 · 6 comments · May be fixed by #312
Open

usize and isize should be typed as bigint on 64-bit architectures #311

lucasvanmol opened this issue Apr 19, 2024 · 6 comments · May be fixed by #312
Labels
breaking change This PR will change the public API in some way bug Something isn't working

Comments

@lucasvanmol
Copy link

This would align with how, for example, u32 and u64 are currently typed.

@lucasvanmol lucasvanmol added the bug Something isn't working label Apr 19, 2024
@escritorio-gustavo
Copy link
Contributor

This can be implemented with the following code

#[cfg(target_pointer_width = "16")]
impl_primitives! {
    usize, isize, NonZeroUsize, NonZeroIsize => "number"
}

#[cfg(target_pointer_width = "32")]
impl_primitives! {
    usize, isize, NonZeroUsize, NonZeroIsize => "number"
}

#[cfg(target_pointer_width = "64")]
impl_primitives! {
    usize, isize, NonZeroUsize, NonZeroIsize => "bigint"
}

But people seem to dislike the use of bigint in the first place (#94)

What do you think @NyxCode?

@escritorio-gustavo
Copy link
Contributor

escritorio-gustavo commented Apr 19, 2024

Alternatively, we could make a manual impl that returns

// 6 bytes is the most that can be guaranteed to fit a JS Number
if std::mem::size_of::<usize>() <= 6 {
    "number"
} else {
    "bigint"
}.to_owned()

This would handle any weird architechture (I'm decently sure there's nothing with 5 byte pointers, but who knows)

@escritorio-gustavo escritorio-gustavo added the breaking change This PR will change the public API in some way label Apr 19, 2024
@escritorio-gustavo
Copy link
Contributor

people seem to dislike the use of bigint in the first place

I do agree that this should be the default though. As #94 (comment) states, "soundness should not be an opt-in", users coercing i/u64, i/u128, i/usize to number should do so on purpose and at their own risk

@lucasvanmol
Copy link
Author

Agreed. The reason I had this issue come was that my rust backend was sending random usizes to the frontend, and they weren't getting parsed correctly because of the limitations with JS numbers.

@escritorio-gustavo
Copy link
Contributor

escritorio-gustavo commented Apr 22, 2024

I see, but beware that even if this gets implemented, you still need to customize your (de)serialization logic to produce bigints for JS, as just changing the type definition in TS doesn't actually fix the issue. In the meantime, you can use #[ts(type = "bigint")] or #[ts(as = "u64")] (I'd prefer as since it prevents typos by actually parsing and using the type you give it, while type just takes in a string and doesn't check it) as a temporary fix

@escritorio-gustavo
Copy link
Contributor

In particular, if you send data to your frontend as JSON, which doesn't support bigint, you'd need to encode your numbers as strings or arrays of bytes, which you'd then need to deserialize into a bigint in your frontend code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This PR will change the public API in some way bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants