Allow values larger than 32 bit in int! and uint! macros

[FSMaxB]

This changes the macros to:

1. Allow arbitrary const expressions, not just literals
2. Support 64 bit values instead of just 32 bit like the current macros

This comes at the expense of an increased MSRV since inline const expressions were introduced in 1.79. It also removes support for explicit 32 bit literals, like int!(1_i32), which was previously supported.

I was considering a different approach that would have kept support for 32 bit literals. The approach would have been to only put an assertion about the range in the inline const expression but have the macro call .into() on the expression. The reason I decided against it is because it would mean that the macros themselves won't be const.

[FSMaxB]

Had to replace .expect() with a match and panic because Option::expect wasn't const in 1.79 yet.

[jplatte]

Thanks! Just a small nitpick about the changelog, otherwise I'm happy to merge this and release it as js_int 0.3.0.

[jplatte]

Suggested change

	This means that explicit `_i32` and `_u32` literals are no longer valid and it also raises the MSRV to 1.79.
	This means that `i32`-suffixed and `u32`-suffixed literals are no longer accepted

Can you make the MSRV thing a separate bullet point?

[jplatte]

Actually, I wonder.. Since the const block is in a macro, shouldn't the MSRV be unaffected? It's still a breaking change, but the crate can still be built by older versions of rustc, right?

[FSMaxB]

I updated the changelog.

Actually, I wonder.. Since the const block is in a macro, shouldn't the MSRV be unaffected? It's still a breaking change, but the crate can still be built by older versions of rustc, right?

I just checked and yes apparently compiling the crate with the updated macros doesn't reqire 1.79 with inline const expression support, as long as the macros aren't instantiated. This would mean changing the implementations of the saturating_add and saturating_sub methods to not use the macro anymore. The remaining problem I ran into with 1.35 (the current MSRV) though was not being able to turn the new methods into const fn's, see:

error[E0723]: `if`, `match`, `&&` and `||` are not stable in const fn (see issue #57563)
  --> src/int.rs:73:12
   |
73 |         if val >= MIN_SAFE_INT && val <= MAX_SAFE_INT {
   |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = help: add #![feature(const_fn)] to the crate attributes to enable

error[E0723]: `if`, `match`, `&&` and `||` are not stable in const fn (see issue #57563)
  --> src/uint.rs:70:9
   |
70 | /         if val <= MAX_SAFE_UINT {
71 | |             Some(Self(val))
72 | |         } else {
73 | |             None
74 | |         }
   | |_________^
   |
   = help: add #![feature(const_fn)] to the crate attributes to enable

I'm sure there's some version between 1.35 and 1.79 that would be a new MSRV when not invoking the macros, but not entirely sure which version that is. I can try and find out if you think it's worth the effort.

[jplatte]

Looks to be 1.46: https://releases.rs/docs/1.46.0/
Let's use that if possible. js_int is a basic enough crate that I would prefer a lower MSRV.

[FSMaxB]

I've lowered it to 1.46 providing an alternative to the macros in the changelog and explaining that 1.79 is required for using the macros!

[FSMaxB]

🤔 Cargo 1.46 didn't yet have MSRV dependency resolution, meaning it breaks with the serde feature since it transitively depends on proc-macro2 which is editition 2021.

[FSMaxB]

Maybe if I get rid of the derive feature of serde 😅?

[jplatte]

You can selectively downgrade things using cargo update -p <pkg> --precise <version> until it compiles on 1.46, then commit the lockfile.

[jplatte]

Idea: Could the macros work on the older version if they used a block-scoped named const, instead of an inline const block?

I suppose this would prohibit using const generic args and such in the argument 🤔

[FSMaxB]

Idea: Could the macros work on the older version if they used a block-scoped named const, instead of an inline const block?

How would that help building the compile time assertion that the number is in range?

[FSMaxB]

Ok, I now did both:

1. Remove the derive feature from serde and implement Serialize manually
2. Commit the lockfile just for future proofing

[FSMaxB]

Note that proc-macro2 still shows up in the lockfile because of serde-test, but it's not getting built if you don't run the tests, so it's still fine on 1.46.

[jplatte]

Idea: Could the macros work on the older version if they used a block-scoped named const, instead of an inline const block?

How would that help building the compile time assertion that the number is in range?

The same way the current const {} works.. To be clear, I was talking about expanding to

{
    const _VALUE: Int = match Int::new($expr) { ... };
    _VALUE
}

instead of const { match Int::new($expr) { ... } }. It has some downsides though.

W.r.t. Serialize, I didn't even remember we derived it, and without #[serde(transparent)]!
I would consider that a bug.
I'm also thinking that those extra deserialize features (float_deserialize, lax_deserialize) should be removed, as changing runtime behavior based on Cargo features is generally a recipe for nasty bugs, and these haven't been used in Ruma for a long time. Do you use one of them?

[FSMaxB]

instead of const { match Int::new($expr) { ... } }. It has some downsides though.

Ah yeah, right. That should probably work. For my use-case the const generics issues isn't a problem, but could be for others.

W.r.t. Serialize, I didn't even remember we derived it, and without #[serde(transparent)]!
I would consider that a bug.

It works like transparent for serde_json at least 😅. But yeah, can be fixed easily. I also found it weird that there was no #[serde(transparent)] but I explicitly went through the output of cargo expand to be sure I'm replicating the current behavior correctly 😆.

I'm also thinking that those extra deserialize features (float_deserialize, lax_deserialize) should be removed, as changing runtime behavior based on Cargo features is generally a recipe for nasty bugs, and these haven't been used in Ruma for a long time. Do you use one of them?

We are using float_deserialize in communityvi, but don't remember whether we actually need it. But my understanding is that deserialization will fail with number that have decimal separators in them if you leave it out? It then comes down to how browsers actually serialize integers to JSON.

[jplatte]

We are using float_deserialize in communityvi, but don't remember whether we actually need it. But my understanding is that deserialization will fail with number that have decimal separators in them if you leave it out? It then comes down to how browsers actually serialize integers to JSON.

Yeah, exactly. I'll post a PR to remove the lax feature, as a start.