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

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

Closed
clarfonthey opened this issue May 4, 2024 · 7 comments
Closed

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

clarfonthey opened this issue May 4, 2024 · 7 comments
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@clarfonthey
Copy link

clarfonthey commented May 4, 2024

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

  • A separate type could be added to indicate zero as a sign, or the sign could be updated to include zero. This option is less than ideal because zero can have a sign in the case of floats and may be entirely absent in the case of nonzero integers.
  • Ordering could be used, but this is less than ideal because it's only equivalent in layout, not meaning, and also has the same problem of including zero.
  • Not including this API seems like the primary alternative. It could be argued that this API is not necessary in the standard library, although in order to have sign methods for existing numbers, you'd have to use an extension trait.

Additional possible extensions:

  • Div could be implemented in addition to Mul for Sign, since division by a sign is equivalent to multiplying. This is left out because it would probably just be confusing.
  • Option<Sign> could have methods similar to those on Sign where None is treated as zero, but this seems less than optimal since floats use None to represent NaN, not zero.

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):

  • We think this problem seems worth solving, and the standard library might be the right place to solve it.
  • We think that this probably doesn't belong in the standard library.

Second, if there's a concrete solution:

  • We think this specific solution looks roughly right, approved, you or someone else should implement this. (Further review will still happen on the subsequent implementation PR.)
  • We're not sure this is the right solution, and the alternatives or other materials don't give us enough information to be sure about that. Here are some questions we have that aren't answered, or rough ideas about alternatives we'd want to see discussed.
@clarfonthey clarfonthey added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels May 4, 2024
@clarfonthey clarfonthey changed the title ACP: "Sign" enum for indicating sign of a number ACP: Sign enum for indicating sign of a number May 4, 2024
@kennytm
Copy link
Member

kennytm commented May 4, 2024

// 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
Copy link
Author

I completely forgot how multiplication worked. Oops.

@joshtriplett
Copy link
Member

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
Copy link

pitaj commented May 14, 2024

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

@kennytm
Copy link
Member

kennytm commented May 14, 2024

@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
Copy link

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
Copy link
Member

joshtriplett commented May 14, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api
Projects
None yet
Development

No branches or pull requests

5 participants