I'm begging forgiveness in advance as this is kind of the inverse of #821, but I have run into this too many times that I feel the need to file an issue.
In #821 it was decided to provide an impl for ToSql/FromSql for u64 (and with it, usize). I'm onboard with the general decision in principal, as u64 is a common enough type. But the actual implementation ended up throwing an IntegralValueOutOfRange error if the provided u64 does not fit in the "natural" range of i64, ostensibly due to SQLite NUMERIC/INTEGER limitations.
I'm going to start by talking about usize: it is very normal to use usize to represent sizes (file sizes, lengths in memory, etc) that may never actually come close to availing themselves of usize's actual range, the rust standard library returns usize values for a wide range of operations, the only alternative to usize is isize which is similarly sized but signed, and so it makes sense to support it, possibly even within the confines of 0..i64::MAX.
But I don't feel that this same rationale/logic holds true for u64, which is a much more deliberately chosen datatype. When an api deliberately chooses to return u64 (and not u32 or usize), it is almost certainly because it actually needs all the bits (because it is known that the api will be returning large numbers that exceed the bounds of u32 or/also oftentimes because the u64 is actually representing 64-bits of state rather than a "counting" number that exceeds the range of u32, with the most common example being 64-bit hash values.
I think the above is fairly objective, but here begins the subjective bit: I feel like having a library/api that looks and acts like it supports u64 but then begins throwing errors later (possibly even in production) is not great. If u64 weren't supported out of the box, it would be very clear to the user that they need to either perform the fallible i64::try_from(u64_val) operation to handle only values that fit within 0..i64::MAX or else use u64_value as i64 to support losslessly round-tripping the full range of u64 values, at the cost of their being (possibly) represented differently in the database.
I can obviously still do this, but now there is a gotcha: first, I need to be aware of the fact that the default implementation can "randomly" fail, and second, I need to remember to always convert when reading to/from the database - which is easier said than done, because normally for types that need to be custom-marshalled to/from the db I can either implement my own ToSql/FromSql (for local types) or I will at least be presented with a compilation error telling me that I forgot to convert an unsupported type when I try to pass it in via params! (or whatever) or retrieve it with .get().
As a concrete example of preferring no implementation, I normally want to store chrono timestamps as posix milliseconds instead of as strings (the default serialization behavior when rusqlite's chrono feature is enabled). So despite using both chrono and rusqlite and actively storing chrono types in the database, I explicitly choose not to enable the rusqlite chrono feature so that I don't inadvertently forget to convert timestamps to utc millis since posix epoch when reading/writing from/to the database, because if I forget in even one spot, the database will essentially be corrupted with a mix of strings and integers in the corresponding column (assuming strict types are disabled, the default).
To me, this same logic extends to u64. The current library design mandates that I both explicitly convert to/from i64 when reading/writing from/to the database, and that I never forget to do so on my own, because the compiler will happily let me fall back on the (undesirable) default behavior, which will not only compile but also quite possibly run and work for some time until an actually out-of-range u64 value comes along.
I hope the above is a decent explanation for why the current support for u64 (and, to some extent but not as problematically so, usize) is problematic. Now for suggestions on how to handle this, all merely thoughts and not necessarily full fleshed out or checked for other issues:
- Put the current
u64 (and maybe usize?) impl behind a feature, similar to i128, documenting how it works and how it fails (question: enabled by default? my preference for strict correctness would say no, but it would still be an improvement either way)
- Change the current
u64 (and maybe usize?) impls to (infallibly) cast to i64 rather than (fallibly) truncate to i64, documenting the behavior (possible con: compatibility with other apps reading the same db?)
- A combination of the two, each behind a feature (e.g.
u64_truncate and u64_cast, where the name of the feature makes it clear what is happening)?
- (ADDED) Just switch to casting instead of fallible conversions
I'm begging forgiveness in advance as this is kind of the inverse of #821, but I have run into this too many times that I feel the need to file an issue.
In #821 it was decided to provide an impl for
ToSql/FromSqlforu64(and with it,usize). I'm onboard with the general decision in principal, asu64is a common enough type. But the actual implementation ended up throwing anIntegralValueOutOfRangeerror if the provided u64 does not fit in the "natural" range ofi64, ostensibly due to SQLiteNUMERIC/INTEGERlimitations.I'm going to start by talking about
usize: it is very normal to useusizeto represent sizes (file sizes, lengths in memory, etc) that may never actually come close to availing themselves ofusize's actual range, the rust standard library returnsusizevalues for a wide range of operations, the only alternative tousizeisisizewhich is similarly sized but signed, and so it makes sense to support it, possibly even within the confines of0..i64::MAX.But I don't feel that this same rationale/logic holds true for
u64, which is a much more deliberately chosen datatype. When an api deliberately chooses to returnu64(and notu32orusize), it is almost certainly because it actually needs all the bits (because it is known that the api will be returning large numbers that exceed the bounds ofu32or/also oftentimes because theu64is actually representing 64-bits of state rather than a "counting" number that exceeds the range ofu32, with the most common example being 64-bit hash values.I think the above is fairly objective, but here begins the subjective bit: I feel like having a library/api that looks and acts like it supports
u64but then begins throwing errors later (possibly even in production) is not great. Ifu64weren't supported out of the box, it would be very clear to the user that they need to either perform the falliblei64::try_from(u64_val)operation to handle only values that fit within0..i64::MAXor else useu64_value as i64to support losslessly round-tripping the full range of u64 values, at the cost of their being (possibly) represented differently in the database.I can obviously still do this, but now there is a gotcha: first, I need to be aware of the fact that the default implementation can "randomly" fail, and second, I need to remember to always convert when reading to/from the database - which is easier said than done, because normally for types that need to be custom-marshalled to/from the db I can either implement my own
ToSql/FromSql(for local types) or I will at least be presented with a compilation error telling me that I forgot to convert an unsupported type when I try to pass it in viaparams!(or whatever) or retrieve it with.get().As a concrete example of preferring no implementation, I normally want to store
chronotimestamps as posix milliseconds instead of as strings (the default serialization behavior when rusqlite'schronofeature is enabled). So despite using bothchronoandrusqliteand actively storingchronotypes in the database, I explicitly choose not to enable the rusqlitechronofeature so that I don't inadvertently forget to convert timestamps to utc millis since posix epoch when reading/writing from/to the database, because if I forget in even one spot, the database will essentially be corrupted with a mix of strings and integers in the corresponding column (assuming strict types are disabled, the default).To me, this same logic extends to
u64. The current library design mandates that I both explicitly convert to/fromi64when reading/writing from/to the database, and that I never forget to do so on my own, because the compiler will happily let me fall back on the (undesirable) default behavior, which will not only compile but also quite possibly run and work for some time until an actually out-of-rangeu64value comes along.I hope the above is a decent explanation for why the current support for
u64(and, to some extent but not as problematically so,usize) is problematic. Now for suggestions on how to handle this, all merely thoughts and not necessarily full fleshed out or checked for other issues:u64(and maybeusize?) impl behind a feature, similar toi128, documenting how it works and how it fails (question: enabled by default? my preference for strict correctness would say no, but it would still be an improvement either way)u64(and maybeusize?) impls to (infallibly) cast toi64rather than (fallibly) truncate toi64, documenting the behavior (possible con: compatibility with other apps reading the same db?)u64_truncateandu64_cast, where the name of the feature makes it clear what is happening)?