FIX: Use array builder from_data_ptr(..).with_strides_dim() where possible

This avoids using the naked ArrayBase { .. } and similar struct
constructors, which should make it easier for us to change
representation in the future if needed.

The constructor methods are unsafe since they rely on the caller to
assert certain invariants; this means that this change increases the
number of `unsafe` code blocks in ndarray, but that's only correct since
every ArrayBase construction is safety-critical w.r.t the validity of
the pointer, dimensions and strides taken together.

The resulting code is often easier to read - especially when we are only
updating the strides and dimensions of an existing array or view.

The .with_strides_dim() method requires the Dimension trait so that
appropriate debug assertions can be added in this method if desired.
This precludes us from using this method in the `Clone` impl, but that's
just a minor flaw and the only exception.

Author: bluss

src/data_traits.rs

@@ -245,11 +245,10 @@ unsafe impl<A> Data for OwnedArcRepr<A> {
     {
         Self::ensure_unique(&mut self_);
         let data = Arc::try_unwrap(self_.data.0).ok().unwrap();
-        ArrayBase {
-            data,
-            ptr: self_.ptr,
-            dim: self_.dim,
-            strides: self_.strides,
+        // safe because data is equivalent
+        unsafe {
+            ArrayBase::from_data_ptr(data, self_.ptr)
+                .with_strides_dim(self_.strides, self_.dim)
         }
     }
 
@@ -544,11 +543,10 @@ unsafe impl<'a, A> Data for CowRepr<'a, A> {
     {
         match self_.data {
             CowRepr::View(_) => self_.to_owned(),
-            CowRepr::Owned(data) => ArrayBase {
-                data,
-                ptr: self_.ptr,
-                dim: self_.dim,
-                strides: self_.strides,
+            CowRepr::Owned(data) => unsafe {
+                // safe because the data is equivalent so ptr, dims remain valid
+                ArrayBase::from_data_ptr(data, self_.ptr)
+                    .with_strides_dim(self_.strides, self_.dim)
             },
         }
     }

src/impl_clone.rs

@@ -11,6 +11,7 @@ use crate::RawDataClone;
 
 impl<S: RawDataClone, D: Clone> Clone for ArrayBase<S, D> {
     fn clone(&self) -> ArrayBase<S, D> {
+        // safe because `clone_with_ptr` promises to provide equivalent data and ptr
         unsafe {
             let (data, ptr) = self.data.clone_with_ptr(self.ptr);
             ArrayBase {

src/impl_constructors.rs

@@ -470,12 +470,9 @@ where
     unsafe fn from_vec_dim_stride_unchecked(dim: D, strides: D, mut v: Vec<A>) -> Self {
         // debug check for issues that indicates wrong use of this constructor
         debug_assert!(dimension::can_index_slice(&v, &dim, &strides).is_ok());
-        ArrayBase {
-            ptr: nonnull_from_vec_data(&mut v).offset(-offset_from_ptr_to_memory(&dim, &strides)),
-            data: DataOwned::new(v),
-            strides,
-            dim,
-        }
+
+        let ptr = nonnull_from_vec_data(&mut v).offset(-offset_from_ptr_to_memory(&dim, &strides));
+        ArrayBase::from_data_ptr(DataOwned::new(v), ptr).with_strides_dim(strides, dim)
     }
 
     /// Create an array with uninitalized elements, shape `shape`.

src/impl_cow.rs

@@ -33,11 +33,10 @@ where
     D: Dimension,
 {
     fn from(view: ArrayView<'a, A, D>) -> CowArray<'a, A, D> {
-        ArrayBase {
-            data: CowRepr::View(view.data),
-            ptr: view.ptr,
-            dim: view.dim,
-            strides: view.strides,
+        // safe because equivalent data
+        unsafe {
+            ArrayBase::from_data_ptr(CowRepr::View(view.data), view.ptr)
+                .with_strides_dim(view.strides, view.dim)
         }
     }
 }
@@ -47,11 +46,10 @@ where
     D: Dimension,
 {
     fn from(array: Array<A, D>) -> CowArray<'a, A, D> {
-        ArrayBase {
-            data: CowRepr::Owned(array.data),
-            ptr: array.ptr,
-            dim: array.dim,
-            strides: array.strides,
+        // safe because equivalent data
+        unsafe {
+            ArrayBase::from_data_ptr(CowRepr::Owned(array.data), array.ptr)
+                .with_strides_dim(array.strides, array.dim)
         }
     }
 }

src/impl_methods.rs

@@ -242,11 +242,9 @@ where
         S: DataOwned,
     {
         let data = self.data.into_shared();
-        ArrayBase {
-            data,
-            ptr: self.ptr,
-            dim: self.dim,
-            strides: self.strides,
+        // safe because: equivalent unmoved data, ptr and dims remain valid
+        unsafe {
+            ArrayBase::from_data_ptr(data, self.ptr).with_strides_dim(self.strides, self.dim)
         }
     }
 
@@ -434,11 +432,9 @@ where
                 *new_s = *s;
             });
 
-        ArrayBase {
-            ptr: self.ptr,
-            data: self.data,
-            dim: new_dim,
-            strides: new_strides,
+        // safe because new dimension, strides allow access to a subset of old data
+        unsafe {
+            self.with_strides_dim(new_strides, new_dim)
         }
     }
 
@@ -757,11 +753,10 @@ where
         self.collapse_axis(axis, index);
         let dim = self.dim.remove_axis(axis);
         let strides = self.strides.remove_axis(axis);
-        ArrayBase {
-            ptr: self.ptr,
-            data: self.data,
-            dim,
-            strides,
+        // safe because new dimension, strides allow access to a subset of old data
+        unsafe {
+            ArrayBase::from_data_ptr(self.data, self.ptr)
+                .with_strides_dim(strides, dim)
         }
     }
 
@@ -1244,11 +1239,9 @@ where
     /// Return the diagonal as a one-dimensional array.
     pub fn into_diag(self) -> ArrayBase<S, Ix1> {
         let (len, stride) = self.diag_params();
-        ArrayBase {
-            data: self.data,
-            ptr: self.ptr,
-            dim: Ix1(len),
-            strides: Ix1(stride as Ix),
+        // safe because new len stride allows access to a subset of the current elements
+        unsafe {
+            self.with_strides_dim(Ix1(stride as Ix), Ix1(len))
         }
     }
 
@@ -1498,22 +1491,15 @@ where
             return Err(error::incompatible_shapes(&self.dim, &shape));
         }
         // Check if contiguous, if not => copy all, else just adapt strides
-        if self.is_standard_layout() {
-            Ok(ArrayBase {
-                data: self.data,
-                ptr: self.ptr,
-                strides: shape.default_strides(),
-                dim: shape,
-            })
-        } else if self.ndim() > 1 && self.raw_view().reversed_axes().is_standard_layout() {
-            Ok(ArrayBase {
-                data: self.data,
-                ptr: self.ptr,
-                strides: shape.fortran_strides(),
-                dim: shape,
-            })
-        } else {
-            Err(error::from_kind(error::ErrorKind::IncompatibleLayout))
+        unsafe {
+            // safe because arrays are contiguous and len is unchanged
+            if self.is_standard_layout() {
+                Ok(self.with_strides_dim(shape.default_strides(), shape))
+            } else if self.ndim() > 1 && self.raw_view().reversed_axes().is_standard_layout() {
+                Ok(self.with_strides_dim(shape.fortran_strides(), shape))
+            } else {
+                Err(error::from_kind(error::ErrorKind::IncompatibleLayout))
+            }
         }
     }
 
@@ -1554,11 +1540,9 @@ where
         // Check if contiguous, if not => copy all, else just adapt strides
         if self.is_standard_layout() {
             let cl = self.clone();
-            ArrayBase {
-                data: cl.data,
-                ptr: cl.ptr,
-                strides: shape.default_strides(),
-                dim: shape,
+            // safe because array is contiguous and shape has equal number of elements
+            unsafe {
+                cl.with_strides_dim(shape.default_strides(), shape)
             }
         } else {
             let v = self.iter().cloned().collect::<Vec<A>>();
@@ -1576,11 +1560,10 @@ where
     ///                                 [3, 4]]).into_dyn();
     /// ```
     pub fn into_dyn(self) -> ArrayBase<S, IxDyn> {
-        ArrayBase {
-            data: self.data,
-            ptr: self.ptr,
-            dim: self.dim.into_dyn(),
-            strides: self.strides.into_dyn(),
+        // safe because new dims equivalent
+        unsafe {
+            ArrayBase::from_data_ptr(self.data, self.ptr)
+                .with_strides_dim(self.strides.into_dyn(), self.dim.into_dyn())
         }
     }
 
@@ -1604,27 +1587,19 @@ where
     where
         D2: Dimension,
     {
-        if D::NDIM == D2::NDIM {
-            // safe because D == D2
-            unsafe {
+        unsafe {
+            if D::NDIM == D2::NDIM {
+                // safe because D == D2
                 let dim = unlimited_transmute::<D, D2>(self.dim);
                 let strides = unlimited_transmute::<D, D2>(self.strides);
-                return Ok(ArrayBase {
-                    data: self.data,
-                    ptr: self.ptr,
-                    dim,
-                    strides,
-                });
-            }
-        } else if D::NDIM == None || D2::NDIM == None { // one is dynamic dim
-            if let Some(dim) = D2::from_dimension(&self.dim) {
-                if let Some(strides) = D2::from_dimension(&self.strides) {
-                    return Ok(ArrayBase {
-                        data: self.data,
-                        ptr: self.ptr,
-                        dim,
-                        strides,
-                    });
+                return Ok(ArrayBase::from_data_ptr(self.data, self.ptr)
+                            .with_strides_dim(strides, dim));
+            } else if D::NDIM == None || D2::NDIM == None { // one is dynamic dim
+                // safe because dim, strides are equivalent under a different type
+                if let Some(dim) = D2::from_dimension(&self.dim) {
+                    if let Some(strides) = D2::from_dimension(&self.strides) {
+                        return Ok(self.with_strides_dim(strides, dim));
+                    }
                 }
             }
         }
@@ -1792,10 +1767,9 @@ where
                 new_strides[new_axis] = strides[axis];
             }
         }
-        ArrayBase {
-            dim: new_dim,
-            strides: new_strides,
-            ..self
+        // safe because axis invariants are checked above; they are a permutation of the old
+        unsafe {
+            self.with_strides_dim(new_strides, new_dim)
         }
     }
 
@@ -1915,17 +1889,11 @@ where
     /// ***Panics*** if the axis is out of bounds.
     pub fn insert_axis(self, axis: Axis) -> ArrayBase<S, D::Larger> {
         assert!(axis.index() <= self.ndim());
-        let ArrayBase {
-            ptr,
-            data,
-            dim,
-            strides,
-        } = self;
-        ArrayBase {
-            ptr,
-            data,
-            dim: dim.insert_axis(axis),
-            strides: strides.insert_axis(axis),
+        // safe because a new axis of length one does not affect memory layout
+        unsafe {
+            let strides = self.strides.insert_axis(axis);
+            let dim = self.dim.insert_axis(axis);
+            self.with_strides_dim(strides, dim)
         }
     }
 

src/impl_raw_views.rs

@@ -17,12 +17,8 @@ where
     /// meet all of the invariants of the `ArrayBase` type.
     #[inline]
     pub(crate) unsafe fn new(ptr: NonNull<A>, dim: D, strides: D) -> Self {
-        RawArrayView {
-            data: RawViewRepr::new(),
-            ptr,
-            dim,
-            strides,
-        }
+        RawArrayView::from_data_ptr(RawViewRepr::new(), ptr)
+            .with_strides_dim(strides, dim)
     }
 
     unsafe fn new_(ptr: *const A, dim: D, strides: D) -> Self {
@@ -163,12 +159,8 @@ where
     /// meet all of the invariants of the `ArrayBase` type.
     #[inline]
     pub(crate) unsafe fn new(ptr: NonNull<A>, dim: D, strides: D) -> Self {
-        RawArrayViewMut {
-            data: RawViewRepr::new(),
-            ptr,
-            dim,
-            strides,
-        }
+        RawArrayViewMut::from_data_ptr(RawViewRepr::new(), ptr)
+            .with_strides_dim(strides, dim)
     }
 
     unsafe fn new_(ptr: *mut A, dim: D, strides: D) -> Self {

src/impl_special_element_types.rs

@@ -40,12 +40,6 @@ where
         // "transmute" from storage of MaybeUninit<A> to storage of A
         let data = S::data_subst(data);
         let ptr = ptr.cast::<A>();
-
-        ArrayBase {
-            data,
-            ptr,
-            dim,
-            strides,
-        }
+        ArrayBase::from_data_ptr(data, ptr).with_strides_dim(strides, dim)
     }
 }

src/impl_views/constructors.rs

@@ -230,12 +230,7 @@ where
             assert!(is_aligned(ptr.as_ptr()), "The pointer must be aligned.");
             dimension::max_abs_offset_check_overflow::<A, _>(&dim, &strides).unwrap();
         }
-        ArrayView {
-            data: ViewRepr::new(),
-            ptr,
-            dim,
-            strides,
-        }
+        ArrayView::from_data_ptr(ViewRepr::new(), ptr).with_strides_dim(strides, dim)
     }
 
     /// Unsafe because: `ptr` must be valid for the given dimension and strides.
@@ -258,12 +253,7 @@ where
             assert!(is_aligned(ptr.as_ptr()), "The pointer must be aligned.");
             dimension::max_abs_offset_check_overflow::<A, _>(&dim, &strides).unwrap();
         }
-        ArrayViewMut {
-            data: ViewRepr::new(),
-            ptr,
-            dim,
-            strides,
-        }
+        ArrayViewMut::from_data_ptr(ViewRepr::new(), ptr).with_strides_dim(strides, dim)
     }
 
     /// Create a new `ArrayView`

src/lib.rs

@@ -1565,11 +1565,9 @@ where
     fn try_remove_axis(self, axis: Axis) -> ArrayBase<S, D::Smaller> {
         let d = self.dim.try_remove_axis(axis);
         let s = self.strides.try_remove_axis(axis);
-        ArrayBase {
-            ptr: self.ptr,
-            data: self.data,
-            dim: d,
-            strides: s,
+        // safe because new dimension, strides allow access to a subset of old data
+        unsafe {
+            self.with_strides_dim(s, d)
         }
     }