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: Negation functions for NonZeroI* #105

Closed
jmillikin opened this issue Sep 17, 2022 · 9 comments
Closed

ACP: Negation functions for NonZeroI* #105

jmillikin opened this issue Sep 17, 2022 · 9 comments
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api

Comments

@jmillikin
Copy link

jmillikin commented Sep 17, 2022

Proposal

Problem statement

Integer primitives have functions such as overflowing_neg() for numeric negation with controlled overflow. These functions are currently missing from the core::num::NonZeroI{8,16,32,64} types.

Motivation, use-cases

Non-zero integers are frequently used to represent error codes from low-level OS APIs, because these APIs use 0 to indicate non-error. Some APIs use signed error codes as a form of namespacing, such as internal/public (FreeBSD) or kernel/userspace (Linux).

For example, the FUSE protocol uses userspace error codes (positive non-zero) but negates them when sending an error frame to the kernel.

// old
let (neg, _) = err.get().overflowing_neg();
let wire_neg = unsafe { NonZeroI32::new_unchecked(neg) };

// new
let (wire_neg, _) = err.overflowing_neg();

Solution sketches

Since the primitive integer types already have *_neg() functions, and negation can't return zero for non-zero inputs, writing wrappers for the non-zero integers is straightforward.

pub const fn is_negative(self) -> bool {
    self.get().is_negative()
}

pub const fn checked_neg(self) -> Option<$Ty> {
    if let Some(result) = self.get().checked_neg() {
        return Some(unsafe { $Ty::new_unchecked(result) });
    }
    None
}

pub const fn overflowing_neg(self) -> ($Ty, bool) {
    let (result, overflow) = self.get().overflowing_neg();
    ((unsafe { $Ty::new_unchecked(result) }), overflow)
}

pub const fn saturating_neg(self) -> $Ty {
    if let Some(result) = self.checked_neg() {
        return result;
    }
    unsafe { $Ty::new_unchecked(<$Int>::MAX) }
}

pub const fn wrapping_neg(self) -> $Ty {
    let result = self.get().wrapping_neg();
    unsafe { $Ty::new_unchecked(result) }
}

Links and related work

rust-lang/rust#89065
rust-lang/rust#84186

What happens now?

This issue is part of the libs-api team API change proposal process. Once this issue is filed the libs-api team will review open proposals in its weekly meeting. You should receive feedback within a week or two.

@jmillikin jmillikin added api-change-proposal A proposal to add or alter unstable APIs in the standard libraries T-libs-api labels Sep 17, 2022
@jmillikin
Copy link
Author

Proposed implementation: jmillikin/upstream__rust@a807a2d

Docs screenshots:

nonzero-negation-1

nonzero-negation-2

nonzero-negation-3

nonzero-negation-4

nonzero-negation-5

@cuviper
Copy link
Member

cuviper commented Sep 18, 2022

How about trait Neg for the operator?

@jmillikin
Copy link
Author

How about trait Neg for the operator?

It's easy to implement, but my understanding is that trait impls for two stable types can't be marked unstable due to rust-lang/rust#55436.

#[unstable(feature = "nonzero_negation_ops", issue = "none")]
#[rustc_const_unstable(feature = "const_ops", issue = "90080")]
impl const Neg for $Ty {
    type Output = Self;

    #[inline]
    fn neg(self) -> Self {
        unsafe { $Ty::new_unchecked(Neg::neg(self.get())) }
    }    
}
error: an `#[unstable]` annotation here has no effect
   --> library/core/src/num/nonzero.rs:877:13
[...]
    = note: `#[deny(ineffective_unstable_trait_impl)]` on by default
    = note: see issue #55436 <https://github.com/rust-lang/rust/issues/55436> for more information

@thomcc
Copy link
Member

thomcc commented Sep 18, 2022

That's not a deal-breaker if the trait implementation is sensible/desirable.

@jmillikin
Copy link
Author

jmillikin commented Sep 18, 2022

I agree that Neg would be useful, have no strong opinion on the exact details of the stabilization process, and would be happy to do whatever the Rust libs team recommends in this case.

@scottmcm
Copy link
Member

