optimize sanity check path printing

During the sanity check, we keep track of the path we are below in a `Vec`.  We
avoid cloning that `Vec` unless we hit a pointer indirection.  The `String`
representation is only computed when validation actually fails.

Author: RalfJung

src/librustc_lint/builtin.rs

@@ -1622,13 +1622,13 @@ fn validate_const<'a, 'tcx>(
         let layout = ecx.layout_of(constant.ty)?;
         let place = ecx.allocate_op(OpTy { op, layout })?.into();
 
-        let mut todo = vec![(place, String::new())];
+        let mut todo = vec![(place, Vec::new())];
         let mut seen = FxHashSet();
         seen.insert(place);
-        while let Some((place, path)) = todo.pop() {
+        while let Some((place, mut path)) = todo.pop() {
             ecx.validate_mplace(
                 place,
-                path,
+                &mut path,
                 &mut seen,
                 &mut todo,
             )?;

src/librustc_mir/interpret/validity.rs

@@ -1,5 +1,6 @@
 use std::fmt::Write;
 
+use syntax_pos::symbol::Symbol;
 use rustc::ty::layout::{self, Size, Primitive};
 use rustc::ty::{self, Ty};
 use rustc_data_structures::fx::FxHashSet;
@@ -13,21 +14,23 @@ use super::{
 
 macro_rules! validation_failure{
     ($what:expr, $where:expr, $details:expr) => {{
-        let where_ = if $where.is_empty() {
+        let where_ = path_format($where);
+        let where_ = if where_.is_empty() {
             String::new()
         } else {
-            format!(" at {}", $where)
+            format!(" at {}", where_)
         };
         err!(ValidationFailure(format!(
             "encountered {}{}, but expected {}",
             $what, where_, $details,
         )))
     }};
     ($what:expr, $where:expr) => {{
-        let where_ = if $where.is_empty() {
+        let where_ = path_format($where);
+        let where_ = if where_.is_empty() {
             String::new()
         } else {
-            format!(" at {}", $where)
+            format!(" at {}", where_)
         };
         err!(ValidationFailure(format!(
             "encountered {}{}",
@@ -36,13 +39,59 @@ macro_rules! validation_failure{
     }};
 }
 
+/// We want to show a nice path to the invalid field for diagnotsics,
+/// but avoid string operations in the happy case where no error happens.
+/// So we track a `Vec<PathElem>` where `PathElem` contains all the data we
+/// need to later print something for the user.
+#[derive(Copy, Clone, Debug)]
+pub enum PathElem {
+    Field(Symbol),
+    ClosureVar(Symbol),
+    ArrayElem(usize),
+    TupleElem(usize),
+    Deref,
+    Tag,
+}
+
+// Adding a Deref and making a copy of the path to be put into the queue
+// always go together.  This one does it with only new allocation.
+fn path_clone_and_deref(path: &Vec<PathElem>) -> Vec<PathElem> {
+    let mut new_path = Vec::with_capacity(path.len()+1);
+    new_path.clone_from(path);
+    new_path.push(PathElem::Deref);
+    new_path
+}
+
+/// Format a path
+fn path_format(path: &Vec<PathElem>) -> String {
+    use self::PathElem::*;
+
+    let mut out = String::new();
+    for elem in path.iter() {
+        match elem {
+            Field(name) => write!(out, ".{}", name).unwrap(),
+            ClosureVar(name) => write!(out, ".<closure-var({})>", name).unwrap(),
+            TupleElem(idx) => write!(out, ".{}", idx).unwrap(),
+            ArrayElem(idx) => write!(out, "[{}]", idx).unwrap(),
+            Deref =>
+                // This does not match Rust syntax, but it is more readable for long paths -- and
+                // some of the other items here also are not Rust syntax.  Actually we can't
+                // even use the usual syntax because we are just showing the projections,
+                // not the root.
+                write!(out, ".<deref>").unwrap(),
+            Tag => write!(out, ".<enum-tag>").unwrap(),
+        }
+    }
+    out
+}
+
 impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
     fn validate_scalar(
         &self,
         value: ScalarMaybeUndef,
         size: Size,
         scalar: &layout::Scalar,
-        path: &str,
+        path: &Vec<PathElem>,
         ty: Ty,
     ) -> EvalResult<'tcx> {
         trace!("validate scalar: {:#?}, {:#?}, {:#?}, {}", value, size, scalar, ty);
@@ -93,7 +142,11 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
             ty::TyChar => {
                 debug_assert_eq!(size.bytes(), 4);
                 if ::std::char::from_u32(bits as u32).is_none() {
-                    return err!(InvalidChar(bits));
+                    return validation_failure!(
+                        "character",
+                        path,
+                        "a valid unicode codepoint"
+                    );
                 }
             }
             _ => {},
@@ -127,46 +180,57 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
     /// This function checks the memory where `dest` points to.  The place must be sized
     /// (i.e., dest.extra == PlaceExtra::None).
     /// It will error if the bits at the destination do not match the ones described by the layout.
+    /// The `path` may be pushed to, but the part that is present when the function
+    /// starts must not be changed!
     pub fn validate_mplace(
         &self,
         dest: MPlaceTy<'tcx>,
-        path: String,
+        path: &mut Vec<PathElem>,
         seen: &mut FxHashSet<(MPlaceTy<'tcx>)>,
-        todo: &mut Vec<(MPlaceTy<'tcx>, String)>,
+        todo: &mut Vec<(MPlaceTy<'tcx>, Vec<PathElem>)>,
     ) -> EvalResult<'tcx> {
         self.memory.dump_alloc(dest.to_ptr()?.alloc_id);
         trace!("validate_mplace: {:?}, {:#?}", *dest, dest.layout);
 
-        // Find the right variant
+        // Find the right variant.  We have to handle this as a prelude, not via
+        // proper recursion with the new inner layout, to be able to later nicely
+        // print the field names of the enum field that is being accessed.
         let (variant, dest) = match dest.layout.variants {
             layout::Variants::NicheFilling { niche: ref tag, .. } |
             layout::Variants::Tagged { ref tag, .. } => {
                 let size = tag.value.size(self);
                 // we first read the tag value as scalar, to be able to validate it
                 let tag_mplace = self.mplace_field(dest, 0)?;
                 let tag_value = self.read_scalar(tag_mplace.into())?;
-                let path = format!("{}.TAG", path);
+                path.push(PathElem::Tag);
                 self.validate_scalar(
                     tag_value, size, tag, &path, tag_mplace.layout.ty
                 )?;
+                path.pop(); // remove the element again
                 // then we read it again to get the index, to continue
                 let variant = self.read_discriminant_as_variant_index(dest.into())?;
-                let dest = self.mplace_downcast(dest, variant)?;
+                let inner_dest = self.mplace_downcast(dest, variant)?;
+                // Put the variant projection onto the path, as a field
+                path.push(PathElem::Field(dest.layout.ty.ty_adt_def().unwrap().variants[variant].name));
                 trace!("variant layout: {:#?}", dest.layout);
-                (variant, dest)
+                (variant, inner_dest)
             },
             layout::Variants::Single { index } => {
                 (index, dest)
             }
         };
 
+        // Remember the length, in case we need to truncate
+        let path_len = path.len();
+
         // Validate all fields
         match dest.layout.fields {
             // primitives are unions with zero fields
             layout::FieldPlacement::Union(0) => {
                 match dest.layout.abi {
                     // nothing to do, whatever the pointer points to, it is never going to be read
-                    layout::Abi::Uninhabited => validation_failure!("a value of an uninhabited type", path),
+                    layout::Abi::Uninhabited =>
+                        return validation_failure!("a value of an uninhabited type", path),
                     // check that the scalar is a valid pointer or that its bit range matches the
                     // expectation.
                     layout::Abi::Scalar(ref scalar_layout) => {
@@ -179,8 +243,8 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
                             if let Scalar::Ptr(ptr) = scalar.not_undef()? {
                                 let alloc_kind = self.tcx.alloc_map.lock().get(ptr.alloc_id);
                                 if let Some(AllocType::Static(did)) = alloc_kind {
-                                    // statics from other crates are already checked
-                                    // extern statics should not be validated as they have no body
+                                    // statics from other crates are already checked.
+                                    // extern statics should not be validated as they have no body.
                                     if !did.is_local() || self.tcx.is_foreign_item(did) {
                                         return Ok(());
                                     }
@@ -190,12 +254,11 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
                                     let ptr_place = self.ref_to_mplace(value)?;
                                     // we have not encountered this pointer+layout combination before
                                     if seen.insert(ptr_place) {
-                                        todo.push((ptr_place, format!("(*{})", path)))
+                                        todo.push((ptr_place, path_clone_and_deref(path)));
                                     }
                                 }
                             }
                         }
-                        Ok(())
                     },
                     _ => bug!("bad abi for FieldPlacement::Union(0): {:#?}", dest.layout.abi),
                 }
@@ -204,16 +267,14 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
                 // We can't check unions, their bits are allowed to be anything.
                 // The fields don't need to correspond to any bit pattern of the union's fields.
                 // See https://github.com/rust-lang/rust/issues/32836#issuecomment-406875389
-                Ok(())
             },
             layout::FieldPlacement::Array { .. } => {
                 for (i, field) in self.mplace_array_fields(dest)?.enumerate() {
                     let field = field?;
-                    let mut path = path.clone();
-                    self.dump_field_name(&mut path, dest.layout.ty, i as usize, variant).unwrap();
+                    path.push(PathElem::ArrayElem(i));
                     self.validate_mplace(field, path, seen, todo)?;
+                    path.truncate(path_len);
                 }
-                Ok(())
             },
             layout::FieldPlacement::Arbitrary { ref offsets, .. } => {
                 // fat pointers need special treatment
@@ -232,9 +293,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
                         assert_eq!(ptr.extra, PlaceExtra::Length(len));
                         let unpacked_ptr = self.unpack_unsized_mplace(ptr)?;
                         if seen.insert(unpacked_ptr) {
-                            let mut path = path.clone();
-                            self.dump_field_name(&mut path, dest.layout.ty, 0, 0).unwrap();
-                            todo.push((unpacked_ptr, path))
+                            todo.push((unpacked_ptr, path_clone_and_deref(path)));
                         }
                     },
                     Some(ty::TyDynamic(..)) => {
@@ -249,9 +308,7 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
                         assert_eq!(ptr.extra, PlaceExtra::Vtable(vtable));
                         let unpacked_ptr = self.unpack_unsized_mplace(ptr)?;
                         if seen.insert(unpacked_ptr) {
-                            let mut path = path.clone();
-                            self.dump_field_name(&mut path, dest.layout.ty, 0, 0).unwrap();
-                            todo.push((unpacked_ptr, path))
+                            todo.push((unpacked_ptr, path_clone_and_deref(path)));
                         }
                         // FIXME: More checks for the vtable... making sure it is exactly
                         // the one one would expect for this type.
@@ -261,84 +318,42 @@ impl<'a, 'mir, 'tcx, M: Machine<'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M> {
                     None => {
                         // Not a pointer, perform regular aggregate handling below
                         for i in 0..offsets.len() {
-                            let mut path = path.clone();
-                            self.dump_field_name(&mut path, dest.layout.ty, i, variant).unwrap();
                             let field = self.mplace_field(dest, i as u64)?;
+                            path.push(self.aggregate_field_path_elem(dest.layout.ty, variant, i));
                             self.validate_mplace(field, path, seen, todo)?;
+                            path.truncate(path_len);
                         }
                         // FIXME: For a TyStr, check that this is valid UTF-8.
                     },
                 }
-
-                Ok(())
             }
         }
+        Ok(())
     }
 
-    fn dump_field_name(&self, s: &mut String, ty: Ty<'tcx>, i: usize, variant: usize) -> ::std::fmt::Result {
+    fn aggregate_field_path_elem(&self, ty: Ty<'tcx>, variant: usize, field: usize) -> PathElem {
         match ty.sty {
-            ty::TyBool |
-            ty::TyChar |
-            ty::TyInt(_) |
-            ty::TyUint(_) |
-            ty::TyFloat(_) |
-            ty::TyFnPtr(_) |
-            ty::TyNever |
-            ty::TyFnDef(..) |
-            ty::TyGeneratorWitness(..) |
-            ty::TyForeign(..) |
-            ty::TyDynamic(..) => {
-                bug!("field_name({:?}): not applicable", ty)
-            }
-
-            // Potentially-fat pointers.
-            ty::TyRef(_, pointee, _) |
-            ty::TyRawPtr(ty::TypeAndMut { ty: pointee, .. }) => {
-                assert!(i < 2);
-
-                // Reuse the fat *T type as its own thin pointer data field.
-                // This provides information about e.g. DST struct pointees
-                // (which may have no non-DST form), and will work as long
-                // as the `Abi` or `FieldPlacement` is checked by users.
-                if i == 0 {
-                    return write!(s, ".data_ptr");
-                }
-
-                match self.tcx.struct_tail(pointee).sty {
-                    ty::TySlice(_) |
-                    ty::TyStr => write!(s, ".len"),
-                    ty::TyDynamic(..) => write!(s, ".vtable_ptr"),
-                    _ => bug!("field_name({:?}): not applicable", ty)
-                }
-            }
-
-            // Arrays and slices.
-            ty::TyArray(_, _) |
-            ty::TySlice(_) |
-            ty::TyStr => write!(s, "[{}]", i),
-
             // generators and closures.
             ty::TyClosure(def_id, _) | ty::TyGenerator(def_id, _, _) => {
                 let node_id = self.tcx.hir.as_local_node_id(def_id).unwrap();
-                let freevar = self.tcx.with_freevars(node_id, |fv| fv[i]);
-                write!(s, ".upvar({})", self.tcx.hir.name(freevar.var_id()))
+                let freevar = self.tcx.with_freevars(node_id, |fv| fv[field]);
+                PathElem::ClosureVar(self.tcx.hir.name(freevar.var_id()))
             }
 
-            ty::TyTuple(_) => write!(s, ".{}", i),
+            // tuples
+            ty::TyTuple(_) => PathElem::TupleElem(field),
 
             // enums
             ty::TyAdt(def, ..) if def.is_enum() => {
                 let variant = &def.variants[variant];
-                write!(s, ".{}::{}", variant.name, variant.fields[i].ident)
+                PathElem::Field(variant.fields[field].ident.name)
             }
 
-            // other ADTs.
-            ty::TyAdt(def, _) => write!(s, ".{}", def.non_enum_variant().fields[i].ident),
+            // other ADTs
+            ty::TyAdt(def, _) => PathElem::Field(def.non_enum_variant().fields[field].ident.name),
 
-            ty::TyProjection(_) | ty::TyAnon(..) | ty::TyParam(_) |
-            ty::TyInfer(_) | ty::TyError => {
-                bug!("dump_field_name: unexpected type `{}`", ty)
-            }
+            // nothing else has an aggregate layout
+            _ => bug!("aggregate_field_path_elem: got non-aggregate type {:?}", ty),
         }
     }
 }

src/test/ui/consts/const-eval/double_check2.stderr

@@ -5,7 +5,7 @@ LL | / static FOO: (&Foo, &Bar) = unsafe {( //~ undefined behavior
 LL | |     Union { usize: &BAR }.foo,
 LL | |     Union { usize: &BAR }.bar,
 LL | | )};
-   | |___^ type validation failed: encountered 5 at (*.1).TAG, but expected something in the range 42..=99
+   | |___^ type validation failed: encountered 5 at .1.<deref>.<enum-tag>, but expected something in the range 42..=99
    |
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior