Skip to content

Commit

Permalink
Div, mod, and floor div overhaul (nushell#14157)
Browse files Browse the repository at this point in the history
# Description
Dividing two ints can currently return either an int or a float. Not
having a single return type for an operation between two types seems
problematic. Additionally, the type signature for division says that
dividing two ints returns only an int which does not match the current
implementation (it can also return a float). This PR changes division
between almost all types to return a float (except for `filesize /
number` or `duration / number`, since there are no float representations
for these types).

Currently, floor division between certain types is not implemented even
though the type signature allows it. Also, the current implementation of
floor division uses a combination of clamping and flooring rather than
simply performing floor division which this PR fixes. Additionally, the
signature was changed so that `int // float`, `float // int`, and `float
// float` now return float instead of int. This matches the automatic
float promotion in the rest of the operators (as well as how Python does
floor division which I think is the original inspiration).

Since regular division has always returned fractional values (and now
returns a float to reflect that), `mod` is now defined in terms of floor
division. That is, `D // d = q`, `D mod d = r`, and `D = d * q + r `.
This is just like the `%` operator in Python, which is also based off
floor division (at least for ints and floats). Additionally,
implementations missing from `mod`'s current type signature have been
added (`duration mod int` and `duration mod float`).

This PR also overhauls the overflow checking and errors for div, mod,
and floor div. If an operation overflows, it will now cause an error.

# User-Facing Changes
- Div now returns a float in most cases.
- Floor division now actually does floor division.
- Floor division now does automatic float promotion, returning a float
in more instances.
- Floor division now actually allows division with filesize and
durations as its type signature claimed.
- Mod is now defined and implemented in terms of floor division rather
than truncating division.
- Mod now actually allows filesize and durations as its type signature
claimed.
- Div, mod, and floor div now all have proper overflow checks.

## Examples

When the divisor and the dividend have the same sign, the quotient and
remainder will be the same as before. (Except that this PR will give
more accurate results, since it does not do an intermediate float
conversion). If the signs of the divisor and dividend are different,
then the results will be different, or rather actually correct.

Before:

```nu
let q = 8 // -3 # -3
let r = 8 mod -3 # 2
8 == $q * -3 + $r # false
```

After:

```nu
let q = 8 // -3 # -3
let r = 8 mod -3 # -1
8 == $q * -3 + $r # true
```


Before:

```nu
let q = -8 // 3 # -3
let r = -8 mod 3 # -2
-8 == $q * 3 + $r # false
```

After:

```nu
let q = -8 // 3 # -3
let r = -8 mod 3 # 1
-8 == $q * 3 + $r # true
```

# Tests + Formatting
Added a few tests.

# After Submitting
Probably update the docs.
  • Loading branch information
IanManske authored Nov 4, 2024
1 parent 20c2de9 commit 9f09930
Show file tree
Hide file tree
Showing 4 changed files with 373 additions and 181 deletions.
36 changes: 36 additions & 0 deletions crates/nu-command/tests/commands/math/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,42 @@ fn modulo() {
assert_eq!(actual.out, "1");
}

#[test]
fn floor_div_mod() {
let actual = nu!("let q = 8 // -3; let r = 8 mod -3; 8 == $q * -3 + $r");
assert_eq!(actual.out, "true");

let actual = nu!("let q = -8 // 3; let r = -8 mod 3; -8 == $q * 3 + $r");
assert_eq!(actual.out, "true");
}

#[test]
fn floor_div_mod_overflow() {
let actual = nu!(format!("{} // -1", i64::MIN));
assert!(actual.err.contains("overflow"));

let actual = nu!(format!("{} mod -1", i64::MIN));
assert!(actual.err.contains("overflow"));
}

#[test]
fn floor_div_mod_zero() {
let actual = nu!("1 // 0");
assert!(actual.err.contains("zero"));

let actual = nu!("1 mod 0");
assert!(actual.err.contains("zero"));
}

#[test]
fn floor_div_mod_large_num() {
let actual = nu!(format!("{} // {}", i64::MAX, i64::MAX / 2));
assert_eq!(actual.out, "2");

let actual = nu!(format!("{} mod {}", i64::MAX, i64::MAX / 2));
assert_eq!(actual.out, "1");
}

#[test]
fn unit_multiplication_math() {
let actual = nu!(pipeline(
Expand Down
18 changes: 9 additions & 9 deletions crates/nu-parser/src/type_check.rs
Original file line number Diff line number Diff line change
Expand Up @@ -280,15 +280,15 @@ pub fn math_result_type(
}
},
Operator::Math(Math::Divide) => match (&lhs.ty, &rhs.ty) {
(Type::Int, Type::Int) => (Type::Int, None),
(Type::Int, Type::Int) => (Type::Float, None),
(Type::Float, Type::Int) => (Type::Float, None),
(Type::Int, Type::Float) => (Type::Float, None),
(Type::Float, Type::Float) => (Type::Float, None),
(Type::Number, Type::Number) => (Type::Number, None),
(Type::Number, Type::Int) => (Type::Number, None),
(Type::Int, Type::Number) => (Type::Number, None),
(Type::Number, Type::Float) => (Type::Number, None),
(Type::Float, Type::Number) => (Type::Number, None),
(Type::Number, Type::Number) => (Type::Float, None),
(Type::Number, Type::Int) => (Type::Float, None),
(Type::Int, Type::Number) => (Type::Float, None),
(Type::Number, Type::Float) => (Type::Float, None),
(Type::Float, Type::Number) => (Type::Float, None),
(Type::Filesize, Type::Filesize) => (Type::Float, None),
(Type::Filesize, Type::Int) => (Type::Filesize, None),
(Type::Filesize, Type::Float) => (Type::Filesize, None),
Expand Down Expand Up @@ -379,9 +379,9 @@ pub fn math_result_type(
},
Operator::Math(Math::FloorDivision) => match (&lhs.ty, &rhs.ty) {
(Type::Int, Type::Int) => (Type::Int, None),
(Type::Float, Type::Int) => (Type::Int, None),
(Type::Int, Type::Float) => (Type::Int, None),
(Type::Float, Type::Float) => (Type::Int, None),
(Type::Float, Type::Int) => (Type::Float, None),
(Type::Int, Type::Float) => (Type::Float, None),
(Type::Float, Type::Float) => (Type::Float, None),
(Type::Number, Type::Number) => (Type::Number, None),
(Type::Number, Type::Int) => (Type::Number, None),
(Type::Int, Type::Number) => (Type::Number, None),
Expand Down
5 changes: 3 additions & 2 deletions crates/nu-protocol/src/errors/shell_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,13 @@ pub enum ShellError {
/// Check the inputs to the operation and add guards for their sizes.
/// Integers are generally of size i64, floats are generally f64.
#[error("Operator overflow.")]
#[diagnostic(code(nu::shell::operator_overflow), help("{help}"))]
#[diagnostic(code(nu::shell::operator_overflow))]
OperatorOverflow {
msg: String,
#[label = "{msg}"]
span: Span,
help: String,
#[help]
help: Option<String>,
},

/// The pipelined input into a command was not of the expected type. For example, it might
Expand Down
Loading

0 comments on commit 9f09930

Please sign in to comment.