refactor unsafety checking of places

Author: RalfJung

compiler/rustc_middle/src/mir/mod.rs

@@ -1744,7 +1744,9 @@ impl<'tcx> Place<'tcx> {
 
     /// Iterate over the projections in evaluation order, i.e., the first element is the base with
     /// its projection and then subsequently more projections are added.
-    pub fn iter_projections(self) -> impl Iterator<Item=(PlaceRef<'tcx>, PlaceElem<'tcx>)> + DoubleEndedIterator {
+    pub fn iter_projections(
+        self,
+    ) -> impl Iterator<Item = (PlaceRef<'tcx>, PlaceElem<'tcx>)> + DoubleEndedIterator {
         self.projection.iter().enumerate().map(move |(i, proj)| {
             let base = PlaceRef { local: self.local, projection: &self.projection[..i] };
             (base, proj)

compiler/rustc_mir/src/transform/check_unsafety.rs

@@ -181,6 +181,9 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
             self.check_mut_borrowing_layout_constrained_field(*place, context.is_mutating_use());
         }
 
+        // Check for borrows to packed fields.
+        // `is_disaligned` already traverses the place to consider all projections after the last
+        // `Deref`, so this only needs to be called once at the top level.
         if context.is_borrow() {
             if util::is_disaligned(self.tcx, self.body, self.param_env, *place) {
                 self.require_unsafe(
@@ -190,99 +193,105 @@ impl<'a, 'tcx> Visitor<'tcx> for UnsafetyChecker<'a, 'tcx> {
             }
         }
 
-        for (i, _elem) in place.projection.iter().enumerate() {
-            let proj_base = &place.projection[..i];
-            if context.is_borrow() {
-                if util::is_disaligned(self.tcx, self.body, self.param_env, *place) {
+        // Some checks below need the extra metainfo of the local declaration.
+        let decl = &self.body.local_decls[place.local];
+
+        // Check the base local: it might be an unsafe-to-access static. We only check derefs of the
+        // temporary holding the static pointer to avoid duplicate errors
+        // <https://github.com/rust-lang/rust/pull/78068#issuecomment-731753506>.
+        if decl.internal && place.projection.first() == Some(&ProjectionElem::Deref) {
+            // If the projection root is an artifical local that we introduced when
+            // desugaring `static`, give a more specific error message
+            // (avoid the general "raw pointer" clause below, that would only be confusing).
+            if let Some(box LocalInfo::StaticRef { def_id, .. }) = decl.local_info {
+                if self.tcx.is_mutable_static(def_id) {
                     self.require_unsafe(
-                        UnsafetyViolationKind::BorrowPacked,
-                        UnsafetyViolationDetails::BorrowOfPackedField,
+                        UnsafetyViolationKind::General,
+                        UnsafetyViolationDetails::UseOfMutableStatic,
                     );
+                    return;
+                } else if self.tcx.is_foreign_item(def_id) {
+                    self.require_unsafe(
+                        UnsafetyViolationKind::General,
+                        UnsafetyViolationDetails::UseOfExternStatic,
+                    );
+                    return;
                 }
             }
-            let source_info = self.source_info;
-            if let [] = proj_base {
-                let decl = &self.body.local_decls[place.local];
-                if decl.internal {
-                    // If the projection root is an artifical local that we introduced when
-                    // desugaring `static`, give a more specific error message
-                    // (avoid the general "raw pointer" clause below, that would only be confusing).
-                    if let Some(box LocalInfo::StaticRef { def_id, .. }) = decl.local_info {
-                        if self.tcx.is_mutable_static(def_id) {
-                            self.require_unsafe(
-                                UnsafetyViolationKind::General,
-                                UnsafetyViolationDetails::UseOfMutableStatic,
-                            );
-                            return;
-                        } else if self.tcx.is_foreign_item(def_id) {
-                            self.require_unsafe(
-                                UnsafetyViolationKind::General,
-                                UnsafetyViolationDetails::UseOfExternStatic,
-                            );
-                            return;
-                        }
-                    } else {
-                        // Internal locals are used in the `move_val_init` desugaring.
-                        // We want to check unsafety against the source info of the
-                        // desugaring, rather than the source info of the RHS.
-                        self.source_info = self.body.local_decls[place.local].source_info;
-                    }
+        }
+
+        // Check for raw pointer `Deref`.
+        for (base, proj) in place.iter_projections() {
+            if proj == ProjectionElem::Deref {
+                let source_info = self.source_info; // Backup source_info so we can restore it later.
+                if base.projection.is_empty() && decl.internal {
+                    // Internal locals are used in the `move_val_init` desugaring.
+                    // We want to check unsafety against the source info of the
+                    // desugaring, rather than the source info of the RHS.
+                    self.source_info = self.body.local_decls[place.local].source_info;
                 }
+                let base_ty = base.ty(self.body, self.tcx).ty;
+                if base_ty.is_unsafe_ptr() {
+                    self.require_unsafe(
+                        UnsafetyViolationKind::GeneralAndConstFn,
+                        UnsafetyViolationDetails::DerefOfRawPointer,
+                    )
+                }
+                self.source_info = source_info; // Restore backed-up source_info.
+            }
+        }
+
+        // Check for union fields. For this we traverse right-to-left, as the last `Deref` changes
+        // whether we *read* the union field or potentially *write* to it (if this place is being assigned to).
+        let mut saw_deref = false;
+        for (base, proj) in place.iter_projections().rev() {
+            if proj == ProjectionElem::Deref {
+                saw_deref = true;
+                continue;
             }
-            let base_ty = Place::ty_from(place.local, proj_base, self.body, self.tcx).ty;
-            match base_ty.kind() {
-                ty::RawPtr(..) => self.require_unsafe(
-                    UnsafetyViolationKind::GeneralAndConstFn,
-                    UnsafetyViolationDetails::DerefOfRawPointer,
-                ),
-                ty::Adt(adt, _) if adt.is_union() => {
-                    let assign_to_field = matches!(
+
+            let base_ty = base.ty(self.body, self.tcx).ty;
+            if base_ty.ty_adt_def().map_or(false, |adt| adt.is_union()) {
+                // If we did not hit a `Deref` yet and the overall place use is an assignment, the
+                // rules are different.
+                let assign_to_field = !saw_deref
+                    && matches!(
                         context,
                         PlaceContext::MutatingUse(
                             MutatingUseContext::Store
                                 | MutatingUseContext::Drop
                                 | MutatingUseContext::AsmOutput
                         )
                     );
-                    // If there is a `Deref` further along the projection chain, this is *not* an
-                    // assignment to a union field. In that case the union field is just read to
-                    // obtain the pointer/reference.
-                    let assign_to_field = assign_to_field
-                        && !place.projection[i..]
-                            .iter()
-                            .any(|elem| matches!(elem, ProjectionElem::Deref));
-                    // If this is just an assignment, determine if the assigned type needs dropping.
-                    if assign_to_field {
-                        // We have to check the actual type of the assignment, as that determines if the
-                        // old value is being dropped.
-                        let assigned_ty = place.ty(&self.body.local_decls, self.tcx).ty;
-                        // To avoid semver hazard, we only consider `Copy` and `ManuallyDrop` non-dropping.
-                        let manually_drop = assigned_ty
-                            .ty_adt_def()
-                            .map_or(false, |adt_def| adt_def.is_manually_drop());
-                        let nodrop = manually_drop
-                            || assigned_ty.is_copy_modulo_regions(
-                                self.tcx.at(self.source_info.span),
-                                self.param_env,
-                            );
-                        if !nodrop {
-                            self.require_unsafe(
-                                UnsafetyViolationKind::GeneralAndConstFn,
-                                UnsafetyViolationDetails::AssignToDroppingUnionField,
-                            );
-                        } else {
-                            // write to non-drop union field, safe
-                        }
-                    } else {
+                // If this is just an assignment, determine if the assigned type needs dropping.
+                if assign_to_field {
+                    // We have to check the actual type of the assignment, as that determines if the
+                    // old value is being dropped.
+                    let assigned_ty = place.ty(&self.body.local_decls, self.tcx).ty;
+                    // To avoid semver hazard, we only consider `Copy` and `ManuallyDrop` non-dropping.
+                    let manually_drop = assigned_ty
+                        .ty_adt_def()
+                        .map_or(false, |adt_def| adt_def.is_manually_drop());
+                    let nodrop = manually_drop
+                        || assigned_ty.is_copy_modulo_regions(
+                            self.tcx.at(self.source_info.span),
+                            self.param_env,
+                        );
+                    if !nodrop {
                         self.require_unsafe(
                             UnsafetyViolationKind::GeneralAndConstFn,
-                            UnsafetyViolationDetails::AccessToUnionField,
-                        )
+                            UnsafetyViolationDetails::AssignToDroppingUnionField,
+                        );
+                    } else {
+                        // write to non-drop union field, safe
                     }
+                } else {
+                    self.require_unsafe(
+                        UnsafetyViolationKind::GeneralAndConstFn,
+                        UnsafetyViolationDetails::AccessToUnionField,
+                    )
                 }
-                _ => {}
             }
-            self.source_info = source_info;
         }
     }
 }