finally this actually looks like a visitor

2 files changed, 118 insertions, 132 deletions

diff --git a/src/librustc_mir/interpret/validity.rs b/src/librustc_mir/interpret/validity.rs
index 45645a714d0..c303d2a1e67 100644
--- a/src/librustc_mir/interpret/validity.rs
+++ b/src/librustc_mir/interpret/validity.rs
@@ -8,12 +8,12 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.
 
-use std::fmt::{self, Write};
+use std::fmt::Write;
 use std::hash::Hash;
 
 use syntax_pos::symbol::Symbol;
 use rustc::ty::layout::{self, Size, Align, TyLayout, LayoutOf};
-use rustc::ty::{self, TyCtxt};
+use rustc::ty;
 use rustc_data_structures::fx::FxHashSet;
 use rustc::mir::interpret::{
     Scalar, AllocType, EvalResult, EvalErrorKind
@@ -122,24 +122,17 @@ fn path_format(path: &Vec<PathElem>) -> String {
     out
 }
 
-struct ValidityVisitor<'rt, 'a, 'tcx: 'a+'rt, Tag: 'static> {
-    op: OpTy<'tcx, Tag>,
+struct ValidityVisitor<'rt, 'a: 'rt, 'mir: 'rt, 'tcx: 'a+'rt+'mir, M: Machine<'a, 'mir, 'tcx>+'rt> {
     /// The `path` may be pushed to, but the part that is present when a function
     /// starts must not be changed!  `visit_fields` and `visit_array` rely on
     /// this stack discipline.
     path: Vec<PathElem>,
-    ref_tracking: Option<&'rt mut RefTracking<'tcx, Tag>>,
+    ref_tracking: Option<&'rt mut RefTracking<'tcx, M::PointerTag>>,
     const_mode: bool,
-    tcx: TyCtxt<'a, 'tcx, 'tcx>,
+    ecx: &'rt mut EvalContext<'a, 'mir, 'tcx, M>,
 }
 
-impl<Tag: fmt::Debug> fmt::Debug for ValidityVisitor<'_, '_, '_, Tag> {
-    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "{:?}, {:?}", *self.op, self.op.layout.ty)
-    }
-}
-
-impl<'rt, 'a, 'tcx, Tag> ValidityVisitor<'rt, 'a, 'tcx, Tag> {
+impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> ValidityVisitor<'rt, 'a, 'mir, 'tcx, M> {
     fn push_aggregate_field_path_elem(
         &mut self,
         layout: TyLayout<'tcx>,
@@ -148,7 +141,7 @@ impl<'rt, 'a, 'tcx, Tag> ValidityVisitor<'rt, 'a, 'tcx, Tag> {
         let elem = match layout.ty.sty {
             // generators and closures.
             ty::Closure(def_id, _) | ty::Generator(def_id, _, _) => {
-                if let Some(upvar) = self.tcx.optimized_mir(def_id).upvar_decls.get(field) {
+                if let Some(upvar) = self.ecx.tcx.optimized_mir(def_id).upvar_decls.get(field) {
                     PathElem::ClosureVar(upvar.debug_name)
                 } else {
                     // Sometimes the index is beyond the number of freevars (seen
@@ -190,41 +183,38 @@ impl<'rt, 'a, 'tcx, Tag> ValidityVisitor<'rt, 'a, 'tcx, Tag> {
 }
 
 impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>
-    ValueVisitor<'a, 'mir, 'tcx, M> for ValidityVisitor<'rt, 'a, 'tcx, M::PointerTag>
+    ValueVisitor<'a, 'mir, 'tcx, M> for ValidityVisitor<'rt, 'a, 'mir, 'tcx, M>
 {
     type V = OpTy<'tcx, M::PointerTag>;
 
     #[inline(always)]
-    fn value(&self) -> &OpTy<'tcx, M::PointerTag> {
-        &self.op
+    fn ecx(&mut self) -> &mut EvalContext<'a, 'mir, 'tcx, M> {
+        &mut self.ecx
     }
 
     #[inline]
     fn visit_field(
         &mut self,
-        ectx: &mut EvalContext<'a, 'mir, 'tcx, M>,
-        val: Self::V,
+        old_op: OpTy<'tcx, M::PointerTag>,
         field: usize,
+        new_op: OpTy<'tcx, M::PointerTag>
     ) -> EvalResult<'tcx> {
         // Remember the old state
         let path_len = self.path.len();
-        let op = self.op;
         // Perform operation
-        self.push_aggregate_field_path_elem(op.layout, field);
-        self.op = val;
-        self.visit_value(ectx)?;
+        self.push_aggregate_field_path_elem(old_op.layout, field);
+        self.visit_value(new_op)?;
         // Undo changes
         self.path.truncate(path_len);
-        self.op = op;
         Ok(())
     }
 
     #[inline]
-    fn visit_value(&mut self, ectx: &mut EvalContext<'a, 'mir, 'tcx, M>)
-        -> EvalResult<'tcx>
+    fn visit_value(&mut self, op: OpTy<'tcx, M::PointerTag>) -> EvalResult<'tcx>
     {
+        trace!("visit_value: {:?}, {:?}", *op, op.layout);
         // Translate enum discriminant errors to something nicer.
-        match self.walk_value(ectx) {
+        match self.walk_value(op) {
             Ok(()) => Ok(()),
             Err(err) => match err.kind {
                 EvalErrorKind::InvalidDiscriminant(val) =>
@@ -236,10 +226,10 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>
         }
     }
 
-    fn visit_primitive(&mut self, ectx: &mut EvalContext<'a, 'mir, 'tcx, M>)
+    fn visit_primitive(&mut self, op: OpTy<'tcx, M::PointerTag>)
         -> EvalResult<'tcx>
     {
-        let value = try_validation!(ectx.read_immediate(self.op),
+        let value = try_validation!(self.ecx.read_immediate(op),
             "uninitialized or unrepresentable data", self.path);
         // Go over all the primitive types
         let ty = value.layout.ty;
@@ -283,21 +273,21 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>
                     "undefined address in pointer", self.path);
                 let meta = try_validation!(value.to_meta(),
                     "uninitialized data in fat pointer metadata", self.path);
-                let layout = ectx.layout_of(value.layout.ty.builtin_deref(true).unwrap().ty)?;
+                let layout = self.ecx.layout_of(value.layout.ty.builtin_deref(true).unwrap().ty)?;
                 if layout.is_unsized() {
-                    let tail = ectx.tcx.struct_tail(layout.ty);
+                    let tail = self.ecx.tcx.struct_tail(layout.ty);
                     match tail.sty {
                         ty::Dynamic(..) => {
                             let vtable = try_validation!(meta.unwrap().to_ptr(),
                                 "non-pointer vtable in fat pointer", self.path);
-                            try_validation!(ectx.read_drop_type_from_vtable(vtable),
+                            try_validation!(self.ecx.read_drop_type_from_vtable(vtable),
                                 "invalid drop fn in vtable", self.path);
-                            try_validation!(ectx.read_size_and_align_from_vtable(vtable),
+                            try_validation!(self.ecx.read_size_and_align_from_vtable(vtable),
                                 "invalid size or align in vtable", self.path);
                             // FIXME: More checks for the vtable.
                         }
                         ty::Slice(..) | ty::Str => {
-                            try_validation!(meta.unwrap().to_usize(ectx),
+                            try_validation!(meta.unwrap().to_usize(self.ecx),
                                 "non-integer slice length in fat pointer", self.path);
                         }
                         ty::Foreign(..) => {
@@ -308,12 +298,12 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>
                     }
                 }
                 // Make sure this is non-NULL and aligned
-                let (size, align) = ectx.size_and_align_of(meta, layout)?
+                let (size, align) = self.ecx.size_and_align_of(meta, layout)?
                     // for the purpose of validity, consider foreign types to have
                     // alignment and size determined by the layout (size will be 0,
                     // alignment should take attributes into account).
                     .unwrap_or_else(|| layout.size_and_align());
-                match ectx.memory.check_align(ptr, align) {
+                match self.ecx.memory.check_align(ptr, align) {
                     Ok(_) => {},
                     Err(err) => {
                         error!("{:?} is not aligned to {:?}", ptr, align);
@@ -334,7 +324,7 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>
                 // Turn ptr into place.
                 // `ref_to_mplace` also calls the machine hook for (re)activating the tag,
                 // which in turn will (in full miri) check if the pointer is dereferencable.
-                let place = ectx.ref_to_mplace(value)?;
+                let place = self.ecx.ref_to_mplace(value)?;
                 // Recursive checking
                 if let Some(ref mut ref_tracking) = self.ref_tracking {
                     assert!(self.const_mode, "We should only do recursie checking in const mode");
@@ -343,19 +333,19 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>
                         let ptr = try_validation!(place.ptr.to_ptr(),
                             "integer pointer in non-ZST reference", self.path);
                         // Skip validation entirely for some external statics
-                        let alloc_kind = ectx.tcx.alloc_map.lock().get(ptr.alloc_id);
+                        let alloc_kind = self.ecx.tcx.alloc_map.lock().get(ptr.alloc_id);
                         if let Some(AllocType::Static(did)) = alloc_kind {
                             // `extern static` cannot be validated as they have no body.
                             // FIXME: Statics from other crates are also skipped.
                             // They might be checked at a different type, but for now we
                             // want to avoid recursing too deeply.  This is not sound!
-                            if !did.is_local() || ectx.tcx.is_foreign_item(did) {
+                            if !did.is_local() || self.ecx.tcx.is_foreign_item(did) {
                                 return Ok(());
                             }
                         }
                         // Maintain the invariant that the place we are checking is
                         // already verified to be in-bounds.
-                        try_validation!(ectx.memory.check_bounds(ptr, size, false),
+                        try_validation!(self.ecx.memory.check_bounds(ptr, size, false),
                             "dangling (not entirely in bounds) reference", self.path);
                     }
                     // Check if we have encountered this pointer+layout combination
@@ -379,7 +369,7 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>
                 let value = value.to_scalar_or_undef();
                 let ptr = try_validation!(value.to_ptr(),
                     value, self.path, "a pointer");
-                let _fn = try_validation!(ectx.memory.get_fn(ptr),
+                let _fn = try_validation!(self.ecx.memory.get_fn(ptr),
                     value, self.path, "a function pointer");
                 // FIXME: Check if the signature matches
             }
@@ -389,21 +379,23 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>
         Ok(())
     }
 
-    fn visit_uninhabited(&mut self, _ectx: &mut EvalContext<'a, 'mir, 'tcx, M>)
+    fn visit_uninhabited(&mut self, _op: OpTy<'tcx, M::PointerTag>)
         -> EvalResult<'tcx>
     {
         validation_failure!("a value of an uninhabited type", self.path)
     }
 
-    fn visit_scalar(&mut self, ectx: &mut EvalContext<'a, 'mir, 'tcx, M>, layout: &layout::Scalar)
-        -> EvalResult<'tcx>
-    {
-        let value = try_validation!(ectx.read_scalar(self.op),
+    fn visit_scalar(
+        &mut self,
+        op: OpTy<'tcx, M::PointerTag>,
+        layout: &layout::Scalar,
+    ) -> EvalResult<'tcx> {
+        let value = try_validation!(self.ecx.read_scalar(op),
             "uninitialized or unrepresentable data", self.path);
         // Determine the allowed range
         let (lo, hi) = layout.valid_range.clone().into_inner();
         // `max_hi` is as big as the size fits
-        let max_hi = u128::max_value() >> (128 - self.op.layout.size.bits());
+        let max_hi = u128::max_value() >> (128 - op.layout.size.bits());
         assert!(hi <= max_hi);
         // We could also write `(hi + 1) % (max_hi + 1) == lo` but `max_hi + 1` overflows for `u128`
         if (lo == 0 && hi == max_hi) || (hi + 1 == lo) {
@@ -421,10 +413,10 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>
                     // We can call `check_align` to check non-NULL-ness, but have to also look
                     // for function pointers.
                     let non_null =
-                        ectx.memory.check_align(
+                        self.ecx.memory.check_align(
                             Scalar::Ptr(ptr), Align::from_bytes(1, 1).unwrap()
                         ).is_ok() ||
-                        ectx.memory.get_fn(ptr).is_ok();
+                        self.ecx.memory.get_fn(ptr).is_ok();
                     if !non_null {
                         // could be NULL
                         return validation_failure!("a potentially NULL pointer", self.path);
@@ -444,7 +436,7 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>
                 }
             }
             Scalar::Bits { bits, size } => {
-                assert_eq!(size as u64, self.op.layout.size.bytes());
+                assert_eq!(size as u64, op.layout.size.bytes());
                 bits
             }
         };
@@ -479,13 +471,12 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>
         }
     }
 
-    fn visit_array(&mut self, ectx: &mut EvalContext<'a, 'mir, 'tcx, M>)
-        -> EvalResult<'tcx>
+    fn visit_array(&mut self, op: OpTy<'tcx, M::PointerTag>) -> EvalResult<'tcx>
     {
-        match self.op.layout.ty.sty {
+        match op.layout.ty.sty {
             ty::Str => {
-                let mplace = self.op.to_mem_place(); // strings are never immediate
-                try_validation!(ectx.read_str(mplace),
+                let mplace = op.to_mem_place(); // strings are never immediate
+                try_validation!(self.ecx.read_str(mplace),
                     "uninitialized or non-UTF-8 data in str", self.path);
             }
             ty::Array(tys, ..) | ty::Slice(tys) if {
@@ -496,17 +487,17 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>
                     _ => false,
                 }
             } => {
-                let mplace = if self.op.layout.is_zst() {
+                let mplace = if op.layout.is_zst() {
                     // it's a ZST, the memory content cannot matter
-                    MPlaceTy::dangling(self.op.layout, ectx)
+                    MPlaceTy::dangling(op.layout, self.ecx)
                 } else {
                     // non-ZST array/slice/str cannot be immediate
-                    self.op.to_mem_place()
+                    op.to_mem_place()
                 };
                 // This is the length of the array/slice.
-                let len = mplace.len(ectx)?;
+                let len = mplace.len(self.ecx)?;
                 // This is the element type size.
-                let ty_size = ectx.layout_of(tys)?.size;
+                let ty_size = self.ecx.layout_of(tys)?.size;
                 // This is the size in bytes of the whole array.
                 let size = ty_size * len;
 
@@ -519,7 +510,7 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>
                 // to reject those pointers, we just do not have the machinery to
                 // talk about parts of a pointer.
                 // We also accept undef, for consistency with the type-based checks.
-                match ectx.memory.check_bytes(
+                match self.ecx.memory.check_bytes(
                     mplace.ptr,
                     size,
                     /*allow_ptr_and_undef*/!self.const_mode,
@@ -548,7 +539,7 @@ impl<'rt, 'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>
                 }
             }
             _ => {
-                self.walk_array(ectx)? // default handler
+                self.walk_array(op)? // default handler
             }
         }
         Ok(())
@@ -574,14 +565,13 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> EvalContext<'a, 'mir, 'tcx, M>
 
         // Construct a visitor
         let mut visitor = ValidityVisitor {
-            op,
             path,
             ref_tracking,
             const_mode,
-            tcx: *self.tcx,
+            ecx: self,
         };
 
         // Run it
-        visitor.visit_value(self)
+        visitor.visit_value(op)
     }
 }
diff --git a/src/librustc_mir/interpret/visitor.rs b/src/librustc_mir/interpret/visitor.rs
index 8b153e17c1e..f50ef96775f 100644
--- a/src/librustc_mir/interpret/visitor.rs
+++ b/src/librustc_mir/interpret/visitor.rs
@@ -1,8 +1,6 @@
 //! Visitor for a run-time value with a given layout: Traverse enums, structs and other compound
 //! types until we arrive at the leaves, with custom handling for primitive types.
 
-use std::fmt;
-
 use rustc::ty::layout::{self, TyLayout};
 use rustc::ty;
 use rustc::mir::interpret::{
@@ -166,103 +164,103 @@ impl<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>> Value<'a, 'mir, 'tcx, M>
 }
 
 // How to traverse a value and what to do when we are at the leaves.
-pub trait ValueVisitor<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>: fmt::Debug + Sized {
+pub trait ValueVisitor<'a, 'mir, 'tcx: 'mir+'a, M: Machine<'a, 'mir, 'tcx>>: Sized {
     type V: Value<'a, 'mir, 'tcx, M>;
 
-    // There's a value in here.
-    fn value(&self) -> &Self::V;
+    // The visitor must have an `EvalContext` in it.
+    fn ecx(&mut self) -> &mut EvalContext<'a, 'mir, 'tcx, M>;
 
-    // The value's layout (not meant to be overwritten).
+    // Recursie actions, ready to be overloaded.
+    /// Visit the given value, dispatching as appropriate to more speicalized visitors.
     #[inline(always)]
-    fn layout(&self) -> TyLayout<'tcx> {
-        self.value().layout()
+    fn visit_value(&mut self, v: Self::V) -> EvalResult<'tcx>
+    {
+        self.walk_value(v)
     }
-
-    // Recursie actions, ready to be overloaded.
-    /// Visit the current value, dispatching as appropriate to more speicalized visitors.
-    #[inline]
-    fn visit_value(&mut self, ectx: &mut EvalContext<'a, 'mir, 'tcx, M>)
-        -> EvalResult<'tcx>
+    /// Visit the given value as a union.
+    #[inline(always)]
+    fn visit_union(&mut self, _v: Self::V) -> EvalResult<'tcx>
     {
-        self.walk_value(ectx)
+        Ok(())
     }
-    /// Visit the current value as an array.
-    #[inline]
-    fn visit_array(&mut self, ectx: &mut EvalContext<'a, 'mir, 'tcx, M>)
-        -> EvalResult<'tcx>
+    /// Visit the given value as an array.
+    #[inline(always)]
+    fn visit_array(&mut self, v: Self::V) -> EvalResult<'tcx>
     {
-        self.walk_array(ectx)
+        self.walk_array(v)
     }
-    /// Called each time we recurse down to a field of the value, to (a) let
-    /// the visitor change its internal state (recording the new current value),
-    /// and (b) let the visitor track the "stack" of fields that we descended below.
+    /// Called each time we recurse down to a field, passing in old and new value.
+    /// This gives the visitor the chance to track the stack of nested fields that
+    /// we are descending through.
+    #[inline(always)]
     fn visit_field(
         &mut self,
-        ectx: &mut EvalContext<'a, 'mir, 'tcx, M>,
-        val: Self::V,
-        field: usize,
-    ) -> EvalResult<'tcx>;
+        _old_val: Self::V,
+        _field: usize,
+        new_val: Self::V,
+    ) -> EvalResult<'tcx> {
+        self.visit_value(new_val)
+    }
 
     // Actions on the leaves, ready to be overloaded.
-    #[inline]
-    fn visit_uninhabited(&mut self, _ectx: &mut EvalContext<'a, 'mir, 'tcx, M>)
-        -> EvalResult<'tcx>
+    /// Called whenever we reach a value with uninhabited layout.
+    /// Recursing to fields will continue after this!
+    #[inline(always)]
+    fn visit_uninhabited(&mut self, _v: Self::V) -> EvalResult<'tcx>
     { Ok(()) }
-    #[inline]
-    fn visit_scalar(&mut self, _ectx: &mut EvalContext<'a, 'mir, 'tcx, M>, _layout: &layout::Scalar)
-        -> EvalResult<'tcx>
+    /// Called whenever we reach a value with scalar layout.
+    /// Recursing to fields will continue after this!
+    #[inline(always)]
+    fn visit_scalar(&mut self, _v: Self::V, _layout: &layout::Scalar) -> EvalResult<'tcx>
     { Ok(()) }
-    #[inline]
-    fn visit_primitive(&mut self, _ectx: &mut EvalContext<'a, 'mir, 'tcx, M>)
-        -> EvalResult<'tcx>
+    /// Called whenever we reach a value of primitive type.  There can be no recursion
+    /// below such a value.
+    #[inline(always)]
+    fn visit_primitive(&mut self, _v: Self::V) -> EvalResult<'tcx>
     { Ok(()) }
 
     // Default recursors. Not meant to be overloaded.
-    fn walk_array(&mut self, ectx: &mut EvalContext<'a, 'mir, 'tcx, M>)
-        -> EvalResult<'tcx>
+    fn walk_array(&mut self, v: Self::V) -> EvalResult<'tcx>
     {
         // Let's get an mplace first.
-        let mplace = if self.layout().is_zst() {
+        let mplace = if v.layout().is_zst() {
             // it's a ZST, the memory content cannot matter
-            MPlaceTy::dangling(self.layout(), ectx)
+            MPlaceTy::dangling(v.layout(), self.ecx())
         } else {
             // non-ZST array/slice/str cannot be immediate
-            self.value().to_mem_place(ectx)?
+            v.to_mem_place(self.ecx())?
         };
         // Now iterate over it.
-        for (i, field) in ectx.mplace_array_fields(mplace)?.enumerate() {
-            self.visit_field(ectx, Value::from_mem_place(field?), i)?;
+        for (i, field) in self.ecx().mplace_array_fields(mplace)?.enumerate() {
+            self.visit_field(v, i, Value::from_mem_place(field?))?;
         }
         Ok(())
     }
-    fn walk_value(&mut self, ectx: &mut EvalContext<'a, 'mir, 'tcx, M>)
-        -> EvalResult<'tcx>
+    fn walk_value(&mut self, v: Self::V) -> EvalResult<'tcx>
     {
-        trace!("walk_value: {:?}", self);
-
         // If this is a multi-variant layout, we have find the right one and proceed with that.
         // (No benefit from making this recursion, but it is equivalent to that.)
-        match self.layout().variants {
+        match v.layout().variants {
             layout::Variants::NicheFilling { .. } |
             layout::Variants::Tagged { .. } => {
-                let (inner, idx) = self.value().project_downcast(ectx)?;
+                let (inner, idx) = v.project_downcast(self.ecx())?;
                 trace!("variant layout: {:#?}", inner.layout());
                 // recurse with the inner type
-                return self.visit_field(ectx, inner, idx);
+                return self.visit_field(v, idx, inner);
             }
             layout::Variants::Single { .. } => {}
         }
 
         // Even for single variants, we might be able to get a more refined type:
         // If it is a trait object, switch to the actual type that was used to create it.
-        match self.layout().ty.sty {
+        match v.layout().ty.sty {
             ty::Dynamic(..) => {
                 // immediate trait objects are not a thing
-                let dest = self.value().to_mem_place(ectx)?;
-                let inner = ectx.unpack_dyn_trait(dest)?.1;
+                let dest = v.to_mem_place(self.ecx())?;
+                let inner = self.ecx().unpack_dyn_trait(dest)?.1;
                 trace!("dyn object layout: {:#?}", inner.layout);
                 // recurse with the inner type
-                return self.visit_field(ectx, Value::from_mem_place(inner), 0);
+                return self.visit_field(v, 0, Value::from_mem_place(inner));
             },
             _ => {},
         };
@@ -274,12 +272,12 @@ pub trait ValueVisitor<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>: fmt::Debug +
         // FIXME: We could avoid some redundant checks here. For newtypes wrapping
         // scalars, we do the same check on every "level" (e.g. first we check
         // MyNewtype and then the scalar in there).
-        match self.layout().abi {
+        match v.layout().abi {
             layout::Abi::Uninhabited => {
-                self.visit_uninhabited(ectx)?;
+                self.visit_uninhabited(v)?;
             }
             layout::Abi::Scalar(ref layout) => {
-                self.visit_scalar(ectx, layout)?;
+                self.visit_scalar(v, layout)?;
             }
             // FIXME: Should we do something for ScalarPair? Vector?
             _ => {}
@@ -290,34 +288,32 @@ pub trait ValueVisitor<'a, 'mir, 'tcx, M: Machine<'a, 'mir, 'tcx>>: fmt::Debug +
         // so we check them separately and before aggregate handling.
         // It is CRITICAL that we get this check right, or we might be
         // validating the wrong thing!
-        let primitive = match self.layout().fields {
+        let primitive = match v.layout().fields {
             // Primitives appear as Union with 0 fields -- except for Boxes and fat pointers.
             layout::FieldPlacement::Union(0) => true,
-            _ => self.layout().ty.builtin_deref(true).is_some(),
+            _ => v.layout().ty.builtin_deref(true).is_some(),
         };
         if primitive {
-            return self.visit_primitive(ectx);
+            return self.visit_primitive(v);
         }
 
         // Proceed into the fields.
-        match self.layout().fields {
+        match v.layout().fields {
             layout::FieldPlacement::Union(fields) => {
                 // Empty unions are not accepted by rustc. That's great, it means we can
                 // use that as an unambiguous signal for detecting primitives.  Make sure
                 // we did not miss any primitive.
                 debug_assert!(fields > 0);
-                // We can't traverse 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
+                self.visit_union(v)?;
             },
             layout::FieldPlacement::Arbitrary { ref offsets, .. } => {
                 for i in 0..offsets.len() {
-                    let val = self.value().project_field(ectx, i as u64)?;
-                    self.visit_field(ectx, val, i)?;
+                    let val = v.project_field(self.ecx(), i as u64)?;
+                    self.visit_field(v, i, val)?;
                 }
             },
             layout::FieldPlacement::Array { .. } => {
-                self.visit_array(ectx)?;
+                self.visit_array(v)?;
             }
         }
         Ok(())