Pre-RFC: Change abs() to return unsigned integers

[pitdicker]

Summary

Change the function abs() to return unsigned integers, instead of signed integers as it does today.

Motivation

The documentation for std::num::SignedIntStable::abs() states, Int::min_value() will be returned if the number is Int::min_value(). This subtlety can lead to bugs, because a developer may count on abs() to return a positive value. And forcing every caller of abs() to know about this corner case and handle it, is not a very nice design.

If abs() is changed to return an unsigned integer of the same width as its input, there is no special case to handle. Code calling abs() is cleaner because it does not have to handle Int::min_value(), and will lead to fewer bugs.

Because (for example) -127i8 happens to have the same binary representation as 128u8, this is basically a free operation.

Consider how code that calls the current version of abs() can ensure its result is positive:

1. Proof by reasoning about your code that the input of abs() will never be Int::min_value().
2. Do a runtime check before calling abs(), and somehow handle it or return an error.
3. Cast the input to a wider signed integer (if available) before calling abs().
4. Cast the result of abs() to an unsigned integer of the same size.

In all cases this requires extra work for the code calling abs. With this proposal option 4 will be the default, while option 1 to 3 remain possible. The default behaviour will lead to correct (positive) results for all inputs.

Drawbacks

It is not possible anymore to do:

let mut a: i8 = -15;
a = abs(a);

This becomes:

let mut a: i8 = -15;
a = abs(a) as i8; // may overflow

This may actually be an advantage, as now the user code is the cause of the overflow, and not abs().

Alternatives

Keep the status quo, which can lead to subtle bugs or code that is less clean.

---

Basically this is not a new idea, see https://github.com/rust-lang/rust/issues/2353 and http://www.reddit.com/r/rust/comments/2cykyz/abs_for_minimum_int_values/. Is this the proper way to suggest such a change? Would it still be possible before 1.0 (abs is not yet stable)? This is a trivial change, and I have a working sample implementation.

[bwo]

Because (for example) -127i8 happens to have the same binary representation as 128u8

Why would abs(-127i8) return 128u8 instead of 127u8?

[pitdicker]

You are completely right, I meant abs(-128i8) has the same binary representation as 128u8.

[comex]

┈┈┈ ┈┈╌ ┈┈┈:+1: ┈┈╌ ┈┈╌:+1:

[oli-obk]

A small analysis of the usage of abs in the Rust source code:

There are a total of 196 uses of abs. Only 11 are not called on float types. These 11 usages are displayed below.

Doc examples:

//! impl PartialEq for FuzzyNum {
//!     // Our custom eq allows numbers which are near each other to be equal! :D
//!     fn eq(&self, other: &FuzzyNum) -> bool {
//!         (self.num - other.num).abs() < 5
//!     }
//! }

/// let a = [-3, 0, 1, 5, -10];
/// assert_eq!(*a.iter().max_by(|x| x.abs()).unwrap(), -10);

/// let a = [-3, 0, 1, 5, -10];
/// assert_eq!(*a.iter().min_by(|x| x.abs()).unwrap(), 0);

In unit tests:

struct SketchyNum {
    num : int
}
impl PartialEq for SketchyNum {
    // Our custom eq allows numbers which are near each other to be equal! :D
    fn eq(&self, other: &SketchyNum) -> bool {
        (self.num - other.num).abs() < 5
    }
}

let xs: &[int] = &[-3, 0, 1, 5, -10];
assert_eq!(*xs.iter().max_by(|x| x.abs()).unwrap(), -10);

let xs: &[int] = &[-3, 0, 1, 5, -10];
assert_eq!(*xs.iter().min_by(|x| x.abs()).unwrap(), 0);

In the “literal out of range” lint it is already expected to end up as an u64, which means this as u64 would simply be a widening cast

if (negative && v > (min.abs() as u64)) ||
   (!negative && v > (max.abs() as u64)) {
    cx.span_lint(OVERFLOWING_LITERALS,
                 e.span,
                 &*format!("literal out of range for {:?}", t));
    return;
}

Unit tests for abs would be somewhat more complicated as they currently expect the result to be of the same type.

assert!((1 as $T).abs() == 1 as $T);
assert!((0 as $T).abs() == 0 as $T);
assert!((-1 as $T).abs() == 1 as $T);

[pitdicker]

@ker: thanks for the analysis!

On my first try at analysing the rust source, I did not find any useful calls to abs. If I ignore the docs and unit tests (which have controlled input and are easely fixable if nessesary) 3 are left.

FuzzyNum and SketchyNum use abs() to calculate the difference between 2 integers. The way they do it can easely lead to overflows, and improving abs does not do much to help. But I guess that is ok with fuzzy and sketchy numbers :).

The “literal out of range” lint is more interesting, as it uses option number 4, and would be helped by this proposal.

[milesrout]

I think it’s pretty logically clear that abs should return an unsigned integer if given an integer. It is by definition an unsigned value.

[rkjnsn]

After having seen some of the problems this can cause in C, I think the proposal here is definitely the way to go. I’d say post a full RFC and hopefully it can be accepted by Friday.

[pitdicker]

I should have posted here, I have made a pull request with a rfc:

Change abs() to return unsigned integers

[system]

This topic was automatically closed 90 days after the last reply. New replies are no longer allowed.