NonZero start

[max-sixty]

I started attempting to implement NonZero from #558

But I was tripping over from the get-go - am I going in the right direction here?

As well as the errors I'd expect to see (e.g. from direct struct construction), I'm also seeing errors like these:

error[E0277]: the trait bound `isize: std::convert::From<i32>` is not satisfied
   --> src/dimension/mod.rs:886:38
    |
886 |         assert_eq!(slice_min_max(10, Slice::new(-9, None, 3)), Some((1, 7)));
    |                                      ^^^^^^^^^^ the trait `std::convert::From<i32>` is not implemented for `isize`
    |
    = help: the following implementations were found:
              <isize as std::convert::From<bool>>
              <isize as std::convert::From<i16>>
              <isize as std::convert::From<i8>>
              <isize as std::convert::From<std::num::NonZeroIsize>>
              <isize as std::convert::From<u8>>
    = note: required because of the requirements on the impl of `std::convert::Into<isize>` for `i32`
note: required by `slice::Slice::new`
   --> src/slice.rs:51:5
    |
51  | /     pub fn new<S>(start: isize, end: Option<isize>, step: S) -> Slice
52  | |     where S: Into<isize> + Debug + PartialEq
53  | |     {
54  | |         debug_assert_ne!(step, 0, "Slice::new: step must be nonzero");
55  | |         let step = NonZeroIsize::new(step).unwrap();
56  | |         Slice { start, end, step }
57  | |     }
    | |_____^

Any guidance would be appreciated!

[jturner314]

The particular error provided in your comment is a result of something like the following (based on my understanding of the compiler's type inference):

1. The compiler sees a call to Slice::new with an integer literal for step and needs to infer the type of that literal.
2. It looks at the signature of Slice::new, but sees that the step parameter is generic and multiple types that could be expressed as an integer literal implement the necessary trait (i8, i16, u8), so it can't narrow the options down to a single valid type.
3. Normally, since no single type is valid, the compiler would give an error indicating that the type is ambiguous. However, since it's trying to determine the type of an integer literal, it falls back to the default of treating it as i32. (Integer and floating-point literals are handled specially in type inference. In the case of ambiguity, integer literals fall-back to i32 and floating-point literals fall-back to f64. This fall-back is for ergonomics so that stuff like for i in 0..10 { println!("{}", i); } works without type annotations.)
4. Now that the compiler has decided that the integer literal has type i32, it gives an error message because i32 doesn't implement Into<isize> (since isize doesn't implement From<i32>), which is required by Slice::new.

To fix this error at the call site, you can specify the type of the step with e.g. Slice::new(-9, None, 3isize). Alternatively, to make the Slice::new function more friendly so that the existing Slice::new(-9, None, 3) works, you can change the step parameter back to step: isize. Alternatively, if you want to expose the nonzero constraint in the type of the public API, you could make the step parameter be step: NonZeroIsize, and then the call would become Slice::new(-9, None, NonZeroIsize::new(3).unwrap()).

My recommendation would be to initially leave the public API as-is (e.g. with step: isize) when doing so is convenient and focus on changing the internal implementation to use the NonZero types. (The NonZero types should help with correctness in the internal implementation. I'd only worry about exposing them in the public API once the internal implementation is done or if doing so makes it more convenient to update the internal implementation.)

[max-sixty]

I've made an effort at an MVP - let me know your thoughts.

It does change a pub struct, but not the new implementation.

Beyond that, there aren't that many places to use NonZero that aren't public. In my very limited experience, it's also not that ergonomic, so I wouldn't suggest rushing to add it to public APIs

[max-sixty]

Tests pass, except for #676

[max-sixty]

Actually I don't think this should be removed yet; I'll add back post-review

[bluss]

Hi, a formality, I've lifted up and bolded my instruction from the new rust features issue:

All features to be used because of what gain they give us, not to score points. Formulate goal & gain before implementing - and write it up the pull request 🙂.

In short it's not a laundry list, and we only use the features if they gain us something. The pull request should formulate what the goal and the gain of the change is. Hope that makes sense.

(If I should continue to rant as a person with maintainer experience, I hope goal and gain is a part of every pull request's description! For me, code doesn't speak for itself, and everyone has different places they are coming from.)

[max-sixty]

Closing as stale; thanks for the feedback @jturner314 & @bluss last year