From 6c6b51cf54746ad77958777fed022007a2bf97c7 Mon Sep 17 00:00:00 2001 From: Liam Perlaki Date: Fri, 13 Mar 2020 19:36:00 +0100 Subject: [PATCH 1/4] Add `array::FillError` to core --- library/core/src/array/mod.rs | 99 +++++++++++++++++++++++++++++++++++ library/core/tests/array.rs | 38 ++++++++++++++ library/core/tests/lib.rs | 1 + 3 files changed, 138 insertions(+) diff --git a/library/core/src/array/mod.rs b/library/core/src/array/mod.rs index c1d3aca6fdd4f..088b2e9c9d438 100644 --- a/library/core/src/array/mod.rs +++ b/library/core/src/array/mod.rs @@ -11,7 +11,9 @@ use crate::cmp::Ordering; use crate::convert::{Infallible, TryFrom}; use crate::fmt; use crate::hash::{self, Hash}; +use crate::iter::FromIterator; use crate::marker::Unsize; +use crate::mem::MaybeUninit; use crate::slice::{Iter, IterMut}; mod iter; @@ -174,6 +176,103 @@ impl fmt::Debug for [T; N] { } } +/// Return Error of the FromIterator impl for array +#[unstable(feature = "array_from_iter_impl", issue = "none")] +pub struct FillError { + array: [MaybeUninit; N], + len: usize, +} + +#[unstable(feature = "array_from_iter_impl", issue = "none")] +impl fmt::Display for FillError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Display::fmt( + &format_args!("The iterator only returned {} items, but {} were needed", self.len(), N), + f, + ) + } +} + +#[unstable(feature = "array_from_iter_impl", issue = "none")] +impl fmt::Debug for FillError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("FillError") + .field("array", &self.as_slice()) + .field("len", &self.len()) + .finish() + } +} + +#[unstable(feature = "array_from_iter_impl", issue = "none")] +impl Drop for FillError { + fn drop(&mut self) { + // SAFETY: This is safe: `as_mut_slice` returns exactly the sub-slice + // of elements that have been initialized and need to be droped + unsafe { crate::ptr::drop_in_place(self.as_mut_slice()) } + } +} + +#[unstable(feature = "array_from_iter_impl", issue = "none")] +impl FillError { + fn new() -> Self { + Self { array: MaybeUninit::uninit_array(), len: 0 } + } + + /// Returns how many elements were read from the given iterator. + pub fn len(&self) -> usize { + self.len + } + + /// Returns an immutable slice of all initialized elements. + pub fn as_slice(&self) -> &[T] { + // SAFETY: We know that all elements from 0 to len are properly initialized. + unsafe { MaybeUninit::slice_get_ref(&self.array[0..self.len]) } + } + + /// Returns a mutable slice of all initialized elements. + pub fn as_mut_slice(&mut self) -> &mut [T] { + // SAFETY: We know that all elements from 0 to len are properly initialized. + unsafe { MaybeUninit::slice_get_mut(&mut self.array[0..self.len]) } + } + + /// Tries to initialize the left-over elements using `iter`. + pub fn fill>(mut self, iter: I) -> Result<[T; N], FillError> { + let mut iter = iter.into_iter(); + + for i in self.len..N { + if let Some(value) = iter.next() { + self.array[i].write(value); + } else { + self.len = i; + return Err(self); + } + } + + // SAFETY: The transmute here is actually safe. The docs of `MaybeUninit` + // promise: + // + // > `MaybeUninit` is guaranteed to have the same size and alignment + // > as `T`. + // + // The docs even show a transmute from an array of `MaybeUninit` to + // an array of `T`. + // + // With that, this initialization satisfies the invariants. + // FIXME: actually use `mem::transmute` here, once it + // works with const generics: + // `mem::transmute::<[MaybeUninit; N], [T; N]>(array)` + Ok(unsafe { crate::ptr::read(&self.array as *const [MaybeUninit; N] as *const [T; N]) }) + } +} + +#[unstable(feature = "array_from_iter_impl", issue = "none")] +impl FromIterator for Result<[T; N], FillError> { + #[inline] + fn from_iter>(iter: I) -> Self { + FillError::::new().fill(iter) + } +} + #[stable(feature = "rust1", since = "1.0.0")] impl<'a, T, const N: usize> IntoIterator for &'a [T; N] { type Item = &'a T; diff --git a/library/core/tests/array.rs b/library/core/tests/array.rs index 5aba1a5d958d1..a42141a8f5079 100644 --- a/library/core/tests/array.rs +++ b/library/core/tests/array.rs @@ -330,3 +330,41 @@ fn array_map_drop_safety() { assert_eq!(DROPPED.load(Ordering::SeqCst), num_to_create); panic!("test succeeded") } + +#[test] +fn array_collects() { + let v = vec![1, 2, 3, 4, 5]; + let a: [i32; 5] = v + .clone() + .into_iter() + .collect::>>() + .unwrap(); + + assert_eq!(v[..], a[..]); +} + +#[test] +fn array_collect_drop_on_panic() { + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + + #[derive(Clone)] + struct Foo(Arc); + + // count the number of eleemts that got dropped + impl Drop for Foo { + fn drop(&mut self) { + self.0.fetch_add(1, Ordering::SeqCst); + } + } + + let i = Arc::new(AtomicUsize::new(0)); + let foo = Foo(i.clone()); + + std::panic::catch_unwind(move || { + let _a: [Foo; 5] = from_iter(vec![foo.clone(), foo.clone(), foo.clone(), foo]); + }) + .unwrap_err(); + + assert_eq!(i.load(Ordering::SeqCst), 4); +} diff --git a/library/core/tests/lib.rs b/library/core/tests/lib.rs index 7e75c7cf47bf6..645f614e94be8 100644 --- a/library/core/tests/lib.rs +++ b/library/core/tests/lib.rs @@ -56,6 +56,7 @@ #![feature(unsafe_block_in_unsafe_fn)] #![feature(int_bits_const)] #![deny(unsafe_op_in_unsafe_fn)] +#![feature(array_from_iter_impl)] extern crate test; From fe8d917221a5904e72bae82743c31c780ddf66ae Mon Sep 17 00:00:00 2001 From: Bastian Kauschke Date: Wed, 19 Aug 2020 20:17:25 +0200 Subject: [PATCH 2/4] remove `FromIterator` impl --- library/core/src/array/mod.rs | 64 +++++++---------------------------- library/core/tests/array.rs | 50 +++++++++++++-------------- 2 files changed, 38 insertions(+), 76 deletions(-) diff --git a/library/core/src/array/mod.rs b/library/core/src/array/mod.rs index 088b2e9c9d438..bf5aa5dcce5f8 100644 --- a/library/core/src/array/mod.rs +++ b/library/core/src/array/mod.rs @@ -11,7 +11,6 @@ use crate::cmp::Ordering; use crate::convert::{Infallible, TryFrom}; use crate::fmt; use crate::hash::{self, Hash}; -use crate::iter::FromIterator; use crate::marker::Unsize; use crate::mem::MaybeUninit; use crate::slice::{Iter, IterMut}; @@ -176,7 +175,10 @@ impl fmt::Debug for [T; N] { } } -/// Return Error of the FromIterator impl for array +/// The error returned by the `FromIterator` implementation of +/// arrays once these are implemented. +/// +/// Until then `FillError::new().fill(iter)` can be used instead. #[unstable(feature = "array_from_iter_impl", issue = "none")] pub struct FillError { array: [MaybeUninit; N], @@ -194,12 +196,9 @@ impl fmt::Display for FillError { } #[unstable(feature = "array_from_iter_impl", issue = "none")] -impl fmt::Debug for FillError { +impl fmt::Debug for FillError { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("FillError") - .field("array", &self.as_slice()) - .field("len", &self.len()) - .finish() + fmt::Display::fmt(self, f) } } @@ -207,14 +206,16 @@ impl fmt::Debug for FillError { impl Drop for FillError { fn drop(&mut self) { // SAFETY: This is safe: `as_mut_slice` returns exactly the sub-slice - // of elements that have been initialized and need to be droped + // of elements that have been initialized and need to be dropped. unsafe { crate::ptr::drop_in_place(self.as_mut_slice()) } } } #[unstable(feature = "array_from_iter_impl", issue = "none")] impl FillError { - fn new() -> Self { + /// Creates a new empty `FillError` which can be used + /// to build `[T; N]` from an iterator. + pub fn new() -> Self { Self { array: MaybeUninit::uninit_array(), len: 0 } } @@ -242,8 +243,8 @@ impl FillError { for i in self.len..N { if let Some(value) = iter.next() { self.array[i].write(value); + self.len = i + 1; } else { - self.len = i; return Err(self); } } @@ -265,14 +266,6 @@ impl FillError { } } -#[unstable(feature = "array_from_iter_impl", issue = "none")] -impl FromIterator for Result<[T; N], FillError> { - #[inline] - fn from_iter>(iter: I) -> Self { - FillError::::new().fill(iter) - } -} - #[stable(feature = "rust1", since = "1.0.0")] impl<'a, T, const N: usize> IntoIterator for &'a [T; N] { type Item = &'a T; @@ -484,42 +477,11 @@ impl [T; N] { /// assert_eq!(y, [6, 9, 3, 3]); /// ``` #[unstable(feature = "array_map", issue = "75243")] - pub fn map(self, mut f: F) -> [U; N] + pub fn map(self, f: F) -> [U; N] where F: FnMut(T) -> U, { - use crate::mem::MaybeUninit; - struct Guard { - dst: *mut T, - initialized: usize, - } - - impl Drop for Guard { - fn drop(&mut self) { - debug_assert!(self.initialized <= N); - - let initialized_part = - crate::ptr::slice_from_raw_parts_mut(self.dst, self.initialized); - // SAFETY: this raw slice will contain only initialized objects - // that's why, it is allowed to drop it. - unsafe { - crate::ptr::drop_in_place(initialized_part); - } - } - } - let mut dst = MaybeUninit::uninit_array::(); - let mut guard: Guard = - Guard { dst: MaybeUninit::slice_as_mut_ptr(&mut dst), initialized: 0 }; - for (src, dst) in IntoIter::new(self).zip(&mut dst) { - dst.write(f(src)); - guard.initialized += 1; - } - // FIXME: Convert to crate::mem::transmute once it works with generics. - // unsafe { crate::mem::transmute::<[MaybeUninit; N], [U; N]>(dst) } - crate::mem::forget(guard); - // SAFETY: At this point we've properly initialized the whole array - // and we just need to cast it to the correct type. - unsafe { crate::mem::transmute_copy::<_, [U; N]>(&dst) } + FillError::new().fill(IntoIter::new(self).map(f)).unwrap() } /// Returns a slice containing the entire array. Equivalent to `&s[..]`. diff --git a/library/core/tests/array.rs b/library/core/tests/array.rs index a42141a8f5079..6c4b2584ebd45 100644 --- a/library/core/tests/array.rs +++ b/library/core/tests/array.rs @@ -1,4 +1,4 @@ -use core::array::{FixedSizeArray, IntoIter}; +use core::array::{FillError, FixedSizeArray, IntoIter}; use core::convert::TryFrom; #[test] @@ -334,37 +334,37 @@ fn array_map_drop_safety() { #[test] fn array_collects() { let v = vec![1, 2, 3, 4, 5]; - let a: [i32; 5] = v - .clone() - .into_iter() - .collect::>>() - .unwrap(); + let a: [i32; 5] = FillError::new().fill(v.clone()).unwrap(); assert_eq!(v[..], a[..]); } #[test] -fn array_collect_drop_on_panic() { - use std::sync::atomic::{AtomicUsize, Ordering}; - use std::sync::Arc; - - #[derive(Clone)] - struct Foo(Arc); - - // count the number of eleemts that got dropped - impl Drop for Foo { +#[should_panic(expected = "test succeeded")] +fn array_collect_drop_safety() { + use core::sync::atomic::AtomicUsize; + use core::sync::atomic::Ordering; + static DROPPED: AtomicUsize = AtomicUsize::new(0); + struct DropCounter; + impl Drop for DropCounter { fn drop(&mut self) { - self.0.fetch_add(1, Ordering::SeqCst); + DROPPED.fetch_add(1, Ordering::SeqCst); } } - let i = Arc::new(AtomicUsize::new(0)); - let foo = Foo(i.clone()); - - std::panic::catch_unwind(move || { - let _a: [Foo; 5] = from_iter(vec![foo.clone(), foo.clone(), foo.clone(), foo]); - }) - .unwrap_err(); - - assert_eq!(i.load(Ordering::SeqCst), 4); + let num_to_create = 5; + let success = std::panic::catch_unwind(|| { + let items = [0; 10]; + let mut nth = 0; + FillError::::new() + .fill(items.iter().map(|_| { + assert!(nth < num_to_create); + nth += 1; + DropCounter + })) + .unwrap() + }); + assert!(success.is_err()); + assert_eq!(DROPPED.load(Ordering::SeqCst), num_to_create); + panic!("test succeeded") } From 8bdf0f3af7b46b4a91f0a5b1cc215064fceab4d7 Mon Sep 17 00:00:00 2001 From: Bastian Kauschke Date: Wed, 19 Aug 2020 21:46:47 +0200 Subject: [PATCH 3/4] fix `FillError::fill` double free --- library/core/src/array/mod.rs | 8 ++++++-- library/core/tests/array.rs | 21 ++++++++++++++++++++- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/library/core/src/array/mod.rs b/library/core/src/array/mod.rs index bf5aa5dcce5f8..09e2a9d57920d 100644 --- a/library/core/src/array/mod.rs +++ b/library/core/src/array/mod.rs @@ -12,7 +12,7 @@ use crate::convert::{Infallible, TryFrom}; use crate::fmt; use crate::hash::{self, Hash}; use crate::marker::Unsize; -use crate::mem::MaybeUninit; +use crate::mem::{self, MaybeUninit}; use crate::slice::{Iter, IterMut}; mod iter; @@ -259,10 +259,14 @@ impl FillError { // an array of `T`. // // With that, this initialization satisfies the invariants. + // // FIXME: actually use `mem::transmute` here, once it // works with const generics: // `mem::transmute::<[MaybeUninit; N], [T; N]>(array)` - Ok(unsafe { crate::ptr::read(&self.array as *const [MaybeUninit; N] as *const [T; N]) }) + let arr = unsafe { crate::mem::transmute_copy::<_, [T; N]>(&self.array) }; + // We forget `self` to not drop anything here. + mem::forget(self); + Ok(arr) } } diff --git a/library/core/tests/array.rs b/library/core/tests/array.rs index 6c4b2584ebd45..f2548c4247ac3 100644 --- a/library/core/tests/array.rs +++ b/library/core/tests/array.rs @@ -341,7 +341,7 @@ fn array_collects() { #[test] #[should_panic(expected = "test succeeded")] -fn array_collect_drop_safety() { +fn array_collect_panic_safety() { use core::sync::atomic::AtomicUsize; use core::sync::atomic::Ordering; static DROPPED: AtomicUsize = AtomicUsize::new(0); @@ -368,3 +368,22 @@ fn array_collect_drop_safety() { assert_eq!(DROPPED.load(Ordering::SeqCst), num_to_create); panic!("test succeeded") } + +#[test] +fn array_collect_no_double_drop() { + use core::sync::atomic::AtomicUsize; + use core::sync::atomic::Ordering; + static DROPPED: AtomicUsize = AtomicUsize::new(0); + struct DropCounter; + impl Drop for DropCounter { + fn drop(&mut self) { + DROPPED.fetch_add(1, Ordering::SeqCst); + } + } + + const LEN: usize = 10; + let items = [0; LEN]; + let _ = FillError::::new().fill(items.iter().map(|_| DropCounter)).unwrap(); + + assert_eq!(DROPPED.load(Ordering::SeqCst), LEN); +} From 0a979aba402510fb6dc3b0958a5fe29c0b1d6ea5 Mon Sep 17 00:00:00 2001 From: Bastian Kauschke Date: Thu, 24 Sep 2020 10:35:06 +0200 Subject: [PATCH 4/4] rebase --- library/core/src/array/mod.rs | 4 ++-- library/core/src/mem/maybe_uninit.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/library/core/src/array/mod.rs b/library/core/src/array/mod.rs index 09e2a9d57920d..7eb20874a80ae 100644 --- a/library/core/src/array/mod.rs +++ b/library/core/src/array/mod.rs @@ -227,13 +227,13 @@ impl FillError { /// Returns an immutable slice of all initialized elements. pub fn as_slice(&self) -> &[T] { // SAFETY: We know that all elements from 0 to len are properly initialized. - unsafe { MaybeUninit::slice_get_ref(&self.array[0..self.len]) } + unsafe { MaybeUninit::slice_assume_init_ref(&self.array[0..self.len]) } } /// Returns a mutable slice of all initialized elements. pub fn as_mut_slice(&mut self) -> &mut [T] { // SAFETY: We know that all elements from 0 to len are properly initialized. - unsafe { MaybeUninit::slice_get_mut(&mut self.array[0..self.len]) } + unsafe { MaybeUninit::slice_assume_init_mut(&mut self.array[0..self.len]) } } /// Tries to initialize the left-over elements using `iter`. diff --git a/library/core/src/mem/maybe_uninit.rs b/library/core/src/mem/maybe_uninit.rs index e629d28eae163..fa98cfe36539c 100644 --- a/library/core/src/mem/maybe_uninit.rs +++ b/library/core/src/mem/maybe_uninit.rs @@ -819,7 +819,7 @@ impl MaybeUninit { #[unstable(feature = "maybe_uninit_slice", issue = "63569")] #[inline(always)] pub unsafe fn slice_assume_init_mut(slice: &mut [Self]) -> &mut [T] { - // SAFETY: similar to safety notes for `slice_get_ref`, but we have a + // SAFETY: similar to safety notes for `slice_assume_init_ref`, but we have a // mutable reference which is also guaranteed to be valid for writes. unsafe { &mut *(slice as *mut [Self] as *mut [T]) } }