Avoid re-borrowing in multislice! macro

[jturner314]

The old implementation always called .view_mut() on the input array, which meant that it was not possible to use multislice! to split an ArrayViewMut instance into pieces with the same lifetime as the
original view. Now, multislice! uses ArrayViewMut::from() instead so that this works. The primary disadvantage is that an explicit borrow is necessary in many cases, which is less concise.

Closes #687.

[LukeMathWalker]

What is the error message if a user forgets the explicit reborrow?

[LukeMathWalker]

Checked it out with

#[test]
fn test_multislice_intersecting() {
    assert_panics!({
        let mut arr = Array2::<u8>::zeros((8, 6));
        multislice!(arr, mut [3, ..], [3, ..]);
    });

The compiler spits out:

error[E0277]: the trait bound `ndarray::ArrayBase<ndarray::ViewRepr<&mut _>, _>: std::convert::From<ndarray::ArrayBase<ndarray::OwnedRepr<u8>, ndarray::dimension::dim::Dim<[usize; 2]>>>` is not satisfied
   --> tests/array.rs:388:9
    |
388 |         multislice!(arr, mut [3, ..], [3, ..]);
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `std::convert::From<ndarray::ArrayBase<ndarray::OwnedRepr<u8>, ndarray::dimension::dim::Dim<[usize; 2]>>>` is not implemented for `ndarray::ArrayBase<ndarray::ViewRepr<&mut _>, _>`
    |
    = help: the following implementations were found:
              <ndarray::ArrayBase<S, ndarray::dimension::dim::Dim<[usize; 1]>> as std::convert::From<std::vec::Vec<A>>>
              <ndarray::ArrayBase<ndarray::CowRepr<'a, A>, D> as std::convert::From<ndarray::ArrayBase<ndarray::OwnedRepr<A>, D>>>
              <ndarray::ArrayBase<ndarray::CowRepr<'a, A>, D> as std::convert::From<ndarray::ArrayBase<ndarray::ViewRepr<&'a A>, D>>>
              <ndarray::ArrayBase<ndarray::OwnedRepr<A>, ndarray::dimension::dim::Dim<[usize; 2]>> as std::convert::From<std::vec::Vec<V>>>
            and 5 others
    = note: required by `std::convert::From::from`
    = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)

error: aborting due to previous error

It's a bit oscure - anything we could do to make it less so?

[Palladinium]

I think forcing the explicit borrow is potentially a good thing, as it makes the first parameter work as if it was a regular generic function call parameter, with fewer hidden behaviours.

The error message is unfortunate, especially since the span of the error is the whole macro call instead of just the arr parameter. I did some digging around but I couldn't find any way to make it more readable without changing behaviour.

The best I can come up with is to shift the burden further towards the user and remove the implicit from() call, taking an argument of type ArrayViewMut. This means having to call .view_mut(), but the error is more readable when that's not done:

error[E0308]: mismatched types
  --> src/main.rs:10:21
   |
10 |         multislice!(arr, mut [3, ..], [3, ..]);
   |                     ^^^ expected struct `ndarray::ViewRepr`, found struct `ndarray::OwnedRepr`
   |
   = note: expected type `ndarray::ArrayBase<ndarray::ViewRepr<&mut _>, _>`
              found type `ndarray::ArrayBase<ndarray::OwnedRepr<u8>, ndarray::dimension::dim::Dim<[usize; 2]>>`

[Palladinium]

Alternatively, we could revert the change in the macro back to calling view_mut() for the user, and add a trivial view_mut() method in ArrayViewMut that just returns self. This is more concise, but less straightforward since the two view_mut() methods would have different signatures.

[bluss]

Having a generic solution that will move or autoref as needed sounds tempting, but adding an extra trait just for that is less so. What we know is that it needs to be a method call to enable contextual autoref or not, and that it could be duck typed (Array and ArrayViewMut could have different same-named methods etc)

[bluss]

I don't have a strong opinion about it - I don't decide. One could have the explicit borrow or adding methods to make it transparent. Methods doesn't seem easy to do well, either. Third option would be to provide a way to change how the conversion to view is done via some optional syntax in the macro.

[jturner314]

I agree that the span of the error message being the whole macro call instead of just the array isn't great. The following give error messages with a span of the whole macro:

let mut view: $crate::ArrayViewMut<_, _> = $arr.into();
let mut view: $crate::ArrayViewMut<_, _> = ::std::convert::Into::into($arr);
let mut view: $crate::ArrayViewMut<_, _> = ::std::convert::From::from($arr);
let mut view: $crate::ArrayViewMut<_, _> = $crate::convert_into_arrayviewmut($arr); // where `convert_into_arrayviewmut` is a function with a parameter of type `impl Into<ArrayViewMut<'a, A, D>>`
let mut view = $crate::ArrayBase::view_mut(&mut $arr);

while the following give error messages with a span of just the array argument:

let mut view: $crate::ArrayViewMut<_, _> = $arr;
let mut view: $crate::ArrayViewMut<_, _> = $arr.view_mut();

So, it seems like non-trait method calls using dot notation or requiring the input to be ArrayViewMut give good spans, but all other conversions don't.

Here are the options that I see:

1. Use the approach proposed in this PR (use From/Into in the macro). Rely on users reading the examples and documentation, and hope the poor error messages aren't a big issue.

2. Call .view_mut() in the macro by default. Add some way to use From/Into instead:

   a. Use a move annotation, like this: multislice!(move v, s![1..2], s![2..3])).
   b. Add a separate macro, e.g. multislice_move!.
   c. Something else.

3. Add a new method (possibly hidden) implemented for ArrayViewMut, Array, ArcArray, and CowArray separately, and call this method in the macro. (For ArrayViewMut, move the view so that the lifetime of the resulting views is the same, while for the other types, take a mutable reference.) In other words, this approach uses duck typing to handle ArrayViewMut differently from the other types.

4. Use an approach similar to the existing slicing methods: provide methods that take slicing information as a parameter. The implementation is somewhat complex, but it avoids the need for any macros. The implementation and usage are simpler if we remove the ability to create both ArrayView and ArrayViewMut instances with multi-slicing, and just always create ArrayViewMut instances. Here's a rough sample implementation (click to expand the details block):

   pub unsafe trait Multislice<'out, A, D>
   where
       A: 'out,
       D: Dimension,
   {
       type Output;
   
       unsafe fn slice_and_deref(&self, view: RawArrayViewMut<A, D>) -> Self::Output;
   
       fn fold<B, F>(&self, init: B, f: F) -> B
       where
           F: FnMut(B, &D::SliceArg) -> B;
   
       fn overlaps_other(&self, dim: &D, indices: &D::SliceArg) -> bool;
   
       fn overlaps_self(&self, dim: &D) -> bool;
   }
   
   unsafe impl<'out, A, D, Do> Multislice<'out, A, D> for SliceInfo<D::SliceArg, Do>
   where
       A: 'out,
       D: Dimension,
       Do: Dimension,
   {
       type Output = ArrayViewMut<'out, A, Do>;
   
       unsafe fn slice_and_deref(&self, view: RawArrayViewMut<A, D>) -> Self::Output {
           view.slice_move(self).deref_into_view_mut()
       }
   
       fn fold<B, F>(&self, init: B, mut f: F) -> B
       where
           F: FnMut(B, &D::SliceArg) -> B,
       {
           f(init, &*self)
       }
   
       fn overlaps_other(&self, dim: &D, indices: &D::SliceArg) -> bool {
           slices_intersect(dim, &*self, indices)
       }
   
       fn overlaps_self(&self, _dim: &D) -> bool {
           false
       }
   }
   
   /// Implement for nested tuples. Since nesting allows arbitrary depth, it's
   /// possible to perform arbitrarily many non-overlapping slices by nesting
   /// slicing information. It would also be possible to implement `Multislice`
   /// for tuples of length greater than 2, for convenience (so the user could
   /// avoid nesting when the number of slices to be performed is small).
   unsafe impl<'out, A, D, T1, T2> Multislice<'out, A, D> for (T1, T2)
   where
       A: 'out,
       D: Dimension,
       T1: Multislice<'out, A, D>,
       T2: Multislice<'out, A, D>,
   {
       type Output = (T1::Output, T2::Output);
   
       unsafe fn slice_and_deref(&self, view: RawArrayViewMut<A, D>) -> Self::Output {
           assert!(!self.overlaps_self(&view.raw_dim()));
           (
               self.0.slice_and_deref(view.clone()),
               self.1.slice_and_deref(view),
           )
       }
   
       fn fold<B, F>(&self, init: B, mut f: F) -> B
       where
           F: FnMut(B, &D::SliceArg) -> B,
       {
           self.1.fold(self.0.fold(init, &mut f), f)
       }
   
       fn overlaps_other(&self, dim: &D, indices: &D::SliceArg) -> bool {
           self.0.overlaps_other(dim, indices) || self.1.overlaps_other(dim, indices)
       }
   
       fn overlaps_self(&self, dim: &D) -> bool {
           self.0.overlaps_self(dim)
               || self.1.overlaps_self(dim)
               || self.0.fold(false, |acc, indices| {
                   acc || self.1.overlaps_other(&dim, indices)
               })
       }
   }
   
   unsafe impl<'out, A, D, T> Multislice<'out, A, D> for &'_ T
   where
       A: 'out,
       D: Dimension,
       T: Multislice<'out, A, D>,
   {
       type Output = T::Output;
   
       unsafe fn slice_and_deref(&self, view: RawArrayViewMut<A, D>) -> Self::Output {
           T::slice_and_deref(self, view)
       }
   
       fn fold<B, F>(&self, init: B, f: F) -> B
       where
           F: FnMut(B, &D::SliceArg) -> B,
       {
           T::fold(self, init, f)
       }
   
       fn overlaps_other(&self, dim: &D, indices: &D::SliceArg) -> bool {
           T::overlaps_other(self, dim, indices)
       }
   
       fn overlaps_self(&self, dim: &D) -> bool {
           T::overlaps_self(self, dim)
       }
   }
   
   impl<A, S, D> ArrayBase<S, D>
   where
       S: RawData<Elem = A>,
       D: Dimension,
   {
       pub fn multislice_mut<'a, M>(&'a mut self, info: M) -> M::Output
       where
           S: DataMut,
           M: Multislice<'a, A, D>,
       {
           unsafe { info.slice_and_deref(self.raw_view_mut()) }
       }
   }
   
   impl<'a, A, D> ArrayViewMut<'a, A, D>
   where
       D: Dimension,
   {
       pub fn multislice_move<M>(mut self, info: M) -> M::Output
       where
           M: Multislice<'a, A, D>,
       {
           unsafe { info.slice_and_deref(self.raw_view_mut()) }
       }
   }

   It could be used like this:

   let mut arr = array![[1, 2, 3, 4], [5, 6, 7, 8]];
   let (v1, (v2, v3)) = arr.multislice_mut((s![.., ..2], (s![0, 2..], s![1, 2..])));
   assert_eq!(v1, aview2(&[[1, 2], [5, 6]]));
   assert_eq!(v2, aview1(&[3, 4]));
   assert_eq!(v3, aview1(&[7, 8]));
   let (v4, v5) = v1.multislice_move((s![.., 0], s![.., 1]));
   assert_eq!(v4, aview1(&[1, 5]));
   assert_eq!(v5, aview1(&[2, 6]));

   or, if we implement Multislice for tuples longer than two elements, the first multi-slice could be written like this:

   let (v1, v2, v3) = arr.multislice_mut((s![.., ..2], s![0, 2..], s![1, 2..]));