Given that the signed nonzero types already have things like checked_abs and overflowing_abs and saturating_mul and such, adding things like checked_neg and saturating_neg seem well covered by existing precedent, so if you want to send a PR for the methods as unstable, you can r? @scottmcm on that and I'll approve it. (They probably just weren't done originally because they don't make sense on the unsigned versions.)

Trait impls can't go in unstable, though, so that at least should be a different PR, and I'll leave it for a real libs-api member to comment on it.

@jmillikin
Copy link
Author

OK, I've filed PR rust-lang/rust#102341 to implement Neg for NonZeroI*.

To avoid merge conflicts, I'll send a second PR for the negation functions once that first one lands.

@scottmcm
Copy link
Member

Traits take at least 10 days for FCP, so that PR will take much longer to land than one for unstable inherent methods.

@jmillikin
Copy link
Author

Traits take at least 10 days for FCP, so that PR will take much longer to land than one for unstable inherent methods.

Interesting. OK, filed rust-lang/rust#102342 to add the new methods. Once one of them merges I'll rebase the second one.

notriddle added a commit to notriddle/rust that referenced this issue Sep 29, 2022
…tmcm

Add negation methods for signed non-zero integers.

Performing negation with defined wrapping semantics (such as `wrapping_neg()`) on a non-zero integer currently requires unpacking to a primitive and re-wrapping. Since negation of non-zero signed integers always produces a non-zero result, it is safe to implement the various `*_neg()` methods for `NonZeroI{N}`.

I'm not sure what to do about the `#[unstable(..., issue = "none")]` here -- should I file a tracking issue, or is that handled by the Rust dev team?

ACP: rust-lang/libs-team#105
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Sep 29, 2022
…tmcm

Add negation methods for signed non-zero integers.

Performing negation with defined wrapping semantics (such as `wrapping_neg()`) on a non-zero integer currently requires unpacking to a primitive and re-wrapping. Since negation of non-zero signed integers always produces a non-zero result, it is safe to implement the various `*_neg()` methods for `NonZeroI{N}`.

I'm not sure what to do about the `#[unstable(..., issue = "none")]` here -- should I file a tracking issue, or is that handled by the Rust dev team?

ACP: rust-lang/libs-team#105
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Feb 10, 2023
Add negation methods for signed non-zero integers.

Performing negation with defined wrapping semantics (such as `wrapping_neg()`) on a non-zero integer currently requires unpacking to a primitive and re-wrapping. Since negation of non-zero signed integers always produces a non-zero result, it is safe to implement the various `*_neg()` methods for `NonZeroI{N}`.

I'm not sure what to do about the `#[unstable(..., issue = "none")]` here -- should I file a tracking issue, or is that handled by the Rust dev team?

ACP: rust-lang/libs-team#105
JohnTitor added a commit to JohnTitor/rust that referenced this issue Apr 20, 2023
Implement `Neg` for signed non-zero integers.

Negating a non-zero integer currently requires unpacking to a primitive and re-wrapping. Since negation of non-zero signed integers always produces a non-zero result, it is safe to implement `Neg` for `NonZeroI{N}`.

The new `impl` is marked as stable because trait impls for two stable types can't be marked unstable.

See discussion on rust-lang/libs-team#105 for additional context.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue May 4, 2023
…tolnay

Add `is_positive` method for signed non-zero integers.

ACP: rust-lang/libs-team#105
bors added a commit to rust-lang-ci/rust that referenced this issue May 16, 2023
RalfJung pushed a commit to RalfJung/miri that referenced this issue May 16, 2023
Stabilize feature `nonzero_negation_ops`

Fixes #102443

ACP: rust-lang/libs-team#105
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Jul 18, 2023
Add `is_positive` method for signed non-zero integers.

ACP: rust-lang/libs-team#105
thomcc pushed a commit to tcdi/postgrestd that referenced this issue Jul 18, 2023
Stabilize feature `nonzero_negation_ops`

Fixes #102443

ACP: rust-lang/libs-team#105
@dtolnay dtolnay added the ACP-accepted API Change Proposal is accepted (seconded with no objections) label Nov 23, 2023
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 20, 2024
Stabilize feature `nonzero_negation_ops`

Fixes #102443

ACP: rust-lang/libs-team#105
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
Stabilize feature `nonzero_negation_ops`

Fixes #102443

ACP: rust-lang/libs-team#105
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ACP-accepted API Change Proposal is accepted (seconded with no objections) 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