From 55492de54557c3308795282bd96c8120ad66b62e Mon Sep 17 00:00:00 2001 From: Scott McMurray Date: Mon, 26 Sep 2022 00:51:42 -0700 Subject: [PATCH 1/7] Use a macro to not have to copy-paste `ConstFnMutClosure::new(&mut fold, NeverShortCircuit::wrap_mut_2_imp)).0` everywhere Also use that macro to replace a bunch of places that had custom closure-wrappers. --- .../core/src/iter/adapters/array_chunks.rs | 19 ++----------- library/core/src/iter/adapters/map_while.rs | 14 +--------- library/core/src/iter/adapters/mod.rs | 11 ++------ library/core/src/iter/adapters/scan.rs | 14 +--------- library/core/src/iter/adapters/skip.rs | 12 +------- library/core/src/iter/adapters/take.rs | 14 +--------- library/core/src/iter/adapters/take_while.rs | 14 +--------- library/core/src/iter/mod.rs | 23 +++++++++++++++ library/core/src/iter/range.rs | 28 ++----------------- 9 files changed, 35 insertions(+), 114 deletions(-) diff --git a/library/core/src/iter/adapters/array_chunks.rs b/library/core/src/iter/adapters/array_chunks.rs index 489fb13c0dc97..d4fb886101fe7 100644 --- a/library/core/src/iter/adapters/array_chunks.rs +++ b/library/core/src/iter/adapters/array_chunks.rs @@ -1,7 +1,6 @@ use crate::array; -use crate::const_closure::ConstFnMutClosure; use crate::iter::{ByRefSized, FusedIterator, Iterator}; -use crate::ops::{ControlFlow, NeverShortCircuit, Try}; +use crate::ops::{ControlFlow, Try}; /// An iterator over `N` elements of the iterator at a time. /// @@ -83,13 +82,7 @@ where } } - fn fold(mut self, init: B, mut f: F) -> B - where - Self: Sized, - F: FnMut(B, Self::Item) -> B, - { - self.try_fold(init, ConstFnMutClosure::new(&mut f, NeverShortCircuit::wrap_mut_2_imp)).0 - } + impl_fold_via_try_fold! { fold -> try_fold } } #[unstable(feature = "iter_array_chunks", reason = "recently added", issue = "100450")] @@ -127,13 +120,7 @@ where try { acc } } - fn rfold(mut self, init: B, mut f: F) -> B - where - Self: Sized, - F: FnMut(B, Self::Item) -> B, - { - self.try_rfold(init, ConstFnMutClosure::new(&mut f, NeverShortCircuit::wrap_mut_2_imp)).0 - } + impl_fold_via_try_fold! { rfold -> try_rfold } } impl ArrayChunks diff --git a/library/core/src/iter/adapters/map_while.rs b/library/core/src/iter/adapters/map_while.rs index 1e8d6bf3e00e1..fbdeca4d4ee4f 100644 --- a/library/core/src/iter/adapters/map_while.rs +++ b/library/core/src/iter/adapters/map_while.rs @@ -64,19 +64,7 @@ where .into_try() } - #[inline] - fn fold(mut self, init: Acc, fold: Fold) -> Acc - where - Self: Sized, - Fold: FnMut(Acc, Self::Item) -> Acc, - { - #[inline] - fn ok(mut f: impl FnMut(B, T) -> B) -> impl FnMut(B, T) -> Result { - move |acc, x| Ok(f(acc, x)) - } - - self.try_fold(init, ok(fold)).unwrap() - } + impl_fold_via_try_fold! { fold -> try_fold } } #[unstable(issue = "none", feature = "inplace_iteration")] diff --git a/library/core/src/iter/adapters/mod.rs b/library/core/src/iter/adapters/mod.rs index de3a534f81b8a..8cc2b7cec4165 100644 --- a/library/core/src/iter/adapters/mod.rs +++ b/library/core/src/iter/adapters/mod.rs @@ -1,6 +1,5 @@ -use crate::const_closure::ConstFnMutClosure; use crate::iter::{InPlaceIterable, Iterator}; -use crate::ops::{ChangeOutputType, ControlFlow, FromResidual, NeverShortCircuit, Residual, Try}; +use crate::ops::{ChangeOutputType, ControlFlow, FromResidual, Residual, Try}; mod array_chunks; mod by_ref_sized; @@ -204,13 +203,7 @@ where .into_try() } - fn fold(mut self, init: B, mut fold: F) -> B - where - Self: Sized, - F: FnMut(B, Self::Item) -> B, - { - self.try_fold(init, ConstFnMutClosure::new(&mut fold, NeverShortCircuit::wrap_mut_2_imp)).0 - } + impl_fold_via_try_fold! { fold -> try_fold } } #[unstable(issue = "none", feature = "inplace_iteration")] diff --git a/library/core/src/iter/adapters/scan.rs b/library/core/src/iter/adapters/scan.rs index 80bfd2231241b..62470512cc747 100644 --- a/library/core/src/iter/adapters/scan.rs +++ b/library/core/src/iter/adapters/scan.rs @@ -74,19 +74,7 @@ where self.iter.try_fold(init, scan(state, f, fold)).into_try() } - #[inline] - fn fold(mut self, init: Acc, fold: Fold) -> Acc - where - Self: Sized, - Fold: FnMut(Acc, Self::Item) -> Acc, - { - #[inline] - fn ok(mut f: impl FnMut(B, T) -> B) -> impl FnMut(B, T) -> Result { - move |acc, x| Ok(f(acc, x)) - } - - self.try_fold(init, ok(fold)).unwrap() - } + impl_fold_via_try_fold! { fold -> try_fold } } #[unstable(issue = "none", feature = "inplace_iteration")] diff --git a/library/core/src/iter/adapters/skip.rs b/library/core/src/iter/adapters/skip.rs index dbf0ae9eca3eb..c6334880db57c 100644 --- a/library/core/src/iter/adapters/skip.rs +++ b/library/core/src/iter/adapters/skip.rs @@ -206,17 +206,7 @@ where if n == 0 { try { init } } else { self.iter.try_rfold(init, check(n, fold)).into_try() } } - fn rfold(mut self, init: Acc, fold: Fold) -> Acc - where - Fold: FnMut(Acc, Self::Item) -> Acc, - { - #[inline] - fn ok(mut f: impl FnMut(Acc, T) -> Acc) -> impl FnMut(Acc, T) -> Result { - move |acc, x| Ok(f(acc, x)) - } - - self.try_rfold(init, ok(fold)).unwrap() - } + impl_fold_via_try_fold! { rfold -> try_rfold } #[inline] fn advance_back_by(&mut self, n: usize) -> Result<(), usize> { diff --git a/library/core/src/iter/adapters/take.rs b/library/core/src/iter/adapters/take.rs index 2962e0104d11d..58a0b9d7bbe99 100644 --- a/library/core/src/iter/adapters/take.rs +++ b/library/core/src/iter/adapters/take.rs @@ -98,19 +98,7 @@ where } } - #[inline] - fn fold(mut self, init: Acc, fold: Fold) -> Acc - where - Self: Sized, - Fold: FnMut(Acc, Self::Item) -> Acc, - { - #[inline] - fn ok(mut f: impl FnMut(B, T) -> B) -> impl FnMut(B, T) -> Result { - move |acc, x| Ok(f(acc, x)) - } - - self.try_fold(init, ok(fold)).unwrap() - } + impl_fold_via_try_fold! { fold -> try_fold } #[inline] #[rustc_inherit_overflow_checks] diff --git a/library/core/src/iter/adapters/take_while.rs b/library/core/src/iter/adapters/take_while.rs index ded216da952a3..ec66dc3aec360 100644 --- a/library/core/src/iter/adapters/take_while.rs +++ b/library/core/src/iter/adapters/take_while.rs @@ -94,19 +94,7 @@ where } } - #[inline] - fn fold(mut self, init: Acc, fold: Fold) -> Acc - where - Self: Sized, - Fold: FnMut(Acc, Self::Item) -> Acc, - { - #[inline] - fn ok(mut f: impl FnMut(B, T) -> B) -> impl FnMut(B, T) -> Result { - move |acc, x| Ok(f(acc, x)) - } - - self.try_fold(init, ok(fold)).unwrap() - } + impl_fold_via_try_fold! { fold -> try_fold } } #[stable(feature = "fused", since = "1.26.0")] diff --git a/library/core/src/iter/mod.rs b/library/core/src/iter/mod.rs index 9514466bd0c05..ef0f397825b15 100644 --- a/library/core/src/iter/mod.rs +++ b/library/core/src/iter/mod.rs @@ -352,6 +352,29 @@ #![stable(feature = "rust1", since = "1.0.0")] +// This needs to be up here in order to be usable in the child modules +macro_rules! impl_fold_via_try_fold { + (fold -> try_fold) => { + impl_fold_via_try_fold! { @internal fold -> try_fold } + }; + (rfold -> try_rfold) => { + impl_fold_via_try_fold! { @internal rfold -> try_rfold } + }; + (@internal $fold:ident -> $try_fold:ident) => { + #[inline] + fn $fold(mut self, init: AAA, mut fold: FFF) -> AAA + where + FFF: FnMut(AAA, Self::Item) -> AAA, + { + use crate::const_closure::ConstFnMutClosure; + use crate::ops::NeverShortCircuit; + + let fold = ConstFnMutClosure::new(&mut fold, NeverShortCircuit::wrap_mut_2_imp); + self.$try_fold(init, fold).0 + } + }; +} + #[stable(feature = "rust1", since = "1.0.0")] pub use self::traits::Iterator; diff --git a/library/core/src/iter/range.rs b/library/core/src/iter/range.rs index f7aeee8c9adc6..ac7b389b15b4d 100644 --- a/library/core/src/iter/range.rs +++ b/library/core/src/iter/range.rs @@ -1150,19 +1150,7 @@ impl Iterator for ops::RangeInclusive { self.spec_try_fold(init, f) } - #[inline] - fn fold(mut self, init: B, f: F) -> B - where - Self: Sized, - F: FnMut(B, Self::Item) -> B, - { - #[inline] - fn ok(mut f: impl FnMut(B, T) -> B) -> impl FnMut(B, T) -> Result { - move |acc, x| Ok(f(acc, x)) - } - - self.try_fold(init, ok(f)).unwrap() - } + impl_fold_via_try_fold! { fold -> try_fold } #[inline] fn last(mut self) -> Option { @@ -1230,19 +1218,7 @@ impl DoubleEndedIterator for ops::RangeInclusive { self.spec_try_rfold(init, f) } - #[inline] - fn rfold(mut self, init: B, f: F) -> B - where - Self: Sized, - F: FnMut(B, Self::Item) -> B, - { - #[inline] - fn ok(mut f: impl FnMut(B, T) -> B) -> impl FnMut(B, T) -> Result { - move |acc, x| Ok(f(acc, x)) - } - - self.try_rfold(init, ok(f)).unwrap() - } + impl_fold_via_try_fold! { rfold -> try_rfold } } // Safety: See above implementation for `ops::Range` From 595e192274b581e3a3a938c84eb128fa8c20c60d Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Thu, 29 Sep 2022 15:40:04 +0200 Subject: [PATCH 2/7] unsafe keyword: trait examples and unsafe_op_in_unsafe_fn update --- library/std/src/keyword_docs.rs | 152 +++++++++++++++++++++++++------- 1 file changed, 120 insertions(+), 32 deletions(-) diff --git a/library/std/src/keyword_docs.rs b/library/std/src/keyword_docs.rs index a4b0522b05024..6121054bd746a 100644 --- a/library/std/src/keyword_docs.rs +++ b/library/std/src/keyword_docs.rs @@ -1867,11 +1867,15 @@ mod type_keyword {} /// Code or interfaces whose [memory safety] cannot be verified by the type /// system. /// -/// The `unsafe` keyword has two uses: to declare the existence of contracts the -/// compiler can't check (`unsafe fn` and `unsafe trait`), and to declare that a -/// programmer has checked that these contracts have been upheld (`unsafe {}` -/// and `unsafe impl`, but also `unsafe fn` -- see below). They are not mutually -/// exclusive, as can be seen in `unsafe fn`. +/// The `unsafe` keyword has two uses: +/// - to declare the existence of contracts the compiler can't check (`unsafe fn` and `unsafe +/// trait`), +/// - and to declare that a programmer has checked that these contracts have been upheld (`unsafe +/// {}` and `unsafe impl`, but also `unsafe fn` -- see below). +/// +/// They are not mutually exclusive, as can be seen in `unsafe fn`: the body of an `unsafe fn` is, +/// by default, treated like an unsafe block. The `unsafe_op_in_unsafe_fn` lint can be enabled to +/// change that. /// /// # Unsafe abilities /// @@ -1914,12 +1918,12 @@ mod type_keyword {} /// - `unsafe impl`: the contract necessary to implement the trait has been /// checked by the programmer and is guaranteed to be respected. /// -/// `unsafe fn` also acts like an `unsafe {}` block +/// By default, `unsafe fn` also acts like an `unsafe {}` block /// around the code inside the function. This means it is not just a signal to /// the caller, but also promises that the preconditions for the operations -/// inside the function are upheld. Mixing these two meanings can be confusing -/// and [proposal]s exist to use `unsafe {}` blocks inside such functions when -/// making `unsafe` operations. +/// inside the function are upheld. Mixing these two meanings can be confusing, so the +/// `unsafe_op_in_unsafe_fn` lint can be enabled to warn against that and require explicit unsafe +/// blocks even inside `unsafe fn`. /// /// See the [Rustnomicon] and the [Reference] for more information. /// @@ -1987,13 +1991,15 @@ mod type_keyword {} /// /// ```rust /// # #![allow(dead_code)] +/// #![deny(unsafe_op_in_unsafe_fn)] /// /// Dereference the given pointer. /// /// /// /// # Safety /// /// /// /// `ptr` must be aligned and must not be dangling. /// unsafe fn deref_unchecked(ptr: *const i32) -> i32 { -/// *ptr +/// // SAFETY: the caller is required to ensure that `ptr` is aligned and dereferenceable. +/// unsafe { *ptr } /// } /// /// let a = 3; @@ -2003,35 +2009,118 @@ mod type_keyword {} /// unsafe { assert_eq!(*b, deref_unchecked(b)); }; /// ``` /// -/// Traits marked as `unsafe` must be [`impl`]emented using `unsafe impl`. This -/// makes a guarantee to other `unsafe` code that the implementation satisfies -/// the trait's safety contract. The [Send] and [Sync] traits are examples of -/// this behaviour in the standard library. +/// ## `unsafe` and traits +/// +/// The interactions of `unsafe` and traits can be surprising, so let us contrast the +/// two combinations of safe `fn` in `unsafe trait` and `unsafe fn` in safe trait using two +/// examples: /// /// ```rust -/// /// Implementors of this trait must guarantee an element is always -/// /// accessible with index 3. -/// unsafe trait ThreeIndexable { -/// /// Returns a reference to the element with index 3 in `&self`. -/// fn three(&self) -> &T; +/// /// # Safety +/// /// +/// /// `make_even` must return an even number. +/// unsafe trait MakeEven { +/// fn make_even(&self) -> i32; /// } /// -/// // The implementation of `ThreeIndexable` for `[T; 4]` is `unsafe` -/// // because the implementor must abide by a contract the compiler cannot -/// // check but as a programmer we know there will always be a valid element -/// // at index 3 to access. -/// unsafe impl ThreeIndexable for [T; 4] { -/// fn three(&self) -> &T { -/// // SAFETY: implementing the trait means there always is an element -/// // with index 3 accessible. -/// unsafe { self.get_unchecked(3) } -/// } +/// // SAFETY: Our `make_even` always returns something even. +/// unsafe impl MakeEven for i32 { +/// fn make_even(&self) -> i32 { +/// self << 1 +/// } +/// } +/// +/// fn use_make_even(x: impl MakeEven) { +/// if x.make_even() % 2 == 1 { +/// // SAFETY: this can never happen, because all `MakeEven` implementations +/// // ensure that `make_even` returns something even. +/// unsafe { std::hint::unreachable_unchecked() }; +/// } +/// } +/// ``` +/// +/// Note how the safety contract of the trait is upheld by the implementation, and is itself used to +/// uphold the safety contract of the unsafe function `unreachable_unchecked` called by +/// `use_make_even`. `make_even` itself is a safe function because its *callers* do not have to +/// worry about any contract, only the *implementation* of `MakeEven` is required to uphold a +/// certain contract. `use_make_even` is safe because it can use the promise made by `MakeEven` +/// implementations to uphold the safety contract of the `unsafe fn unreachable_unchecked` it calls. +/// +/// It is also possible to have `unsafe fn` in a regular safe `trait`: +/// +/// ```rust +/// # #![feature(never_type)] +/// #![deny(unsafe_op_in_unsafe_fn)] +/// +/// trait Indexable { +/// const LEN: usize; +/// +/// /// # Safety +/// /// +/// /// The caller must ensure that `idx < LEN`. +/// unsafe fn idx_unchecked(&self, idx: usize) -> i32; +/// } +/// +/// // The implementation for `i32` doesn't need to do any contract reasoning. +/// impl Indexable for i32 { +/// const LEN: usize = 1; +/// +/// unsafe fn idx_unchecked(&self, idx: usize) -> i32 { +/// debug_assert_eq!(idx, 0); +/// *self +/// } +/// } +/// +/// // The implementation for arrays exploits the function contract to +/// // make use of `get_unchecked` on slices and avoid a run-time check. +/// impl Indexable for [i32; 42] { +/// const LEN: usize = 42; +/// +/// unsafe fn idx_unchecked(&self, idx: usize) -> i32 { +/// // SAFETY: As per this trait's documentation, the caller ensures +/// // that `idx < 42`. +/// unsafe { *self.get_unchecked(idx) } +/// } +/// } +/// +/// // The implementation for the never type declares a length of 0, +/// // which means `idx_unchecked` can never be called. +/// impl Indexable for ! { +/// const LEN: usize = 0; +/// +/// unsafe fn idx_unchecked(&self, idx: usize) -> i32 { +/// // SAFETY: As per this trait's documentation, the caller ensures +/// // that `idx < 0`, which is impossible, so this is dead code. +/// unsafe { std::hint::unreachable_unchecked() } +/// } /// } /// -/// let a = [1, 2, 4, 8]; -/// assert_eq!(a.three(), &8); +/// fn use_indexable(x: I, idx: usize) -> i32 { +/// if idx < I::LEN { +/// // SAFETY: We have checked that `idx < I::LEN`. +/// unsafe { x.idx_unchecked(idx) } +/// } else { +/// panic!("index out-of-bounds") +/// } +/// } /// ``` /// +/// This time, `use_indexable` is safe because it uses a run-time check to discharge the safety +/// contract of `idx_unchecked`. Implementing `Indexable` is safe because when writing +/// `idx_unchecked`, we don't have to worry: our *callers* need to discharge a proof obligation +/// (like `use_indexable` does), but the *implementation* of `get_unchecked` has no proof obligation +/// to contend with. Of course, the implementation of `Indexable` may choose to call other unsafe +/// operations, and then it needs an `unsafe` *block* to indicate it discharged the proof +/// obligations of its callees. (We enabled `unsafe_op_in_unsafe_fn`, so the body of `idx_unchecked` +/// is not implicitly an unsafe block.) For that purpose it can make use of the contract that all +/// its callers must uphold -- the fact that `idx < LEN`. +/// +/// Formally speaking, an `unsafe fn` in a trait is a function with extra +/// *preconditions* (such as `idx < LEN`), whereas an `unsafe trait` can declare +/// that some of its functions have extra *postconditions* (such as returning an +/// even integer). If a trait needs a function with both extra precondition and +/// extra postcondition, then it needs an `unsafe fn` in an `unsafe trait`. +/// /// [`extern`]: keyword.extern.html /// [`trait`]: keyword.trait.html /// [`static`]: keyword.static.html @@ -2043,7 +2132,6 @@ mod type_keyword {} /// [nomicon-soundness]: ../nomicon/safe-unsafe-meaning.html /// [soundness]: https://rust-lang.github.io/unsafe-code-guidelines/glossary.html#soundness-of-code--of-a-library /// [Reference]: ../reference/unsafety.html -/// [proposal]: https://github.com/rust-lang/rfcs/pull/2585 /// [discussion on Rust Internals]: https://internals.rust-lang.org/t/what-does-unsafe-mean/6696 mod unsafe_keyword {} From 414319468bc57ccba8be27e25581dd053469e27c Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Fri, 7 Oct 2022 02:29:19 +0000 Subject: [PATCH 3/7] Check WhereClauseReferencesSelf after all other object safety checks --- .../src/traits/object_safety.rs | 28 ++++++++++--------- src/test/ui/object-safety/issue-102762.rs | 26 +++++++++++++++++ src/test/ui/object-safety/issue-102762.stderr | 20 +++++++++++++ 3 files changed, 61 insertions(+), 13 deletions(-) create mode 100644 src/test/ui/object-safety/issue-102762.rs create mode 100644 src/test/ui/object-safety/issue-102762.stderr diff --git a/compiler/rustc_trait_selection/src/traits/object_safety.rs b/compiler/rustc_trait_selection/src/traits/object_safety.rs index 8f87a7fdeba5e..b6f8f51bdf491 100644 --- a/compiler/rustc_trait_selection/src/traits/object_safety.rs +++ b/compiler/rustc_trait_selection/src/traits/object_safety.rs @@ -447,19 +447,6 @@ fn virtual_call_violation_for_method<'tcx>( return Some(MethodViolationCode::Generic); } - if tcx - .predicates_of(method.def_id) - .predicates - .iter() - // A trait object can't claim to live more than the concrete type, - // so outlives predicates will always hold. - .cloned() - .filter(|(p, _)| p.to_opt_type_outlives().is_none()) - .any(|pred| contains_illegal_self_type_reference(tcx, trait_def_id, pred)) - { - return Some(MethodViolationCode::WhereClauseReferencesSelf); - } - let receiver_ty = tcx.liberate_late_bound_regions(method.def_id, sig.input(0)); // Until `unsized_locals` is fully implemented, `self: Self` can't be dispatched on. @@ -538,6 +525,21 @@ fn virtual_call_violation_for_method<'tcx>( } } + // NOTE: This check happens last, because it results in a lint, and not a + // hard error. + if tcx + .predicates_of(method.def_id) + .predicates + .iter() + // A trait object can't claim to live more than the concrete type, + // so outlives predicates will always hold. + .cloned() + .filter(|(p, _)| p.to_opt_type_outlives().is_none()) + .any(|pred| contains_illegal_self_type_reference(tcx, trait_def_id, pred)) + { + return Some(MethodViolationCode::WhereClauseReferencesSelf); + } + None } diff --git a/src/test/ui/object-safety/issue-102762.rs b/src/test/ui/object-safety/issue-102762.rs new file mode 100644 index 0000000000000..4f4c3634528a1 --- /dev/null +++ b/src/test/ui/object-safety/issue-102762.rs @@ -0,0 +1,26 @@ +// compile-flags: --crate-type=lib +// This test checks that the `where_clauses_object_safety` lint does not cause +// other object safety *hard errors* to be suppressed, because we currently +// only emit one object safety error per trait... + +use std::future::Future; +use std::pin::Pin; + +pub trait Fetcher: Send + Sync { + fn get<'a>(self: &'a Box) -> Pin> + 'a>> + where + Self: Sync, + { + todo!() + } +} + +fn fetcher() -> Box { + //~^ ERROR the trait `Fetcher` cannot be made into an object + todo!() +} + +pub fn foo() { + let fetcher = fetcher(); + let _ = fetcher.get(); +} diff --git a/src/test/ui/object-safety/issue-102762.stderr b/src/test/ui/object-safety/issue-102762.stderr new file mode 100644 index 0000000000000..5041ebe776051 --- /dev/null +++ b/src/test/ui/object-safety/issue-102762.stderr @@ -0,0 +1,20 @@ +error[E0038]: the trait `Fetcher` cannot be made into an object + --> $DIR/issue-102762.rs:18:21 + | +LL | fn get<'a>(self: &'a Box) -> Pin> + 'a>> + | ------------- help: consider changing method `get`'s `self` parameter to be `&self`: `&Self` +... +LL | fn fetcher() -> Box { + | ^^^^^^^^^^^ `Fetcher` cannot be made into an object + | +note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit + --> $DIR/issue-102762.rs:10:22 + | +LL | pub trait Fetcher: Send + Sync { + | ------- this trait cannot be made into an object... +LL | fn get<'a>(self: &'a Box) -> Pin> + 'a>> + | ^^^^^^^^^^^^^ ...because method `get`'s `self` parameter cannot be dispatched on + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0038`. From 95ae993bd86b97aff9a27498f2187fef431cab58 Mon Sep 17 00:00:00 2001 From: Ben Kimock Date: Thu, 6 Oct 2022 20:09:54 -0400 Subject: [PATCH 4/7] Avoid defensive re-initialization of the BufReader buffer --- library/std/src/io/buffered/bufreader.rs | 8 +++++++ .../std/src/io/buffered/bufreader/buffer.rs | 19 ++++++++++++--- library/std/src/io/buffered/tests.rs | 24 +++++++++++++++++++ 3 files changed, 48 insertions(+), 3 deletions(-) diff --git a/library/std/src/io/buffered/bufreader.rs b/library/std/src/io/buffered/bufreader.rs index 88ad92d8a9859..4f339a18a480e 100644 --- a/library/std/src/io/buffered/bufreader.rs +++ b/library/std/src/io/buffered/bufreader.rs @@ -224,6 +224,14 @@ impl BufReader { } } +// This is only used by a test which asserts that the initialization-tracking is correct. +#[cfg(test)] +impl BufReader { + pub fn initialized(&self) -> usize { + self.buf.initialized() + } +} + impl BufReader { /// Seeks relative to the current position. If the new position lies within the buffer, /// the buffer will not be flushed, allowing for more efficient seeks. diff --git a/library/std/src/io/buffered/bufreader/buffer.rs b/library/std/src/io/buffered/bufreader/buffer.rs index 867c22c6041e7..e9e29d60ca282 100644 --- a/library/std/src/io/buffered/bufreader/buffer.rs +++ b/library/std/src/io/buffered/bufreader/buffer.rs @@ -20,13 +20,19 @@ pub struct Buffer { // Each call to `fill_buf` sets `filled` to indicate how many bytes at the start of `buf` are // initialized with bytes from a read. filled: usize, + // This is the max number of bytes returned across all `fill_buf` calls. We track this so that we + // can accurately tell `read_buf` how many bytes of buf are initialized, to bypass as much of its + // defensive initialization as possible. Note that while this often the same as `filled`, it + // doesn't need to be. Calls to `fill_buf` are not required to actually fill the buffer, and + // omitting this is a huge perf regression for `Read` impls that do not. + initialized: usize, } impl Buffer { #[inline] pub fn with_capacity(capacity: usize) -> Self { let buf = Box::new_uninit_slice(capacity); - Self { buf, pos: 0, filled: 0 } + Self { buf, pos: 0, filled: 0, initialized: 0 } } #[inline] @@ -51,6 +57,12 @@ impl Buffer { self.pos } + // This is only used by a test which asserts that the initialization-tracking is correct. + #[cfg(test)] + pub fn initialized(&self) -> usize { + self.initialized + } + #[inline] pub fn discard_buffer(&mut self) { self.pos = 0; @@ -96,13 +108,14 @@ impl Buffer { let mut buf = BorrowedBuf::from(&mut *self.buf); // SAFETY: `self.filled` bytes will always have been initialized. unsafe { - buf.set_init(self.filled); + buf.set_init(self.initialized); } reader.read_buf(buf.unfilled())?; - self.filled = buf.len(); self.pos = 0; + self.filled = buf.len(); + self.initialized = buf.init_len(); } Ok(self.buffer()) } diff --git a/library/std/src/io/buffered/tests.rs b/library/std/src/io/buffered/tests.rs index bd6d95242ad94..f4e688eb926cc 100644 --- a/library/std/src/io/buffered/tests.rs +++ b/library/std/src/io/buffered/tests.rs @@ -1039,3 +1039,27 @@ fn single_formatted_write() { writeln!(&mut writer, "{}, {}!", "hello", "world").unwrap(); assert_eq!(writer.get_ref().events, [RecordedEvent::Write("hello, world!\n".to_string())]); } + +#[test] +fn bufreader_full_initialize() { + struct OneByteReader; + impl Read for OneByteReader { + fn read(&mut self, buf: &mut [u8]) -> crate::io::Result { + if buf.len() > 0 { + buf[0] = 0; + Ok(1) + } else { + Ok(0) + } + } + } + let mut reader = BufReader::new(OneByteReader); + // Nothing is initialized yet. + assert_eq!(reader.initialized(), 0); + + let buf = reader.fill_buf().unwrap(); + // We read one byte... + assert_eq!(buf.len(), 1); + // But we initialized the whole buffer! + assert_eq!(reader.initialized(), reader.capacity()); +} From fa767868dfba9eb1f2161cea789bc4d9828d53e2 Mon Sep 17 00:00:00 2001 From: Takayuki Maeda Date: Fri, 7 Oct 2022 21:10:08 +0900 Subject: [PATCH 5/7] fix a ICE #102768 --- .../rustc_hir_analysis/src/collect/type_of.rs | 6 ++-- .../generic_const_exprs/issue-102768.rs | 14 ++++++++ .../generic_const_exprs/issue-102768.stderr | 33 +++++++++++++++++++ 3 files changed, 51 insertions(+), 2 deletions(-) create mode 100644 src/test/ui/const-generics/generic_const_exprs/issue-102768.rs create mode 100644 src/test/ui/const-generics/generic_const_exprs/issue-102768.stderr diff --git a/compiler/rustc_hir_analysis/src/collect/type_of.rs b/compiler/rustc_hir_analysis/src/collect/type_of.rs index f8a62c8491076..5972f6b88008e 100644 --- a/compiler/rustc_hir_analysis/src/collect/type_of.rs +++ b/compiler/rustc_hir_analysis/src/collect/type_of.rs @@ -493,8 +493,10 @@ pub(super) fn type_of(tcx: TyCtxt<'_>, def_id: DefId) -> Ty<'_> { }, def_id.to_def_id(), ); - if let Some(assoc_item) = assoc_item { - tcx.type_of(tcx.generics_of(assoc_item.def_id).params[idx].def_id) + if let Some(param) + = assoc_item.map(|item| &tcx.generics_of(item.def_id).params[idx]).filter(|param| param.kind.is_ty_or_const()) + { + tcx.type_of(param.def_id) } else { // FIXME(associated_const_equality): add a useful error message here. tcx.ty_error_with_message( diff --git a/src/test/ui/const-generics/generic_const_exprs/issue-102768.rs b/src/test/ui/const-generics/generic_const_exprs/issue-102768.rs new file mode 100644 index 0000000000000..7aea0d30d1a76 --- /dev/null +++ b/src/test/ui/const-generics/generic_const_exprs/issue-102768.rs @@ -0,0 +1,14 @@ +#![feature(generic_const_exprs)] +#![allow(incomplete_features)] + +trait X { + type Y<'a>; +} + +const _: () = { + fn f2<'a>(arg: Box = &'a ()>>) {} + //~^ ERROR this associated type takes 1 lifetime argument but 0 lifetime arguments + //~| ERROR this associated type takes 0 generic arguments but 1 generic argument +}; + +fn main() {} diff --git a/src/test/ui/const-generics/generic_const_exprs/issue-102768.stderr b/src/test/ui/const-generics/generic_const_exprs/issue-102768.stderr new file mode 100644 index 0000000000000..9deb9b26588f8 --- /dev/null +++ b/src/test/ui/const-generics/generic_const_exprs/issue-102768.stderr @@ -0,0 +1,33 @@ +error[E0107]: this associated type takes 1 lifetime argument but 0 lifetime arguments were supplied + --> $DIR/issue-102768.rs:9:30 + | +LL | fn f2<'a>(arg: Box = &'a ()>>) {} + | ^ expected 1 lifetime argument + | +note: associated type defined here, with 1 lifetime parameter: `'a` + --> $DIR/issue-102768.rs:5:10 + | +LL | type Y<'a>; + | ^ -- +help: add missing lifetime argument + | +LL | fn f2<'a>(arg: Box = &'a ()>>) {} + | +++ + +error[E0107]: this associated type takes 0 generic arguments but 1 generic argument was supplied + --> $DIR/issue-102768.rs:9:30 + | +LL | fn f2<'a>(arg: Box = &'a ()>>) {} + | ^--- help: remove these generics + | | + | expected 0 generic arguments + | +note: associated type defined here, with 0 generic parameters + --> $DIR/issue-102768.rs:5:10 + | +LL | type Y<'a>; + | ^ + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0107`. From 3d93c3848a4fa85487f930f93deb687d155761d6 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 7 Oct 2022 14:39:07 +0200 Subject: [PATCH 6/7] run Miri CI when std::sys changes --- src/ci/scripts/should-skip-this.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/ci/scripts/should-skip-this.sh b/src/ci/scripts/should-skip-this.sh index 67ff00cc1dfe2..60c2960b160ec 100755 --- a/src/ci/scripts/should-skip-this.sh +++ b/src/ci/scripts/should-skip-this.sh @@ -19,9 +19,12 @@ if [[ -n "${CI_ONLY_WHEN_SUBMODULES_CHANGED-}" ]]; then # those files are present in the diff a submodule was updated. echo "Submodules were updated" elif ! (git diff --quiet "$BASE_COMMIT" -- \ - src/tools/clippy src/tools/rustfmt src/tools/miri); then + src/tools/clippy src/tools/rustfmt src/tools/miri + library/std/src/sys); then # There is not an easy blanket search for subtrees. For now, manually list # the subtrees. + # Also run this when the platform-specific parts of std change, in case + # that breaks Miri. echo "Tool subtrees were updated" elif ! (git diff --quiet "$BASE_COMMIT" -- \ src/test/rustdoc-gui \ From c30dcff97a43292b729f384f847febe777daf629 Mon Sep 17 00:00:00 2001 From: Ralf Jung Date: Fri, 7 Oct 2022 15:21:47 +0200 Subject: [PATCH 7/7] review feedback --- library/std/src/keyword_docs.rs | 85 +++++++++++++++++---------------- 1 file changed, 43 insertions(+), 42 deletions(-) diff --git a/library/std/src/keyword_docs.rs b/library/std/src/keyword_docs.rs index 6121054bd746a..e35145c4ade48 100644 --- a/library/std/src/keyword_docs.rs +++ b/library/std/src/keyword_docs.rs @@ -1992,6 +1992,7 @@ mod type_keyword {} /// ```rust /// # #![allow(dead_code)] /// #![deny(unsafe_op_in_unsafe_fn)] +/// /// /// Dereference the given pointer. /// /// /// /// # Safety @@ -2020,22 +2021,22 @@ mod type_keyword {} /// /// /// /// `make_even` must return an even number. /// unsafe trait MakeEven { -/// fn make_even(&self) -> i32; +/// fn make_even(&self) -> i32; /// } /// /// // SAFETY: Our `make_even` always returns something even. /// unsafe impl MakeEven for i32 { -/// fn make_even(&self) -> i32 { -/// self << 1 -/// } +/// fn make_even(&self) -> i32 { +/// self << 1 +/// } /// } /// /// fn use_make_even(x: impl MakeEven) { -/// if x.make_even() % 2 == 1 { -/// // SAFETY: this can never happen, because all `MakeEven` implementations -/// // ensure that `make_even` returns something even. -/// unsafe { std::hint::unreachable_unchecked() }; -/// } +/// if x.make_even() % 2 == 1 { +/// // SAFETY: this can never happen, because all `MakeEven` implementations +/// // ensure that `make_even` returns something even. +/// unsafe { std::hint::unreachable_unchecked() }; +/// } /// } /// ``` /// @@ -2053,55 +2054,55 @@ mod type_keyword {} /// #![deny(unsafe_op_in_unsafe_fn)] /// /// trait Indexable { -/// const LEN: usize; +/// const LEN: usize; /// -/// /// # Safety -/// /// -/// /// The caller must ensure that `idx < LEN`. -/// unsafe fn idx_unchecked(&self, idx: usize) -> i32; +/// /// # Safety +/// /// +/// /// The caller must ensure that `idx < LEN`. +/// unsafe fn idx_unchecked(&self, idx: usize) -> i32; /// } /// /// // The implementation for `i32` doesn't need to do any contract reasoning. /// impl Indexable for i32 { -/// const LEN: usize = 1; +/// const LEN: usize = 1; /// -/// unsafe fn idx_unchecked(&self, idx: usize) -> i32 { -/// debug_assert_eq!(idx, 0); -/// *self -/// } +/// unsafe fn idx_unchecked(&self, idx: usize) -> i32 { +/// debug_assert_eq!(idx, 0); +/// *self +/// } /// } /// /// // The implementation for arrays exploits the function contract to /// // make use of `get_unchecked` on slices and avoid a run-time check. /// impl Indexable for [i32; 42] { -/// const LEN: usize = 42; +/// const LEN: usize = 42; /// -/// unsafe fn idx_unchecked(&self, idx: usize) -> i32 { -/// // SAFETY: As per this trait's documentation, the caller ensures -/// // that `idx < 42`. -/// unsafe { *self.get_unchecked(idx) } -/// } +/// unsafe fn idx_unchecked(&self, idx: usize) -> i32 { +/// // SAFETY: As per this trait's documentation, the caller ensures +/// // that `idx < 42`. +/// unsafe { *self.get_unchecked(idx) } +/// } /// } /// /// // The implementation for the never type declares a length of 0, /// // which means `idx_unchecked` can never be called. /// impl Indexable for ! { -/// const LEN: usize = 0; +/// const LEN: usize = 0; /// -/// unsafe fn idx_unchecked(&self, idx: usize) -> i32 { -/// // SAFETY: As per this trait's documentation, the caller ensures -/// // that `idx < 0`, which is impossible, so this is dead code. -/// unsafe { std::hint::unreachable_unchecked() } -/// } +/// unsafe fn idx_unchecked(&self, idx: usize) -> i32 { +/// // SAFETY: As per this trait's documentation, the caller ensures +/// // that `idx < 0`, which is impossible, so this is dead code. +/// unsafe { std::hint::unreachable_unchecked() } +/// } /// } /// /// fn use_indexable(x: I, idx: usize) -> i32 { -/// if idx < I::LEN { -/// // SAFETY: We have checked that `idx < I::LEN`. -/// unsafe { x.idx_unchecked(idx) } -/// } else { -/// panic!("index out-of-bounds") -/// } +/// if idx < I::LEN { +/// // SAFETY: We have checked that `idx < I::LEN`. +/// unsafe { x.idx_unchecked(idx) } +/// } else { +/// panic!("index out-of-bounds") +/// } /// } /// ``` /// @@ -2115,11 +2116,11 @@ mod type_keyword {} /// is not implicitly an unsafe block.) For that purpose it can make use of the contract that all /// its callers must uphold -- the fact that `idx < LEN`. /// -/// Formally speaking, an `unsafe fn` in a trait is a function with extra -/// *preconditions* (such as `idx < LEN`), whereas an `unsafe trait` can declare -/// that some of its functions have extra *postconditions* (such as returning an -/// even integer). If a trait needs a function with both extra precondition and -/// extra postcondition, then it needs an `unsafe fn` in an `unsafe trait`. +/// Formally speaking, an `unsafe fn` in a trait is a function with *preconditions* that go beyond +/// those encoded by the argument types (such as `idx < LEN`), whereas an `unsafe trait` can declare +/// that some of its functions have *postconditions* that go beyond those encoded in the return type +/// (such as returning an even integer). If a trait needs a function with both extra precondition +/// and extra postcondition, then it needs an `unsafe fn` in an `unsafe trait`. /// /// [`extern`]: keyword.extern.html /// [`trait`]: keyword.trait.html