I like options 1 and 2 pretty well. The semantics of option 3 are too confusing IMO. From the user's perspective, option 4 is really nice (no macros other than s!). The implementation is somewhat complex, but it's not much worse than the implementation of the multislice! macro.

[LukeMathWalker]

Speaking purely from an API perspective, I feel option 4. significantly improves what we have right now.
It reduces the amount of non-standard syntax a user has to learn to s! and provides much more understandable error messages without compromising on functionality.

[bluss]

Alternative 2 sounds like something we can do for 0.13 or 0.13.1, and alternative 4 sounds cool of course, but there is no rush and it can happen if someone has time to write it(?)

[jturner314]

Okay, I've created #716 and #717 to implement option 4. (#716 supports nested tuples of slicing information, while #717 does not. We should pick one.)

If we don't want to wait for #716/#717, we could remove the multislice! macro for 0.13.0 and merge #716/#717 for 0.13.1.

[bluss]

Awesome. I prefer smooth evolution when not terribly hard to do so, deprecations when possible.

So for example here multislice! Can be a deprecated shim for a new method, even if not a perfect shim?

How to weigh the importance of deprecations is usually about gauging the number of users it impacts and how easy it is to understand and fix the break.

[jturner314]

Since multislice! isn't part of any crates.io release of ndarray (it's just on the master branch), we can remove it entirely without being a breaking change (except for anyone using the master branch instead of official releases). It seems like a shame to include it as part of 0.13.0, and then have to deprecate it immediately after its first release.

So, I'd prefer to remove multislice! for 0.13.0 and either (1) not include any multi-slicing functionality until 0.13.1 or (2) finish up #716/#717 in time for 0.13.0.

[bluss]

Oh sounds good. (I'm sorry I didn't realize multislice hasn't been released yet.) Another reason we should get the release out of the door. We shouldn't need to rewrite features before we premiere them :)