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

OptionRanged[something]'s is_none and is_some methods take ownership of the value #15

Open
babichjacob opened this issue Oct 30, 2024 · 2 comments · May be fixed by #16
Open

OptionRanged[something]'s is_none and is_some methods take ownership of the value #15

babichjacob opened this issue Oct 30, 2024 · 2 comments · May be fixed by #16

Comments

@babichjacob
Copy link

The standard library's Option's is_none and is_some take the value by reference.

I encountered this when using serde:

#[derive(Serialize)]
struct Struct {
    #[serde(skip_serializing_if = "OptionRangedU16::is_none")]
    number: OptionRangedU16<0, 1000>,
}

with the full compilation error:

error[E0308]: mismatched types
    --> src/main.rs:5:10
     |
5    | #[derive(Serialize)]
     |          ^^^^^^^^^ expected `OptionRangedU16<_, _>`, found `&OptionRangedU16<0, 1000>`
6    | struct Struct {
7    |     #[serde(skip_serializing_if = "OptionRangedU16::is_none")]
     |                                   -------------------------- arguments to this function are incorrect
     |
     = note: expected struct `OptionRangedU16<_, _>`
             found reference `&OptionRangedU16<0, 1000>`
note: method defined here
    --> C:\Users\Jacob\scoop\persist\rustup\.cargo\registry\src\index.crates.io-6f17d22bba15001f\deranged-0.3.11\src\lib.rs:1389:1
     |
1389 | / impl_ranged! {
1390 | |     RangedU8 {
1391 | |         mod_name: ranged_u8
1392 | |         internal: u8
...    |
1473 | |     }
1474 | | }
     | |_^
     = note: this error originates in the derive macro `Serialize` which comes from the expansion of the macro `impl_ranged` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0308`.

The relevant part of the macro is here:

deranged/src/lib.rs

Lines 958 to 970 in c25ef2c

/// Returns `true` if the value is the niche value.
#[inline(always)]
pub const fn is_none(self) -> bool {
<$type<MIN, MAX> as $crate::traits::RangeIsValid>::ASSERT;
self.get().is_none()
}
/// Returns `true` if the value is not the niche value.
#[inline(always)]
pub const fn is_some(self) -> bool {
<$type<MIN, MAX> as $crate::traits::RangeIsValid>::ASSERT;
self.get().is_some()
}

where changing the self parameter to &self would be trivial, but would be a breaking change.

Working around this on my end without a change to this crate is easy enough.

Thanks for your work on deranged, it's a great crate!

@jhpratt
Copy link
Owner

jhpratt commented Oct 30, 2024

I almost certainly assumed it didn't matter because the type is Copy but failed to consider that the function signature itself matters in some contexts.

I can definitely change this. The library isn't widely used (I honestly thought I was one of the only people using it), so having a semver breaking release is essentially a non-issue.

@babichjacob
Copy link
Author

It's a great crate! Reflecting the constraints of a number's range in its type when e.g. designing a library for interacting with an API (e.g. a percent is 0 to 100, a color hue is 0 to 360) brings us self-documentation and validation for free (because you and the other handful of contributors did it for us :) ).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants