From 60a900ee10c70ea90927c1192ec6683f26f496de Mon Sep 17 00:00:00 2001 From: The8472 Date: Sat, 15 May 2021 20:08:09 +0200 Subject: [PATCH 1/3] remove InPlaceIterable marker from Peekable due to unsoundness The unsoundness is not in Peekable per se, it rather is due to the interaction between Peekable being able to hold an extra item and vec::IntoIter's clone implementation shortening the allocation. An alternative solution would be to change IntoIter's clone implementation to keep enough spare capacity available. --- library/alloc/benches/vec.rs | 1 - library/alloc/tests/vec.rs | 1 - library/core/src/iter/adapters/peekable.rs | 5 +---- 3 files changed, 1 insertion(+), 6 deletions(-) diff --git a/library/alloc/benches/vec.rs b/library/alloc/benches/vec.rs index 48709e89823d8..c9bdcaa78f391 100644 --- a/library/alloc/benches/vec.rs +++ b/library/alloc/benches/vec.rs @@ -468,7 +468,6 @@ fn bench_in_place_recycle(b: &mut Bencher) { .enumerate() .map(|(idx, e)| idx.wrapping_add(e)) .fuse() - .peekable() .collect::>(), ); }); diff --git a/library/alloc/tests/vec.rs b/library/alloc/tests/vec.rs index 4dcc5d30debf7..aa606cd23158a 100644 --- a/library/alloc/tests/vec.rs +++ b/library/alloc/tests/vec.rs @@ -1002,7 +1002,6 @@ fn test_from_iter_specialization_with_iterator_adapters() { .zip(std::iter::repeat(1usize)) .map(|(a, b)| a + b) .map_while(Option::Some) - .peekable() .skip(1) .map(|e| if e != usize::MAX { Ok(std::num::NonZeroUsize::new(e)) } else { Err(()) }); assert_in_place_trait(&iter); diff --git a/library/core/src/iter/adapters/peekable.rs b/library/core/src/iter/adapters/peekable.rs index 8ee7d0955c6d1..69bd2996efe73 100644 --- a/library/core/src/iter/adapters/peekable.rs +++ b/library/core/src/iter/adapters/peekable.rs @@ -1,4 +1,4 @@ -use crate::iter::{adapters::SourceIter, FusedIterator, InPlaceIterable, TrustedLen}; +use crate::iter::{adapters::SourceIter, FusedIterator, TrustedLen}; use crate::ops::{ControlFlow, Try}; /// An iterator with a `peek()` that returns an optional reference to the next @@ -356,6 +356,3 @@ where unsafe { SourceIter::as_inner(&mut self.iter) } } } - -#[unstable(issue = "none", feature = "inplace_iteration")] -unsafe impl InPlaceIterable for Peekable {} From a44a059c3b65b5ce9c996db0995eb574e46fa314 Mon Sep 17 00:00:00 2001 From: The8472 Date: Sat, 15 May 2021 20:41:15 +0200 Subject: [PATCH 2/3] add regression test --- library/alloc/tests/vec.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/library/alloc/tests/vec.rs b/library/alloc/tests/vec.rs index aa606cd23158a..6c71cf924d2a3 100644 --- a/library/alloc/tests/vec.rs +++ b/library/alloc/tests/vec.rs @@ -1094,6 +1094,18 @@ fn test_from_iter_specialization_panic_during_drop_leaks() { } } +// regression test for issue #85322. Peekable previously implemented InPlaceIterable, +// but due to an interaction with IntoIter's current Clone implementation it failed to uphold +// the contract. +#[test] +fn test_collect_after_iterator_clone() { + let v = vec![0; 5]; + let mut i = v.into_iter().peekable(); + i.peek(); + let v = i.clone().collect::>(); + assert!(v.len() <= v.capacity()); +} + #[test] fn test_cow_from() { let borrowed: &[_] = &["borrowed", "(slice)"]; From 7cb4e5180f16ad83d39da9555561360add5fb22d Mon Sep 17 00:00:00 2001 From: the8472 Date: Sun, 16 May 2021 15:35:05 +0200 Subject: [PATCH 3/3] from review: more robust test This also checks the contents and not only the capacity in case IntoIter's clone implementation is changed to add capacity at the end. Extra capacity at the beginning would be needed to make InPlaceIterable work. Co-authored-by: Giacomo Stevanato --- library/alloc/tests/vec.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/library/alloc/tests/vec.rs b/library/alloc/tests/vec.rs index 6c71cf924d2a3..ad69234403b9c 100644 --- a/library/alloc/tests/vec.rs +++ b/library/alloc/tests/vec.rs @@ -1100,12 +1100,12 @@ fn test_from_iter_specialization_panic_during_drop_leaks() { #[test] fn test_collect_after_iterator_clone() { let v = vec![0; 5]; - let mut i = v.into_iter().peekable(); + let mut i = v.into_iter().map(|i| i + 1).peekable(); i.peek(); let v = i.clone().collect::>(); + assert_eq!(v, [1, 1, 1, 1, 1]); assert!(v.len() <= v.capacity()); } - #[test] fn test_cow_from() { let borrowed: &[_] = &["borrowed", "(slice)"];