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

Consider deprecating Ipv4Network::new_unchecked & Ipv6Network::new_unchecked in favor of safe dittos #202

Closed
MarkusPettersson98 opened this issue Jan 3, 2025 · 0 comments

Comments

@MarkusPettersson98
Copy link
Contributor

MarkusPettersson98 commented Jan 3, 2025

Hello! 👋

I created this issue to highlight that there exist safe ways to implement #185 without losing the intended utility of constructing IpNetwork values in a const context.

Leveraging assert

Since Rust 1.57, panic! and assert! are stable in const contexts.

impl Ipv4Network {
    /// Panics if the prefix is larger than 32.
    pub const fn new_assert(addr: Ipv4Addr, prefix: u8) -> Ipv4Network {
        assert!(prefix <= IPV4_BITS, "The network prefix is larger than 32"); 
        Ipv4Network { addr, prefix })
    }
    ..
}

impl Ipv6Network {
    /// Panics if the prefix is larger than 128.
    pub const fn new_assert(addr: Ipv6Addr, prefix: u8) -> Ipv6Network {
        assert!(prefix <= IPV6_BITS, "The network prefix is larger than 128"); 
        Ipv6Network { addr, prefix })
    }
    ..
}

Leveraging Option

Since Rust 1.83, Option::unwrap is stable in const contexts. The implication of this is that #185 may be implemented in a safe, checked way by encoding the fallibility at the type level.

impl Ipv4Network {
    /// If the prefix is larger than 32 this will return `None`.
    pub const fn new_checked(addr: Ipv4Addr, prefix: u8) -> Option<Ipv4Network> {
        if prefix > IPV4_BITS {
            None
        } else {
            Some(Ipv4Network { addr, prefix })
        }
    }
    ..
}

impl Ipv6Network {
    /// If the prefix is larger than 128 this will return `None`.
    pub const fn new_checked(addr: Ipv6Addr, prefix: u8) -> Option<Ipv6Network> {
        if prefix > IPV6_BITS {
            None
        } else {
            Some(Ipv6Network { addr, prefix })
        }
    }
    ..
}

The consumer of this library may then unwrap the return value from these functions to get a compile time error if they fail to uphold the required prefix invariants.

I think this is better from an API design perspective as the API is impossible to misuse and doesn't perform side effects on its own (such as panicking).

Proposal

I suggest that both Ipv4Network::new_unchecked and Ipv6Network::new_unchecked should be replaced with at least one of the proposed safe variants. Doing so would enforce an MSRV of atleast 1.57 to preserve the utility on const contexts. The easiest time to make this change is now, since #185 has not been included in any ipnetwork release yet and thus removing Ipv4Network::new_unchecked and Ipv6Network::new_unchecked won't be considered a breaking change 😊

Thanks for this crate 💜

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

No branches or pull requests

1 participant