From 0e7eca77e13b0021d3763e1470cd15a6c25bf80b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Mi=C4=85sko?= Date: Mon, 23 May 2022 00:00:00 +0000 Subject: [PATCH 1/7] Fix precise field capture of univariant enums When constructing a MIR from a THIR field expression, introduce an additional downcast projection before accessing a field of an enum. When rebasing a place builder on top of a captured place, account for the fact that a single HIR enum field projection corresponds to two MIR projection elements: a downcast element and a field element. --- compiler/rustc_middle/src/thir.rs | 4 +- compiler/rustc_middle/src/thir/visit.rs | 2 +- .../src/build/expr/as_place.rs | 59 +++++++++++++++---- compiler/rustc_mir_build/src/thir/cx/expr.rs | 14 ++--- .../capture-enum-field.rs | 27 +++++++++ 5 files changed, 85 insertions(+), 21 deletions(-) create mode 100644 src/test/ui/closures/2229_closure_analysis/capture-enum-field.rs diff --git a/compiler/rustc_middle/src/thir.rs b/compiler/rustc_middle/src/thir.rs index b99e7573000c9..c3b79917dda91 100644 --- a/compiler/rustc_middle/src/thir.rs +++ b/compiler/rustc_middle/src/thir.rs @@ -317,9 +317,11 @@ pub enum ExprKind<'tcx> { lhs: ExprId, rhs: ExprId, }, - /// Access to a struct or tuple field. + /// Access to a field of a struct, a tuple, an union, or an enum. Field { lhs: ExprId, + /// Variant containing the field. + variant_index: VariantIdx, /// This can be a named (`.foo`) or unnamed (`.0`) field. name: Field, }, diff --git a/compiler/rustc_middle/src/thir/visit.rs b/compiler/rustc_middle/src/thir/visit.rs index f57569522d58d..8c8ebb0a6b87a 100644 --- a/compiler/rustc_middle/src/thir/visit.rs +++ b/compiler/rustc_middle/src/thir/visit.rs @@ -80,7 +80,7 @@ pub fn walk_expr<'a, 'tcx: 'a, V: Visitor<'a, 'tcx>>(visitor: &mut V, expr: &Exp visitor.visit_expr(&visitor.thir()[lhs]); visitor.visit_expr(&visitor.thir()[rhs]); } - Field { lhs, name: _ } => visitor.visit_expr(&visitor.thir()[lhs]), + Field { lhs, variant_index: _, name: _ } => visitor.visit_expr(&visitor.thir()[lhs]), Index { lhs, index } => { visitor.visit_expr(&visitor.thir()[lhs]); visitor.visit_expr(&visitor.thir()[index]); diff --git a/compiler/rustc_mir_build/src/build/expr/as_place.rs b/compiler/rustc_mir_build/src/build/expr/as_place.rs index 045d6eb1c3021..438a68e71f054 100644 --- a/compiler/rustc_mir_build/src/build/expr/as_place.rs +++ b/compiler/rustc_mir_build/src/build/expr/as_place.rs @@ -5,6 +5,7 @@ use crate::build::ForGuard::{OutsideGuard, RefWithinGuard}; use crate::build::{BlockAnd, BlockAndExtension, Builder}; use rustc_hir::def_id::DefId; use rustc_hir::HirId; +use rustc_middle::hir::place::Projection as HirProjection; use rustc_middle::hir::place::ProjectionKind as HirProjectionKind; use rustc_middle::middle::region; use rustc_middle::mir::AssertKind::BoundsCheck; @@ -268,20 +269,52 @@ fn to_upvars_resolved_place_builder<'a, 'tcx>( ty::UpvarCapture::ByValue => upvar_resolved_place_builder, }; - let next_projection = capture.place.projections.len(); - let mut curr_projections = from_builder.projection; - // We used some of the projections to build the capture itself, // now we apply the remaining to the upvar resolved place. - upvar_resolved_place_builder - .projection - .extend(curr_projections.drain(next_projection..)); + let remaining_projections = strip_prefix( + capture.place.base_ty, + from_builder.projection, + &capture.place.projections, + ); + upvar_resolved_place_builder.projection.extend(remaining_projections); Ok(upvar_resolved_place_builder) } } } +/// Returns projections remaining after stripping an initial prefix of HIR +/// projections. +/// +/// Supports only HIR projection kinds that represent a path that might be +/// captured by a closure or a generator, i.e., an `Index` or a `Subslice` +/// projection kinds are unsupported. +fn strip_prefix<'tcx>( + mut base_ty: Ty<'tcx>, + projections: Vec>, + prefix_projections: &[HirProjection<'tcx>], +) -> impl Iterator> { + let mut iter = projections.into_iter(); + for projection in prefix_projections { + match projection.kind { + HirProjectionKind::Deref => { + assert!(matches!(iter.next(), Some(ProjectionElem::Deref))); + } + HirProjectionKind::Field(..) => { + if base_ty.is_enum() { + assert!(matches!(iter.next(), Some(ProjectionElem::Downcast(..)))); + } + assert!(matches!(iter.next(), Some(ProjectionElem::Field(..)))); + } + HirProjectionKind::Index | HirProjectionKind::Subslice => { + bug!("unexpected projection kind: {:?}", projection); + } + } + base_ty = projection.ty; + } + iter +} + impl<'tcx> PlaceBuilder<'tcx> { pub(crate) fn into_place<'a>( self, @@ -438,11 +471,15 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { this.expr_as_place(block, &this.thir[value], mutability, fake_borrow_temps) }) } - ExprKind::Field { lhs, name } => { - let place_builder = unpack!( - block = - this.expr_as_place(block, &this.thir[lhs], mutability, fake_borrow_temps,) - ); + ExprKind::Field { lhs, variant_index, name } => { + let lhs = &this.thir[lhs]; + let mut place_builder = + unpack!(block = this.expr_as_place(block, lhs, mutability, fake_borrow_temps,)); + if let ty::Adt(adt_def, _) = lhs.ty.kind() { + if adt_def.is_enum() { + place_builder = place_builder.downcast(*adt_def, variant_index); + } + } block.and(place_builder.field(name, expr.ty)) } ExprKind::Deref { arg } => { diff --git a/compiler/rustc_mir_build/src/thir/cx/expr.rs b/compiler/rustc_mir_build/src/thir/cx/expr.rs index 480c5b195cc2d..bd9f599fff0a1 100644 --- a/compiler/rustc_mir_build/src/thir/cx/expr.rs +++ b/compiler/rustc_mir_build/src/thir/cx/expr.rs @@ -591,6 +591,7 @@ impl<'tcx> Cx<'tcx> { } hir::ExprKind::Field(ref source, ..) => ExprKind::Field { lhs: self.mirror_expr(source), + variant_index: VariantIdx::new(0), name: Field::new(tcx.field_index(expr.hir_id, self.typeck_results)), }, hir::ExprKind::Cast(ref source, ref cast_ty) => { @@ -994,14 +995,11 @@ impl<'tcx> Cx<'tcx> { HirProjectionKind::Deref => { ExprKind::Deref { arg: self.thir.exprs.push(captured_place_expr) } } - HirProjectionKind::Field(field, ..) => { - // Variant index will always be 0, because for multi-variant - // enums, we capture the enum entirely. - ExprKind::Field { - lhs: self.thir.exprs.push(captured_place_expr), - name: Field::new(field as usize), - } - } + HirProjectionKind::Field(field, variant_index) => ExprKind::Field { + lhs: self.thir.exprs.push(captured_place_expr), + variant_index, + name: Field::new(field as usize), + }, HirProjectionKind::Index | HirProjectionKind::Subslice => { // We don't capture these projections, so we can ignore them here continue; diff --git a/src/test/ui/closures/2229_closure_analysis/capture-enum-field.rs b/src/test/ui/closures/2229_closure_analysis/capture-enum-field.rs new file mode 100644 index 0000000000000..bbe3aa31a98df --- /dev/null +++ b/src/test/ui/closures/2229_closure_analysis/capture-enum-field.rs @@ -0,0 +1,27 @@ +// edition:2021 +// run-pass + +#[derive(Debug, PartialEq, Eq)] +pub enum Color { + RGB(u8, u8, u8), +} + +fn main() { + let mut color = Color::RGB(0, 0, 0); + let mut red = |v| { + let Color::RGB(ref mut r, _, _) = color; + *r = v; + }; + let mut green = |v| { + let Color::RGB(_, ref mut g, _) = color; + *g = v; + }; + let mut blue = |v| { + let Color::RGB(_, _, ref mut b) = color; + *b = v; + }; + red(1); + green(2); + blue(3); + assert_eq!(Color::RGB(1, 2, 3), color); +} From 2e8508fdd509658734960fee18146f38c5a5d619 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauren=C8=9Biu=20Nicola?= Date: Tue, 7 Jun 2022 08:52:15 +0300 Subject: [PATCH 2/7] :arrow_up: rust-analyzer --- src/tools/rust-analyzer | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tools/rust-analyzer b/src/tools/rust-analyzer index f94fa62d69faf..ad6810e90bf89 160000 --- a/src/tools/rust-analyzer +++ b/src/tools/rust-analyzer @@ -1 +1 @@ -Subproject commit f94fa62d69faf5bd63b3772d3ec4f0c76cf2db57 +Subproject commit ad6810e90bf89a4ef0ae21349d077050bc2a4fa2 From 83af085c777fb71e49a132a6a713aa93fb4f2019 Mon Sep 17 00:00:00 2001 From: Nilstrieb <48135649+Nilstrieb@users.noreply.github.com> Date: Tue, 7 Jun 2022 09:53:44 +0200 Subject: [PATCH 3/7] Remove confusing sentence from `Mutex` docs The docs were saying something about "statically initializing" the mutex, and it's not clear what this means. Remove that part to avoid confusion. --- library/std/src/sync/mutex.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/library/std/src/sync/mutex.rs b/library/std/src/sync/mutex.rs index 3d8281fe59389..69b025a58f496 100644 --- a/library/std/src/sync/mutex.rs +++ b/library/std/src/sync/mutex.rs @@ -10,11 +10,10 @@ use crate::sys_common::mutex as sys; /// A mutual exclusion primitive useful for protecting shared data /// /// This mutex will block threads waiting for the lock to become available. The -/// mutex can also be statically initialized or created via a [`new`] -/// constructor. Each mutex has a type parameter which represents the data that -/// it is protecting. The data can only be accessed through the RAII guards -/// returned from [`lock`] and [`try_lock`], which guarantees that the data is only -/// ever accessed when the mutex is locked. +/// mutex can created via a [`new`] constructor. Each mutex has a type parameter +/// which represents the data that it is protecting. The data can only be accessed +/// through the RAII guards returned from [`lock`] and [`try_lock`], which +/// guarantees that the data is only ever accessed when the mutex is locked. /// /// # Poisoning /// From 232c2d6c256eed76f24bd6cf249a8462ab297060 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 7 Jun 2022 11:49:22 +0200 Subject: [PATCH 4/7] Update help message for rustdoc-gui runner --- src/tools/rustdoc-gui/tester.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/tools/rustdoc-gui/tester.js b/src/tools/rustdoc-gui/tester.js index 4599e12de5f33..a512185036915 100644 --- a/src/tools/rustdoc-gui/tester.js +++ b/src/tools/rustdoc-gui/tester.js @@ -16,6 +16,7 @@ function showHelp() { console.log(" --debug : show extra information about script run"); console.log(" --show-text : render font in pages"); console.log(" --no-headless : disable headless mode"); + console.log(" --no-sandbox : disable sandbox mode"); console.log(" --help : show this message then quit"); console.log(" --tests-folder [PATH] : location of the .GOML tests folder"); console.log(" --jobs [NUMBER] : number of threads to run tests on"); From 5799e7de160ee5c4a9c9ab1121f7e5b70ccfc187 Mon Sep 17 00:00:00 2001 From: Guillaume Gomez Date: Tue, 7 Jun 2022 11:49:35 +0200 Subject: [PATCH 5/7] Update rustdoc-gui README --- src/test/rustdoc-gui/README.md | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/src/test/rustdoc-gui/README.md b/src/test/rustdoc-gui/README.md index 8efe7a578b8f0..d9854e2e71548 100644 --- a/src/test/rustdoc-gui/README.md +++ b/src/test/rustdoc-gui/README.md @@ -11,14 +11,24 @@ You can find more information and its documentation in its [repository][browser- If you need to have more information on the tests run, you can use `--test-args`: ```bash -$ ./x.py test src/test/rustdoc-gui --stage 1 --jobs 8 --test-args --debug +$ ./x.py test src/test/rustdoc-gui --stage 1 --test-args --debug ``` -There are three options supported: +If you don't want to run in headless mode (helpful to debug sometimes), you can use +`--no-headless`: - * `--debug`: allows to see puppeteer commands. - * `--no-headless`: disable headless mode so you can see what's going on. - * `--show-text`: by default, text isn't rendered because of issues with fonts, it enables it back. +```bash +$ ./x.py test src/test/rustdoc-gui --stage 1 --test-args --no-headless +``` + +To see the supported options, use `--help`. + +Important to be noted: if the chromium instance crashes when you run it, you might need to +use `--no-sandbox` to make it work: + +```bash +$ ./x.py test src/test/rustdoc-gui --stage 1 --test-args --no-sandbox +``` [browser-ui-test]: https://github.com/GuillaumeGomez/browser-UI-test/ [puppeteer]: https://pptr.dev/ From e224185409bd20b91c5eb0652748283658d7c4dd Mon Sep 17 00:00:00 2001 From: Dylan DPC <99973273+Dylan-DPC@users.noreply.github.com> Date: Tue, 7 Jun 2022 15:15:19 +0200 Subject: [PATCH 6/7] Update library/std/src/sync/mutex.rs Co-authored-by: Weiyi Wang --- library/std/src/sync/mutex.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/std/src/sync/mutex.rs b/library/std/src/sync/mutex.rs index 69b025a58f496..b2fbb77204aa0 100644 --- a/library/std/src/sync/mutex.rs +++ b/library/std/src/sync/mutex.rs @@ -10,7 +10,7 @@ use crate::sys_common::mutex as sys; /// A mutual exclusion primitive useful for protecting shared data /// /// This mutex will block threads waiting for the lock to become available. The -/// mutex can created via a [`new`] constructor. Each mutex has a type parameter +/// mutex can be created via a [`new`] constructor. Each mutex has a type parameter /// which represents the data that it is protecting. The data can only be accessed /// through the RAII guards returned from [`lock`] and [`try_lock`], which /// guarantees that the data is only ever accessed when the mutex is locked. From 0dda42bc143309e3036341298414269351627528 Mon Sep 17 00:00:00 2001 From: Nilstrieb <48135649+Nilstrieb@users.noreply.github.com> Date: Mon, 11 Apr 2022 21:31:41 +0200 Subject: [PATCH 7/7] Improve the safety docs for `CStr` Namely, the two functions `from_ptr` and `from_bytes_with_nul_unchecked`. Before, this functions didn't state the requirements clearly enough, and I was not immediately able to find them like for other functions. This doesn't change the content of the docs, but simply rewords them for clarity. --- library/core/src/ffi/c_str.rs | 37 +++++++++++++++++++++++++---------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/library/core/src/ffi/c_str.rs b/library/core/src/ffi/c_str.rs index ca335d53c7caa..264f4da5aa66e 100644 --- a/library/core/src/ffi/c_str.rs +++ b/library/core/src/ffi/c_str.rs @@ -196,20 +196,32 @@ impl CStr { /// allows inspection and interoperation of non-owned C strings. The total /// size of the raw C string must be smaller than `isize::MAX` **bytes** /// in memory due to calling the `slice::from_raw_parts` function. - /// This method is unsafe for a number of reasons: /// - /// * There is no guarantee to the validity of `ptr`. - /// * The returned lifetime is not guaranteed to be the actual lifetime of - /// `ptr`. - /// * There is no guarantee that the memory pointed to by `ptr` contains a - /// valid nul terminator byte at the end of the string. - /// * It is not guaranteed that the memory pointed by `ptr` won't change - /// before the `CStr` has been destroyed. + /// # Safety + /// + /// * The memory pointed to by `ptr` must contain a valid nul terminator at the + /// end of the string. + /// + /// * `ptr` must be [valid] for reads of bytes up to and including the null terminator. + /// This means in particular: + /// + /// * The entire memory range of this `CStr` must be contained within a single allocated object! + /// * `ptr` must be non-null even for a zero-length cstr. + /// + /// * The memory referenced by the returned `CStr` must not be mutated for + /// the duration of lifetime `'a`. /// /// > **Note**: This operation is intended to be a 0-cost cast but it is /// > currently implemented with an up-front calculation of the length of /// > the string. This is not guaranteed to always be the case. /// + /// # Caveat + /// + /// The lifetime for the returned slice is inferred from its usage. To prevent accidental misuse, + /// it's suggested to tie the lifetime to whichever source lifetime is safe in the context, + /// such as by providing a helper function taking the lifetime of a host value for the slice, + /// or by explicit annotation. + /// /// # Examples /// /// ```ignore (extern-declaration) @@ -227,6 +239,8 @@ impl CStr { /// } /// # } /// ``` + /// + /// [valid]: core::ptr#safety #[inline] #[must_use] #[stable(feature = "rust1", since = "1.0.0")] @@ -349,8 +363,11 @@ impl CStr { /// Unsafely creates a C string wrapper from a byte slice. /// /// This function will cast the provided `bytes` to a `CStr` wrapper without - /// performing any sanity checks. The provided slice **must** be nul-terminated - /// and not contain any interior nul bytes. + /// performing any sanity checks. + /// + /// # Safety + /// The provided slice **must** be nul-terminated and not contain any interior + /// nul bytes. /// /// # Examples ///