rust-lang / libs-team

The home of the library team
Apache License 2.0
110 stars 18 forks source link

ACP: `Sign` enum for indicating sign of a number #377

Closed clarfonthey closed 1 month ago

clarfonthey commented 2 months ago

Proposal

Problem statement

The standard library offers Ordering as a simple enum of -1, 0, or 1, but its meaning is limited to ordering specifically, and isn't suitable for many other uses. Additionally, the presence of zero might be undesired in some cases, since for example, floating-point numbers always have a positive or negative sign.

Motivating examples or use cases

A lot of APIs could benefit from an enum specifically dedicated to indicating a positive or negative sign, like the currently unstable ln_gamma function (rust-lang/rust#99842) and the various signum methods offered by numbers which could return a sign instead.

Additionally, many APIs outside the standard library could benefit from a standardised "sign" type as a simple way of ensuring that an input is specifically limited to the values ±1.

Solution sketch

#[repr(i8)]
#[derive(Copy, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
enum Sign {
    Minus = -1,
    Plus = 1,
}
impl Sign {
     // iN is a placeholder for all signed integer types
    pub fn as_iN(self) -> iN {
        self as iN
    }
    pub fn as_nonzero_iN(self) -> NonZeroIN {
        NonZeroIN::new(self.as_iN()).unwrap()
    }

    // fN is a placeholder for all floating-point types
    pub fn as_fN(self) -> fN {
        self as i8 as fN
    }
}

// * is a placeholder for all formatting traits implemented by integers
impl fmt::* for Sign {
    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
        self.as_i8().fmt(f)
    }
}

// fN is a placeholder for all floating point types
impl fN {
    // note: this is done to mirror the signum function, but technically, a sign could be included for NaN regardless
    pub fn sign(self) -> Option<Sign> {
        if self.is_nan() {
            None
        } else {
            Some(if self.is_sign_positive() { Sign::Plus } else { Sign::Minus })
        }
    }

    // modification of existing unstable API
    pub fn ln_gamma(self) -> (fN, Sign) { /* ... */ }
}

// iN is a placeholder for all signed integer types
impl iN {
    pub fn sign(self) -> Option<Sign> {
        if self == 0 {
            None
        } else {
            Some(if self > 0 { Sign::Plus } else { Sign::Minus })
        }
    }
}
impl NonZeroIN {
    pub fn sign(self) -> Sign {
        if self > 0 { Sign::Plus } else { Sign::Minus }
    }
}
impl Mul<iN> for Sign {
    type Output = iN;
    fn mul(self, rhs: iN) -> iN {
        self.as_iN() * rhs
    }
}
impl Mul<Sign> for iN {
    type Output = iN;
    fn mul(self, rhs: Sign) -> iN {
        self * rhs.as_iN()
    }
}
impl Mul<NonZeroIN> for Sign {
    type Output = NonZeroIN;
    fn mul(self, rhs: NonZeroIN) -> NonZeroIN {
        self.as_nonzero_iN() * rhs
    }
}
impl Mul<Sign> for NonZeroIN {
    type Output = NonZeroIN;
    fn mul(self, rhs: Sign) -> NonZeroIN {
        self * rhs.as_nonzero_iN()
    }
}
impl MulAssign<Sign> for iN {
    fn mul_assign(&mut self, rhs: Sign) {
        self *= rhs.as_iN();
    }
}
impl MulAssign<Sign> for NonZeroIN {
    fn mul_assign(&mut self, rhs: Sign) {
        self *= rhs.as_nonzero_iN();
    }
}

// it's debatable whether these should use copysign or a direct multiplication
impl Mul<fN> for Sign {
    type Output = fN;
    fn mul(self, rhs: fN) -> fN {
        self.as_fN() * rhs
    }
}
impl Mul<Sign> for fN {
    type Output = fN;
    fn mul(self, rhs: Sign) -> fN {
        self * rhs.as_fN()
    }
}
impl MulAssign<Sign> for fN {
    fn mul_assign(&mut self, rhs: Sign) {
        self *= rhs.as_fN();
    }
}

Alternatives

Additional possible extensions:

Links and related work

N/A

What happens now?

This issue contains an API change proposal (or ACP) and is part of the libs-api team feature lifecycle. Once this issue is filed, the libs-api team will review open proposals as capability becomes available. Current response times do not have a clear estimate, but may be up to several months.

Possible responses

The libs team may respond in various different ways. First, the team will consider the problem (this doesn't require any concrete solution or alternatives to have been proposed):

Second, if there's a concrete solution:

kennytm commented 2 months ago
// it's debatable whether these should use copysign or a direct multiplication
impl Mul<Sign> for fN {
    type Output = fN;
    fn mul(self, rhs: Sign) -> fN {
        fN::copysign(self, rhs.as_fN())
    }
}

it will make (Sign::Minus * -5.0) == -5.0 and (Sign::Plus * -7.0) == 7.0 which is very confusing as a multiplication, in particular it differs from (Sign::Minus * -5) == 5. (IMO there shouldn't be any Mul impl at all)

clarfonthey commented 2 months ago

I completely forgot how multiplication worked. Oops.

joshtriplett commented 1 month ago

We reviewed this in today's @rust-lang/libs-api meeting.

There were various opinions on this. Some folks weren't in favor of adding this as a trait. Others were in favor of adding numeric traits in general but not in favor of adding a trait that introduces a new subtly different semantic from the existing signum method.

The combination of those two things led us to reject this ACP.

pitaj commented 1 month ago

@joshtriplett I'm confused because this ACP doesn't seem to propose a new trait at all

kennytm commented 1 month ago

@joshtriplett

Some folks weren't in favor of adding this as a trait.

I'm sorry but did the lib team confuse this ACP with some other proposal? OP suggested adding an enum not a trait.

(I'm neutral to whether accepting this ACP but the rejection rationale keep talking about "trait" is just very confusing)

ChrisDenton commented 1 month ago

I was at the meeting but didn't take notes so the following is just my fallible recollection.

In the meeting some felt that the existence of signum makes it weird if there's also an enum which has slightly different behaviour.

Others felt that this is fairly niche. If you actually need this you're likely already using some numerics crate (or your own lib), which could provide such an enum.

joshtriplett commented 1 month ago

My apologies, I clearly picked up an incorrect detail from the meeting, and had the impression this was proposing a trait to contain the pub fn sign method. (I think some others in the meeting had that impression as well, rather than these being inherent methods.)

Even in the absence of that, though, the remainder of the consensus still holds: we don't want to add a method that seems confusingly similar to the signum method, with subtly different semantics.