Repository: tweag/nickel
Pull request #903 (2022-11-01 09:58:00)

[Refactor] Introduce `RecordBase` to share common fields between `Record` and `RecRecord` terms
### Why is this needed?

As part of refactoring record contracts with polymorphic tails, we're going to introduce an `Option<Box<Term>>` field to both `Record`s and `RecRecord`s which will hold the sealed portion of records passed through contracts with polymorphic tails (see [this issue](https://github.com/tweag/nickel/issues/201) for more info, in particular the section "Coarse sealing built-in"). 

To make this simpler, this PR groups together the existing common fields into a `RecordBase` struct. It also introduces two functions on this type to avoid the need for consumers to destructure & then rebuild `RecordBase`s everywhere. 

### Notes

- Both `map_fields` and `filter_map_fields` take their function parameter as `mut`. This is because in certain places we build up additional state while iterating (e.g. by pushing the `Ident`s into a `Vec`). I _think_ it's reasonable to do this, but would appreciate other opinions in case there's something I missed.

### Recommendation for reviewers

Each commit is self-contained & passes the whole test suite. Several also have extra detail in the commit messages. As such I'd recommend reviewing commit-by-commit rather than trying to review the whole PR as a single unit.

matthew-healy: @vkleen Huh, I hadn't considered `RecordBase` but I prefer it to `BaseRecord` for reasons I can't quite put into words. Unless I hear an alternative suggestion I'll make that change later today. Thank you! :pray: 
matthew-healy: @yannham Good suggestion! I've also seen the `_Data` naming convention somewhere, but completely forgot about it until you mentioned it. I think in this case I'll stick with `RecordBase` because this represents part of two variants, but we can always come back & rename it later.
yannham: `RecordBase` is fine, but I think the `Base` part doesn't convey much information. `RecordFields`? `RecordMap`? Those are just suggestion, you can stick to `RecordBase` if you think this is better.

Another random thought: have you thought about inlining `RecordAttrs` into `RecordBase`? I think I originally put it in a separate variant for the very same reason (for it to be shared between `RecRecord` and `Record`, and also to separate it from the rest of the data). I think going either way is fine (separated or inlined), I'm just wondering.
matthew-healy: Following the discussions outside of this PR, I've decided to:

- rename `RecordBase` to `RecordData`
- inline `RecordAttrs` into `RecordData`

I'll do that now, then merge once it passes CI.
matthew-healy: Actually, I'd like to close this PR & then inline `RecordAttrs` separately, as I think that change will lead to some non-trivial changes around how records are merged.

Patch:
diff --git a/lsp/nls/src/linearization/mod.rs b/lsp/nls/src/linearization/mod.rs
index dbbd6364..ec8ad5de 100644
--- a/lsp/nls/src/linearization/mod.rs
+++ b/lsp/nls/src/linearization/mod.rs
@@ -292,7 +292,7 @@ impl Linearizer for AnalysisHost {
                     }
                 }
             }
-            Term::Record(fields, _) | Term::RecRecord(fields, ..) => {
+            Term::Record(record) | Term::RecRecord(record, ..) => {
                 lin.push(LinearizationItem {
                     id,
                     pos,
@@ -302,8 +302,8 @@ impl Linearizer for AnalysisHost {
                     meta: self.meta.take(),
                 });
 
-                lin.register_fields(fields, id, self.scope.clone(), &mut self.env);
-                let mut field_names = fields.keys().cloned().collect::<Vec<_>>();
+                lin.register_fields(&record.fields, id, self.scope.clone(), &mut self.env);
+                let mut field_names = record.fields.keys().cloned().collect::<Vec<_>>();
                 field_names.sort_unstable();
 
                 self.record_fields =
diff --git a/src/cache.rs b/src/cache.rs
index 62f88245..bc8650fc 100644
--- a/src/cache.rs
+++ b/src/cache.rs
@@ -4,6 +4,7 @@ use crate::error::{Error, ImportError, ParseError, ParseErrors, TypecheckError};
 use crate::parser::lexer::Lexer;
 use crate::position::TermPos;
 use crate::stdlib as nickel_stdlib;
+use crate::term::record::RecordData;
 use crate::term::{RichTerm, SharedTerm, Term};
 use crate::transform::import_resolution;
 use crate::typecheck::type_check;
@@ -567,20 +568,22 @@ impl Cache {
                     let pos = term.pos;
 
                     match SharedTerm::make_mut(&mut term.term) {
-                        Term::Record(ref mut map, _) => {
-                            let map_res: Result<_, UnboundTypeVariableError> = std::mem::take(map)
-                                .into_iter()
-                                .map(|(id, t)| Ok((id, transform::transform(t, wildcards)?)))
-                                .collect();
-                            *map = map_res.map_err(|err| {
+                        Term::Record(RecordData { ref mut fields, .. }) => {
+                            let map_res: Result<_, UnboundTypeVariableError> =
+                                std::mem::take(fields)
+                                    .into_iter()
+                                    .map(|(id, t)| Ok((id, transform::transform(t, wildcards)?)))
+                                    .collect();
+                            *fields = map_res.map_err(|err| {
                                 CacheError::Error(ImportError::ParseErrors(err.into(), pos))
                             })?;
                         }
-                        Term::RecRecord(ref mut map, ref mut dyn_fields, ..) => {
-                            let map_res: Result<_, UnboundTypeVariableError> = std::mem::take(map)
-                                .into_iter()
-                                .map(|(id, t)| Ok((id, transform::transform(t, wildcards)?)))
-                                .collect();
+                        Term::RecRecord(ref mut record, ref mut dyn_fields, ..) => {
+                            let map_res: Result<_, UnboundTypeVariableError> =
+                                std::mem::take(&mut record.fields)
+                                    .into_iter()
+                                    .map(|(id, t)| Ok((id, transform::transform(t, wildcards)?)))
+                                    .collect();
 
                             let dyn_fields_res: Result<_, UnboundTypeVariableError> =
                                 std::mem::take(dyn_fields)
@@ -593,7 +596,7 @@ impl Cache {
                                     })
                                     .collect();
 
-                            *map = map_res.map_err(|err| {
+                            record.fields = map_res.map_err(|err| {
                                 CacheError::Error(ImportError::ParseErrors(err.into(), pos))
                             })?;
                             *dyn_fields = dyn_fields_res.map_err(|err| {
diff --git a/src/deserialize.rs b/src/deserialize.rs
index c25dbb99..af372688 100644
--- a/src/deserialize.rs
+++ b/src/deserialize.rs
@@ -75,7 +75,7 @@ impl<'de> serde::Deserializer<'de> for RichTerm {
                 variant: v.into_label(),
                 rich_term: None,
             }),
-            Term::Record(v, _) => visit_record(v, visitor),
+            Term::Record(record) => visit_record(record.fields, visitor),
             Term::Array(v, _) => visit_array(v, visitor),
             Term::MetaValue(_) => visitor.visit_unit(),
             other => Err(RustDeserializationError::UnimplementedType {
@@ -120,8 +120,8 @@ impl<'de> serde::Deserializer<'de> for RichTerm {
     {
         let (variant, rich_term) = match unwrap_term(self)? {
             Term::Enum(ident) => (ident.into_label(), None),
-            Term::Record(v, _) => {
-                let mut iter = v.into_iter();
+            Term::Record(record) => {
+                let mut iter = record.fields.into_iter();
                 let (variant, value) = match iter.next() {
                     Some(v) => v,
                     None => {
@@ -296,7 +296,7 @@ impl<'de> serde::Deserializer<'de> for RichTerm {
         V: Visitor<'de>,
     {
         match unwrap_term(self)? {
-            Term::Record(v, _) => visit_record(v, visitor),
+            Term::Record(record) => visit_record(record.fields, visitor),
             other => Err(RustDeserializationError::InvalidType {
                 expected: "Record".to_string(),
                 occurred: other.type_of().unwrap_or_else(|| "Other".to_string()),
@@ -316,7 +316,7 @@ impl<'de> serde::Deserializer<'de> for RichTerm {
     {
         match unwrap_term(self)? {
             Term::Array(v, _) => visit_array(v, visitor),
-            Term::Record(v, _) => visit_record(v, visitor),
+            Term::Record(record) => visit_record(record.fields, visitor),
             other => Err(RustDeserializationError::InvalidType {
                 expected: "Record".to_string(),
                 occurred: other.type_of().unwrap_or_else(|| "Other".to_string()),
@@ -524,7 +524,7 @@ impl<'de> VariantAccess<'de> for VariantDeserializer {
         V: Visitor<'de>,
     {
         match self.rich_term.map(unwrap_term) {
-            Some(Ok(Term::Record(v, _))) => visit_record(v, visitor),
+            Some(Ok(Term::Record(record))) => visit_record(record.fields, visitor),
             Some(Ok(other)) => Err(RustDeserializationError::InvalidType {
                 expected: "Array variant".to_string(),
                 occurred: other.type_of().unwrap_or_else(|| "Other".to_string()),
diff --git a/src/destruct.rs b/src/destruct.rs
index eebe808b..cf19a68e 100644
--- a/src/destruct.rs
+++ b/src/destruct.rs
@@ -4,6 +4,7 @@
 use crate::identifier::Ident;
 use crate::label::Label;
 use crate::position::RawSpan;
+use crate::term::record::RecordData;
 use crate::term::{Contract, MetaValue, RecordAttrs, RichTerm, Term};
 use crate::types::{AbsType, Types};
 
@@ -60,13 +61,13 @@ impl Destruct {
         MetaValue {
             contracts: vec![Contract {
                 types: Types(AbsType::Flat(
-                    Term::Record(
+                    Term::Record(RecordData::new(
                         self.inner()
                             .into_iter()
                             .map(|m| m.as_meta_field())
                             .collect(),
                         RecordAttrs { open },
-                    )
+                    ))
                     .into(),
                 )),
                 label,
diff --git a/src/eval/merge.rs b/src/eval/merge.rs
index 7341bee1..af564a7d 100644
--- a/src/eval/merge.rs
+++ b/src/eval/merge.rs
@@ -55,6 +55,7 @@ use super::*;
 use crate::error::EvalError;
 use crate::label::Label;
 use crate::position::TermPos;
+use crate::term::record::RecordData;
 use crate::term::{
     make as mk_term, BinaryOp, Contract, MetaValue, RecordAttrs, RichTerm, SharedTerm, Term,
 };
@@ -361,7 +362,7 @@ pub fn merge(
         }
         // Merge put together the fields of records, and recursively merge
         // fields that are present in both terms
-        (Term::Record(mut m1, attrs1), Term::Record(mut m2, attrs2)) => {
+        (Term::Record(mut r1), Term::Record(mut r2)) => {
             /* Terms inside m1 and m2 may capture variables of resp. env1 and env2.  Morally, we
              * need to store closures, or a merge of closures, inside the resulting record.  We use
              * the same trick as in the evaluation of the operator DynExtend, and replace each such
@@ -370,19 +371,19 @@ pub fn merge(
             // Merging recursive record is the one operation that may override recursive fields. To
             // have the recursive fields depend on the updated values, we need to revert the thunks
             // first.
-            rev_thunks(m1.values_mut(), &mut env1);
-            rev_thunks(m2.values_mut(), &mut env2);
+            rev_thunks(r1.fields.values_mut(), &mut env1);
+            rev_thunks(r2.fields.values_mut(), &mut env2);
 
             // We save the original fields before they are potentially merged in order to patch
             // their environment in the final record (cf `fixpoint::patch_fields`). Note that we
             // are only cloning shared terms (`Rc`s) here.
-            let m1_values: Vec<_> = m1.values().cloned().collect();
-            let m2_values: Vec<_> = m2.values().cloned().collect();
+            let m1_values: Vec<_> = r1.fields.values().cloned().collect();
+            let m2_values: Vec<_> = r2.fields.values().cloned().collect();
 
-            let (left, center, right) = hashmap::split(m1, m2);
+            let (left, center, right) = hashmap::split(r1.fields, r2.fields);
 
             match mode {
-                MergeMode::Contract(mut lbl) if !attrs2.open && !left.is_empty() => {
+                MergeMode::Contract(mut lbl) if !r2.attrs.open && !left.is_empty() => {
                     let fields: Vec<String> =
                         left.keys().map(|field| format!("`{}`", field)).collect();
                     let plural = if fields.len() == 1 { "" } else { "s" };
@@ -426,7 +427,7 @@ pub fn merge(
 
             Ok(Closure {
                 body: RichTerm::new(
-                    Term::Record(m, RecordAttrs::merge(attrs1, attrs2)),
+                    Term::Record(RecordData::new(m, RecordAttrs::merge(r1.attrs, r2.attrs))),
                     final_pos,
                 ),
                 env,
diff --git a/src/eval/mod.rs b/src/eval/mod.rs
index 20b1afa1..c5286b7c 100644
--- a/src/eval/mod.rs
+++ b/src/eval/mod.rs
@@ -96,8 +96,8 @@ use crate::{
     identifier::Ident,
     match_sharedterm,
     term::{
-        make as mk_term, ArrayAttrs, BinaryOp, BindingType, LetAttrs, MetaValue, PendingContract,
-        RichTerm, SharedTerm, StrChunk, Term, UnaryOp,
+        make as mk_term, record::RecordData, ArrayAttrs, BinaryOp, BindingType, LetAttrs,
+        MetaValue, PendingContract, RichTerm, SharedTerm, StrChunk, Term, UnaryOp,
     },
     transform::Closurizable,
 };
@@ -399,7 +399,7 @@ impl<R: ImportResolver> VirtualMachine<R> {
                     self.stack.push_arg(
                         Closure {
                             body: RichTerm::new(
-                                Term::Record(cases.clone(), Default::default()),
+                                Term::Record(RecordData::with_fields(cases.clone())),
                                 pos,
                             ),
                             env: env.clone(),
@@ -514,15 +514,17 @@ impl<R: ImportResolver> VirtualMachine<R> {
                         }
                     }
                 }
-                Term::RecRecord(ts, dyn_fields, attrs, _) => {
-                    let rec_env = fixpoint::rec_env(ts.iter(), &env)?;
+                Term::RecRecord(record, dyn_fields, _) => {
+                    let rec_env = fixpoint::rec_env(record.fields.iter(), &env)?;
 
-                    ts.iter()
+                    record
+                        .fields
+                        .iter()
                         .try_for_each(|(_, rt)| fixpoint::patch_field(rt, &rec_env, &env))?;
 
-                    //TODO: We should probably avoid cloning the `ts` hashmap, using `match_sharedterm`
+                    //TODO: We should probably avoid cloning the record, using `match_sharedterm`
                     //instead of `match` in the main eval loop, if possible
-                    let static_part = RichTerm::new(Term::Record(ts.clone(), *attrs), pos);
+                    let static_part = RichTerm::new(Term::Record(record.clone()), pos);
 
                     // Transform the static part `{stat1 = val1, ..., statn = valn}` and the dynamic
                     // part `{exp1 = dyn_val1, ..., expm = dyn_valm}` to a sequence of extensions
@@ -731,8 +733,8 @@ pub enum EnvBuildError {
 /// Add the bindings of a record to an environment. Ignore the fields defined by interpolation.
 pub fn env_add_term(env: &mut Environment, rt: RichTerm) -> Result<(), EnvBuildError> {
     match_sharedterm! {rt.term, with {
-            Term::Record(bindings, _) | Term::RecRecord(bindings, ..) => {
-                let ext = bindings.into_iter().map(|(id, t)| {
+            Term::Record(record) | Term::RecRecord(record, ..) => {
+                let ext = record.fields.into_iter().map(|(id, t)| {
                     (
                         id,
                         Thunk::new(Closure::atomic_closure(t), IdentKind::Record),
@@ -841,29 +843,13 @@ pub fn subst(rt: RichTerm, initial_env: &Environment, env: &Environment) -> Rich
             let t = subst(t, initial_env, env);
             RichTerm::new(Term::Sealed(i, t, lbl), pos)
         }
-        Term::Record(map, attrs) => {
-            let map = map
-                .into_iter()
-                .map(|(id, t)| {
-                    (
-                        id,
-                        subst(t, initial_env, env),
-                    )
-                })
-                .collect();
+        Term::Record(record) => {
+            let record = record.map_fields(|_, t| subst(t, initial_env, env));
 
-            RichTerm::new(Term::Record(map, attrs), pos)
+            RichTerm::new(Term::Record(record), pos)
         }
-        Term::RecRecord(map, dyn_fields, attrs, deps) => {
-            let map = map
-                .into_iter()
-                .map(|(id, t)| {
-                    (
-                        id,
-                        subst(t, initial_env, env),
-                    )
-                })
-                .collect();
+        Term::RecRecord(record, dyn_fields, deps) => {
+            let record = record.map_fields(|_, t| subst(t, initial_env, env));
 
             let dyn_fields = dyn_fields
                 .into_iter()
@@ -875,7 +861,7 @@ pub fn subst(rt: RichTerm, initial_env: &Environment, env: &Environment) -> Rich
                 })
                 .collect();
 
-            RichTerm::new(Term::RecRecord(map, dyn_fields, attrs, deps), pos)
+            RichTerm::new(Term::RecRecord(record, dyn_fields, deps), pos)
         }
         Term::Array(ts, attrs) => {
             let ts = ts
diff --git a/src/eval/operation.rs b/src/eval/operation.rs
index 144baa2d..9e852ebb 100644
--- a/src/eval/operation.rs
+++ b/src/eval/operation.rs
@@ -11,8 +11,8 @@ use super::{
     merge::{merge, MergeMode},
     subst, Closure, Environment, ImportResolver, VirtualMachine,
 };
-use crate::term::MergePriority;
 use crate::term::MetaValue;
+use crate::term::{record::RecordData, MergePriority};
 
 use crate::{
     error::EvalError,
@@ -25,8 +25,8 @@ use crate::{
     serialize::ExportFormat,
     stdlib::internals,
     term::{
-        array::Array, make as mk_term, ArrayAttrs, BinaryOp, NAryOp, PendingContract, RecordAttrs,
-        RichTerm, SharedTerm, StrChunk, Term, UnaryOp,
+        array::Array, make as mk_term, ArrayAttrs, BinaryOp, NAryOp, PendingContract, RichTerm,
+        SharedTerm, StrChunk, Term, UnaryOp,
     },
     transform::{apply_contracts::apply_contracts, Closurizable},
 };
@@ -35,10 +35,7 @@ use md5::digest::Digest;
 
 use simple_counter::*;
 
-use std::{
-    iter::Extend,
-    {collections::HashMap, rc::Rc},
-};
+use std::{iter::Extend, rc::Rc};
 
 generate_counter!(FreshVariableCounter, usize);
 
@@ -320,7 +317,7 @@ impl<R: ImportResolver> VirtualMachine<R> {
                     } = cases_closure;
 
                     let mut cases = match cases_term.into_owned() {
-                        Term::Record(map, _) => map,
+                        Term::Record(r) => r.fields,
                         _ => panic!("invalid argument for switch"),
                     };
 
@@ -441,8 +438,8 @@ impl<R: ImportResolver> VirtualMachine<R> {
                 }
             },
             UnaryOp::StaticAccess(id) => {
-                if let Term::Record(static_map, ..) = &*t {
-                    match static_map.get(&id) {
+                if let Term::Record(record) = &*t {
+                    match record.fields.get(&id) {
                         Some(e) => {
                             self.call_stack.enter_field(id, pos, e.pos, pos_op);
                             Ok(Closure {
@@ -467,8 +464,8 @@ impl<R: ImportResolver> VirtualMachine<R> {
                 }
             }
             UnaryOp::FieldsOf() => match_sharedterm! {t, with {
-                    Term::Record(map, ..) => {
-                        let mut fields: Vec<String> = map
+                    Term::Record(record) => {
+                        let mut fields: Vec<String> = record.fields
                             .into_iter()
                             // Ignore optional fields without definitions.
                             .filter_map(|(id, t)| {
@@ -493,8 +490,8 @@ impl<R: ImportResolver> VirtualMachine<R> {
                 }
             },
             UnaryOp::ValuesOf() => match_sharedterm! {t, with {
-                    Term::Record(map, ..) => {
-                        let mut values: Vec<_> = map.into_iter().collect();
+                    Term::Record(record) => {
+                        let mut values: Vec<_> = record.fields.into_iter().collect();
                         // Although it seems that sort_by_key would be easier here, it would actually
                         // require to copy the identifiers because of the lack of HKT. See
                         // https://github.com/rust-lang/rust/issues/34162.
@@ -617,27 +614,21 @@ impl<R: ImportResolver> VirtualMachine<R> {
                 })?;
 
                 match_sharedterm! {t, with {
-                        Term::Record(rec, attr) => {
+                        Term::Record(record) => {
                             let mut shared_env = Environment::new();
                             let f_as_var = f.body.closurize(&mut env, f.env);
 
                             // As for `ArrayMap` (see above), we closurize the content of fields
-                            let rec = rec
-                                .into_iter()
-                                .filter(|(_, t)| !is_empty_optional(t, &env))
-                                .map(|(id, t)| {
-                                    let pos = t.pos.into_inherited();
-                                    (
-                                        id.clone(),
-                                        mk_app!(f_as_var.clone(), mk_term::string(id.label()), t)
-                                            .closurize(&mut shared_env, env.clone())
-                                            .with_pos(pos),
-                                    )
-                                })
-                                .collect();
+                            let record = record.map_fields_without_optionals(&env, |id, t| {
+                                let pos = t.pos.into_inherited();
+                                let map_app = mk_app!(f_as_var.clone(), mk_term::string(id.label()), t)
+                                    .closurize(&mut shared_env, env.clone())
+                                    .with_pos(pos);
+                                map_app
+                            });
 
                             Ok(Closure {
-                                body: RichTerm::new(Term::Record(rec, attr), pos_op_inh),
+                                body: RichTerm::new(Term::Record(record), pos_op_inh),
                                 env: shared_env,
                             })
                         }
@@ -685,10 +676,11 @@ impl<R: ImportResolver> VirtualMachine<R> {
                 }
 
                 match t.into_owned() {
-                    Term::Record(map, _) if !map.is_empty() => {
+                    Term::Record(record) if !record.fields.is_empty() => {
                         let pos_record = pos;
                         let pos_access = pos_op;
-                        let terms = map
+                        let terms = record
+                            .fields
                             .into_iter()
                             // We ignore empty optional fields
                             .filter(|(_, t)| !is_empty_optional(t, &env))
@@ -1150,34 +1142,23 @@ impl<R: ImportResolver> VirtualMachine<R> {
 
                 match_sharedterm! {t,
                     with {
-                        Term::Record(map, attrs) if !map.is_empty() => {
+                        Term::Record(record) if !record.fields.is_empty() => {
                             let mut shared_env = Environment::new();
 
-                            let map = map
-                                .into_iter()
-                                // We ignore empty optional fields
-                                .filter(|(_, t)| !is_empty_optional(t, &env))
-                                .map(|(id, t)| {
-                                    let stack_elem = Some(callstack::StackElem::Field {
-                                        id: id.clone(),
-                                        pos_record: pos,
-                                        pos_field: t.pos,
-                                        pos_access: pos_op,
-                                    });
-
-                                    (
-                                        id,
-                                        mk_term::op1(UnaryOp::Force(stack_elem), t)
-                                            .closurize(&mut shared_env, env.clone()),
-                                    )
-                                })
-                                // It's important to collect here, otherwise the two usages below
-                                // will each do their own .closurize(...) calls and end up with
-                                // different variables, which means that `cont` won't be properly updated.
-                                .collect::<HashMap<_, _>>();
+                            let record = record.map_fields_without_optionals(&env, |id, t| {
+                                let stack_elem = Some(callstack::StackElem::Field {
+                                    id: id.clone(),
+                                    pos_record: pos,
+                                    pos_field: t.pos,
+                                    pos_access: pos_op,
+                                });
+                                let forced = mk_term::op1(UnaryOp::Force(stack_elem), t)
+                                    .closurize(&mut shared_env, env.clone());
+                                forced
+                            });
 
-                            let terms = map.clone().into_values();
-                            let cont = RichTerm::new(Term::Record(map, attrs), pos.into_inherited());
+                            let terms = record.fields.clone().into_values();
+                            let cont = RichTerm::new(Term::Record(record), pos.into_inherited());
 
                             Ok(Closure {
                                 body: seq_terms(terms, pos_op, cont),
@@ -1838,8 +1819,8 @@ impl<R: ImportResolver> VirtualMachine<R> {
             },
             BinaryOp::DynAccess() => match_sharedterm! {t1, with {
                     Term::Str(id) => {
-                        if let Term::Record(static_map, _attrs) = &*t2 {
-                            match static_map.get(&Ident::from(&id)) {
+                        if let Term::Record(record) = &*t2 {
+                            match record.fields.get(&Ident::from(&id)) {
                                 Some(e) => {
                                     self.call_stack.enter_field(Ident::from(id), pos2, e.pos, pos_op);
                                     Ok(Closure {
@@ -1889,13 +1870,13 @@ impl<R: ImportResolver> VirtualMachine<R> {
 
                 if let Term::Str(id) = &*t1 {
                     match_sharedterm! {t2, with {
-                            Term::Record(static_map, attrs) => {
-                                let mut static_map = static_map;
+                            Term::Record(record) => {
+                                let mut fields = record.fields;
                                 let as_var = clos.body.closurize(&mut env2, clos.env);
-                                match static_map.insert(Ident::from(id), as_var) {
+                                match fields.insert(Ident::from(id), as_var) {
                                     Some(t) if !is_empty_optional(&t, &env2) => Err(EvalError::Other(format!("$[ .. ]: tried to extend record with the field {}, but it already exists", id), pos_op)),
                                     _ => Ok(Closure {
-                                        body: Term::Record(static_map, attrs).into(),
+                                        body: Term::Record(RecordData::new(fields, record.attrs)).into(),
                                         env: env2,
                                     }),
                                 }
@@ -1926,16 +1907,16 @@ impl<R: ImportResolver> VirtualMachine<R> {
             }
             BinaryOp::DynRemove() => match_sharedterm! {t1, with {
                     Term::Str(id) => match_sharedterm! {t2, with {
-                            Term::Record(static_map, attrs) => {
-                                let mut static_map = static_map;
-                                let fetched = static_map.remove(&Ident::from(&id));
+                            Term::Record(record) => {
+                                let mut fields = record.fields;
+                                let fetched = fields.remove(&Ident::from(&id));
                                 if fetched.is_none()
                                    || matches!(fetched, Some(t) if is_empty_optional(&t, &env2)) {
                                     Err(EvalError::FieldMissing(
                                         id,
                                         String::from("(-$)"),
                                         RichTerm::new(
-                                            Term::Record(static_map, attrs),
+                                            Term::Record(RecordData::new(fields, record.attrs)),
                                             pos2,
                                         ),
                                         pos_op,
@@ -1944,7 +1925,7 @@ impl<R: ImportResolver> VirtualMachine<R> {
                                 else {
                                     Ok(Closure {
                                         body: RichTerm::new(
-                                            Term::Record(static_map, attrs), pos_op_inh
+                                            Term::Record(RecordData::new(fields, record.attrs)), pos_op_inh
                                         ),
                                         env: env2,
                                     })
@@ -1976,9 +1957,9 @@ impl<R: ImportResolver> VirtualMachine<R> {
             },
             BinaryOp::HasField() => match_sharedterm! {t1, with {
                     Term::Str(id) => {
-                        if let Term::Record(map, _) = &*t2 {
+                        if let Term::Record(record) = &*t2 {
                             Ok(Closure::atomic_closure(RichTerm::new(
-                                Term::Bool(matches!(map.get(&Ident::from(id)), Some(t5) if !is_empty_optional(t5, &env2))),
+                                Term::Bool(matches!(record.fields.get(&Ident::from(id)), Some(t5) if !is_empty_optional(t5, &env2))),
                                 pos_op_inh,
                             )))
                         } else {
@@ -2630,75 +2611,65 @@ impl PushPriority {
     }
 
     /// Push the priority down the fields of a record.
-    fn push_into_record(
-        &self,
-        map: HashMap<Ident, RichTerm>,
-        attrs: RecordAttrs,
-        env: &Environment,
-        pos: TermPos,
-    ) -> Closure {
+    fn push_into_record(&self, record: RecordData, env: &Environment, pos: TermPos) -> Closure {
         let mut new_env = Environment::new();
 
-        let map: HashMap<_, _> = map
-            .into_iter()
-            .filter(|(_, rt)| !is_empty_optional(rt, &env))
-            .map(|(id, rt)| {
-                // There is a subtlety with respect to overriding here. Take:
-                //
-                // ```nickel
-                // ({foo = bar + 1, bar = 1} | _push_default) & {bar = 2}
-                // ```
-                //
-                // In the example above, if we just map `$push_default` on the value of `foo` and
-                // closurize it into a new, normal thunk (non revertible), we lose the ability to
-                // override `foo` and we end up with the unexpected result `{foo = 2, bar = 2}`.
-                //
-                // What we want is that:
-                //
-                // ```nickel
-                // {foo = bar + 1, bar = 1} | _push_default
-                // ```
-                //
-                // is equivalent to writing:
-                //
-                // ```nickel
-                // {foo | default = bar + 1, bar | default = 1}
-                // ```
-                //
-                // For revertible thunks, we don't want to only map the push operator on the
-                // current cached value, but also on the original expression.
-                //
-                // To do so, we create a new independent copy of the original thunk by mapping the
-                // function over both expressions (in the sense of both the original expression and
-                // the cached expression). This logic is encapsulated by `Thunk::map`.
-                let pos = rt.pos;
-
-                let thunk = match rt.as_ref() {
-                    Term::Var(id) => {
-                        env.get(id)
-                            .unwrap()
-                            .map(|Closure { ref body, ref env }| Closure {
-                                body: self.apply_push_op(body.clone()),
-                                env: env.clone(),
-                            })
-                    }
-                    _ => eval::lazy::Thunk::new(
-                        Closure {
-                            body: self.apply_push_op(rt),
+        let record = record.map_fields_without_optionals(&env, |_, rt| {
+            // There is a subtlety with respect to overriding here. Take:
+            //
+            // ```nickel
+            // ({foo = bar + 1, bar = 1} | _push_default) & {bar = 2}
+            // ```
+            //
+            // In the example above, if we just map `$push_default` on the value of `foo` and
+            // closurize it into a new, normal thunk (non revertible), we lose the ability to
+            // override `foo` and we end up with the unexpected result `{foo = 2, bar = 2}`.
+            //
+            // What we want is that:
+            //
+            // ```nickel
+            // {foo = bar + 1, bar = 1} | _push_default
+            // ```
+            //
+            // is equivalent to writing:
+            //
+            // ```nickel
+            // {foo | default = bar + 1, bar | default = 1}
+            // ```
+            //
+            // For revertible thunks, we don't want to only map the push operator on the
+            // current cached value, but also on the original expression.
+            //
+            // To do so, we create a new independent copy of the original thunk by mapping the
+            // function over both expressions (in the sense of both the original expression and
+            // the cached expression). This logic is encapsulated by `Thunk::map`.
+            let pos = rt.pos;
+
+            let thunk = match rt.as_ref() {
+                Term::Var(id) => {
+                    env.get(id)
+                        .unwrap()
+                        .map(|Closure { ref body, ref env }| Closure {
+                            body: self.apply_push_op(body.clone()),
                             env: env.clone(),
-                        },
-                        eval::IdentKind::Record,
-                    ),
-                };
+                        })
+                }
+                _ => eval::lazy::Thunk::new(
+                    Closure {
+                        body: self.apply_push_op(rt),
+                        env: env.clone(),
+                    },
+                    eval::IdentKind::Record,
+                ),
+            };
 
-                let fresh_id = Ident::fresh();
-                new_env.insert(fresh_id.clone(), thunk);
-                (id, RichTerm::new(Term::Var(fresh_id), pos))
-            })
-            .collect();
+            let fresh_id = Ident::fresh();
+            new_env.insert(fresh_id.clone(), thunk);
+            RichTerm::new(Term::Var(fresh_id), pos)
+        });
 
         Closure {
-            body: RichTerm::new(Term::Record(map, attrs), pos),
+            body: RichTerm::new(Term::Record(record), pos),
             env: new_env,
         }
     }
@@ -2718,8 +2689,8 @@ impl PushPriority {
 
         let t = st.into_owned();
 
-        if let Term::Record(maps, attrs) = t {
-            self.push_into_record(maps, attrs, &env, pos)
+        if let Term::Record(record) = t {
+            self.push_into_record(record, &env, pos)
         } else {
             // Extract the metavalue, or if `st` isn't a metavalue, wrap it in a new one to set the merge
             // priority.
@@ -2753,11 +2724,11 @@ impl PushPriority {
                 let pos_inner = inner.pos;
 
                 match inner.term.into_owned() {
-                    Term::Record(map, attrs) => {
+                    Term::Record(record) => {
                         let Closure {
                             body,
                             env: record_env,
-                        } = self.push_into_record(map, attrs, &env_inner, pos_inner);
+                        } = self.push_into_record(record, &env_inner, pos_inner);
                         meta.value = Some(body.closurize(&mut env, record_env));
                     }
                     t if t.is_whnf() => {
@@ -2841,8 +2812,8 @@ fn eq(env: &mut Environment, c1: Closure, c2: Closure) -> EqResult {
         (Term::Lbl(l1), Term::Lbl(l2)) => EqResult::Bool(l1 == l2),
         (Term::SealingKey(s1), Term::SealingKey(s2)) => EqResult::Bool(s1 == s2),
         (Term::Enum(id1), Term::Enum(id2)) => EqResult::Bool(id1 == id2),
-        (Term::Record(m1, _), Term::Record(m2, _)) => {
-            let (left, center, right) = merge::hashmap::split(m1, m2);
+        (Term::Record(r1), Term::Record(r2)) => {
+            let (left, center, right) = merge::hashmap::split(r1.fields, r2.fields);
 
             // As for other record operations, we ignore optional fields without a definition.
             if !left.values().all(|rt| is_empty_optional(&rt, &env1))
@@ -2910,6 +2881,33 @@ fn eq(env: &mut Environment, c1: Closure, c2: Closure) -> EqResult {
     }
 }
 
+trait RecordDataExt {
+    fn map_fields_without_optionals<F>(self, env: &Environment, f: F) -> Self
+    where
+        F: FnMut(Ident, RichTerm) -> RichTerm;
+}
+
+impl RecordDataExt for RecordData {
+    /// Returns the record resulting from applying the provided function
+    /// to each field, and removing any field which is an empty optional
+    /// in the provided environment.
+    ///
+    /// Note that `f` is taken as `mut` in order to allow it to mutate
+    /// external state while iterating.
+    fn map_fields_without_optionals<F>(self, env: &Environment, mut f: F) -> Self
+    where
+        F: FnMut(Ident, RichTerm) -> RichTerm,
+    {
+        let fields = self
+            .fields
+            .into_iter()
+            .filter(|(_, t)| !is_empty_optional(t, env))
+            .map(|(id, t)| (id, f(id, t)))
+            .collect();
+        Self { fields, ..self }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
diff --git a/src/parser/utils.rs b/src/parser/utils.rs
index ee4d45d4..f4be2baa 100644
--- a/src/parser/utils.rs
+++ b/src/parser/utils.rs
@@ -17,8 +17,8 @@ use crate::{
     mk_app, mk_fun,
     position::{RawSpan, TermPos},
     term::{
-        make as mk_term, BinaryOp, Contract, MetaValue, RecordAttrs, RichTerm, StrChunk, Term,
-        UnaryOp,
+        make as mk_term, record::RecordData, BinaryOp, Contract, MetaValue, RecordAttrs, RichTerm,
+        StrChunk, Term, UnaryOp,
     },
     types::{AbsType, Types},
 };
@@ -253,9 +253,9 @@ pub fn elaborate_field_path(
 
     let content = it.rev().fold(content, |acc, path_elem| match path_elem {
         FieldPathElem::Ident(id) => {
-            let mut map = HashMap::new();
-            map.insert(id, acc);
-            Term::Record(map, Default::default()).into()
+            let mut fields = HashMap::new();
+            fields.insert(id, acc);
+            Term::Record(RecordData::with_fields(fields)).into()
         }
         FieldPathElem::Expr(exp) => {
             let static_access = match exp.term.as_ref() {
@@ -275,11 +275,14 @@ pub fn elaborate_field_path(
 
             if let Some(static_access) = static_access {
                 let id = Ident::new_with_pos(static_access, exp.pos);
-                let mut map = HashMap::new();
-                map.insert(id, acc);
-                Term::Record(map, Default::default()).into()
+                let mut fields = HashMap::new();
+                fields.insert(id, acc);
+                Term::Record(RecordData::with_fields(fields)).into()
             } else {
-                let empty = Term::Record(HashMap::new(), Default::default());
+                let empty = Term::Record(RecordData {
+                    fields: HashMap::new(),
+                    attrs: Default::default(),
+                });
                 mk_app!(mk_term::op2(BinaryOp::DynExtend(), exp, empty), acc)
             }
         }
@@ -294,11 +297,11 @@ pub fn build_record<I>(fields: I, attrs: RecordAttrs) -> Term
 where
     I: IntoIterator<Item = (FieldPathElem, RichTerm)> + Debug,
 {
-    let mut static_map = HashMap::new();
+    let mut static_fields = HashMap::new();
     let mut dynamic_fields = Vec::new();
 
-    fn insert_static_field(static_map: &mut HashMap<Ident, RichTerm>, id: Ident, t: RichTerm) {
-        match static_map.entry(id) {
+    fn insert_static_field(static_fields: &mut HashMap<Ident, RichTerm>, id: Ident, t: RichTerm) {
+        match static_fields.entry(id) {
             Entry::Occupied(mut occpd) => {
                 // temporary putting null in the entry to take the previous value.
                 let prev = occpd.insert(Term::Null.into());
@@ -313,7 +316,7 @@ where
     }
 
     fields.into_iter().for_each(|field| match field {
-        (FieldPathElem::Ident(id), t) => insert_static_field(&mut static_map, id, t),
+        (FieldPathElem::Ident(id), t) => insert_static_field(&mut static_fields, id, t),
         (FieldPathElem::Expr(e), t) => {
             // Dynamic fields (whose name is defined by an interpolated string) have a different
             // semantics than fields whose name can be determined statically. However, static
@@ -330,16 +333,23 @@ where
                 Term::StrChunks(chunks) => {
                     let mut buffer = String::new();
 
-                    let is_static = chunks.iter().try_for_each(|chunk| match chunk {
-                        StrChunk::Literal(s) => {
-                            buffer.push_str(s);
-                            Ok(())
-                        }
-                        StrChunk::Expr(..) => Err(()),
-                    });
-
-                    if is_static.is_ok() {
-                        insert_static_field(&mut static_map, Ident::new_with_pos(buffer, e.pos), t)
+                    let is_static = chunks
+                        .iter()
+                        .try_for_each(|chunk| match chunk {
+                            StrChunk::Literal(s) => {
+                                buffer.push_str(s);
+                                Ok(())
+                            }
+                            StrChunk::Expr(..) => Err(()),
+                        })
+                        .is_ok();
+
+                    if is_static {
+                        insert_static_field(
+                            &mut static_fields,
+                            Ident::new_with_pos(buffer, e.pos),
+                            t,
+                        )
                     } else {
                         dynamic_fields.push((e, t));
                     }
@@ -351,7 +361,7 @@ where
         }
     });
 
-    Term::RecRecord(static_map, dynamic_fields, attrs, None)
+    Term::RecRecord(RecordData::new(static_fields, attrs), dynamic_fields, None)
 }
 
 /// Merge two fields by performing the merge of both their value and MetaValue if any.
diff --git a/src/pretty.rs b/src/pretty.rs
index de3b74c8..194acd4f 100644
--- a/src/pretty.rs
+++ b/src/pretty.rs
@@ -470,10 +470,10 @@ where
             },
             Var(id) => allocator.as_string(id),
             Enum(id) => allocator.text("`").append(allocator.quote_if_needed(id)),
-            Record(fields, attr) => allocator
+            Record(record) => allocator
                 .line()
                 .append(allocator.intersperse(
-                    sorted_map(fields).iter().map(|&(id, rt)| {
+                    sorted_map(&record.fields).iter().map(|&(id, rt)| {
                         allocator
                             .quote_if_needed(id)
                             .append(allocator.space())
@@ -502,7 +502,7 @@ where
                     }),
                     allocator.line(),
                 ))
-                .append(if attr.open {
+                .append(if record.attrs.open {
                     allocator.line().append(allocator.text(".."))
                 } else {
                     allocator.nil()
@@ -512,15 +512,14 @@ where
                 .group()
                 .braces(),
             RecRecord(
-                fields,
+                record,
                 dyn_fields, /* field whose name is defined by interpolation */
-                attr,
-                _deps, /* dependency tracking between fields. None before the free var pass */
+                _deps,      /* dependency tracking between fields. None before the free var pass */
             ) => allocator
                 .line()
                 .append(
                     allocator.intersperse(
-                        sorted_map(fields)
+                        sorted_map(&record.fields)
                             .iter()
                             .map(|&(id, rt)| {
                                 allocator
@@ -579,7 +578,7 @@ where
                         allocator.line(),
                     ),
                 )
-                .append(if attr.open {
+                .append(if record.attrs.open {
                     allocator.line().append(allocator.text(".."))
                 } else {
                     allocator.nil()
diff --git a/src/program.rs b/src/program.rs
index 123e48bf..f1151184 100644
--- a/src/program.rs
+++ b/src/program.rs
@@ -300,9 +300,9 @@ mod doc {
             Term::MetaValue(MetaValue { doc: Some(md), .. }) => {
                 document.append(parse_documentation(header_level, arena, md, options))
             }
-            Term::Record(map, _) | Term::RecRecord(map, _, _, _) => {
+            Term::Record(record) | Term::RecRecord(record, _, _) => {
                 // Sorting fields for a determinstic output
-                let mut entries: Vec<(_, _)> = map.iter().collect();
+                let mut entries: Vec<(_, _)> = record.fields.iter().collect();
                 entries.sort_by_key(|(k, _)| *k);
 
                 for (ident, rt) in entries {
diff --git a/src/repl/query_print.rs b/src/repl/query_print.rs
index 990ad2b3..0c52e2e7 100644
--- a/src/repl/query_print.rs
+++ b/src/repl/query_print.rs
@@ -191,13 +191,13 @@ fn write_query_result_<R: QueryPrinter>(
     ) -> io::Result<()> {
         writeln!(out)?;
         match t {
-            Term::Record(map, _) if !map.is_empty() => {
-                let mut fields: Vec<_> = map.keys().collect();
+            Term::Record(record) if !record.fields.is_empty() => {
+                let mut fields: Vec<_> = record.fields.keys().collect();
                 fields.sort();
                 renderer.write_fields(out, fields.into_iter())
             }
-            Term::RecRecord(map, dyn_fields, ..) if !map.is_empty() => {
-                let mut fields: Vec<_> = map.keys().collect();
+            Term::RecRecord(record, dyn_fields, ..) if !record.fields.is_empty() => {
+                let mut fields: Vec<_> = record.fields.keys().collect();
                 fields.sort();
                 let dynamic = Ident::from("<dynamic>");
                 fields.extend(dyn_fields.iter().map(|_| &dynamic));
diff --git a/src/repl/rustyline_frontend.rs b/src/repl/rustyline_frontend.rs
index 3901657f..0898ccd8 100644
--- a/src/repl/rustyline_frontend.rs
+++ b/src/repl/rustyline_frontend.rs
@@ -53,15 +53,15 @@ pub fn repl(histfile: PathBuf) -> Result<(), InitError> {
                 let cmd = line.chars().skip(1).collect::<String>().parse::<Command>();
                 let result = match cmd {
                     Ok(Command::Load(path)) => repl.load(&path).map(|term| match term.as_ref() {
-                        Term::Record(map, _) => {
-                             println!("Loaded {} symbol(s) in the environment.", map.len())
+                        Term::Record(record) => {
+                             println!("Loaded {} symbol(s) in the environment.", record.fields.len())
                         }
-                        Term::RecRecord(map, dyn_fields, ..) => {
+                        Term::RecRecord(record, dyn_fields, ..) => {
                             if !dyn_fields.is_empty() {
                                 println!("Warning: loading dynamic fields is currently not supported. {} symbols ignored", dyn_fields.len());
                             }
 
-                            println!("Loaded {} symbol(s) in the environment.", map.len())
+                            println!("Loaded {} symbol(s) in the environment.", record.fields.len())
                         }
                         _ => (),
                     }),
diff --git a/src/serialize.rs b/src/serialize.rs
index c744091c..4cb0d114 100644
--- a/src/serialize.rs
+++ b/src/serialize.rs
@@ -2,8 +2,7 @@
 use crate::{
     error::SerializationError,
     eval::{self, is_empty_optional},
-    identifier::Ident,
-    term::{array::Array, ArrayAttrs, MetaValue, RecordAttrs, RichTerm, Term},
+    term::{array::Array, record::RecordData, ArrayAttrs, MetaValue, RichTerm, Term},
 };
 
 use serde::{
@@ -97,15 +96,12 @@ where
 
 /// Serializer for a record. Serialize fields in alphabetical order to get a deterministic output
 /// (by default, `HashMap`'s randomness implies a randomized order of fields in the output).
-pub fn serialize_record<S>(
-    map: &HashMap<Ident, RichTerm>,
-    _attrs: &RecordAttrs,
-    serializer: S,
-) -> Result<S::Ok, S::Error>
+pub fn serialize_record<S>(record: &RecordData, serializer: S) -> Result<S::Ok, S::Error>
 where
     S: Serializer,
 {
-    let mut entries: Vec<(_, _)> = map
+    let mut entries: Vec<(_, _)> = record
+        .fields
         .iter()
         // Filtering out optional fields without a definition. All variable should have been
         // substituted at this point, so we pass an empty environment.
@@ -122,14 +118,12 @@ where
 }
 
 /// Deserialize for a record. Required to set the record attributes to default.
-pub fn deserialize_record<'de, D>(
-    deserializer: D,
-) -> Result<(HashMap<Ident, RichTerm>, RecordAttrs), D::Error>
+pub fn deserialize_record<'de, D>(deserializer: D) -> Result<RecordData, D::Error>
 where
     D: Deserializer<'de>,
 {
-    let map: HashMap<Ident, RichTerm> = HashMap::deserialize(deserializer)?;
-    Ok((map, Default::default()))
+    let fields = HashMap::deserialize(deserializer)?;
+    Ok(RecordData::with_fields(fields))
 }
 
 /// Serialize for an Array. Required to hide the internal attributes.
@@ -196,8 +190,11 @@ pub fn validate(format: ExportFormat, t: &RichTerm) -> Result<(), SerializationE
             Null if format == ExportFormat::Json || format == ExportFormat::Yaml => Ok(()),
             Null => Err(SerializationError::UnsupportedNull(format, t.clone())),
             Bool(_) | Num(_) | Str(_) | Enum(_) => Ok(()),
-            Record(map, _) => {
-                map.iter().try_for_each(|(_, t)| validate(format, t))?;
+            Record(record) => {
+                record
+                    .fields
+                    .iter()
+                    .try_for_each(|(_, t)| validate(format, t))?;
                 Ok(())
             }
             Array(array, _) => {
diff --git a/src/term.rs b/src/term.rs
index 41896104..08437770 100644
--- a/src/term.rs
+++ b/src/term.rs
@@ -42,6 +42,8 @@ use std::{
     rc::Rc,
 };
 
+use self::record::RecordData;
+
 /// The AST of a Nickel expression.
 ///
 /// Parsed terms also need to store their position in the source for error reporting.  This is why
@@ -98,13 +100,12 @@ pub enum Term {
     /// A record, mapping identifiers to terms.
     #[serde(serialize_with = "crate::serialize::serialize_record")]
     #[serde(deserialize_with = "crate::serialize::deserialize_record")]
-    Record(HashMap<Ident, RichTerm>, RecordAttrs),
+    Record(RecordData),
     /// A recursive record, where the fields can reference each others.
     #[serde(skip)]
     RecRecord(
-        HashMap<Ident, RichTerm>,
+        RecordData,
         Vec<(RichTerm, RichTerm)>, /* field whose name is defined by interpolation */
-        RecordAttrs,
         Option<RecordDeps>, /* dependency tracking between fields. None before the free var pass */
     ),
     /// A switch construct. The evaluation is done by the corresponding unary operator, but we
@@ -417,6 +418,58 @@ impl ArrayAttrs {
     }
 }
 
+pub mod record {
+    use super::{RecordAttrs, RichTerm};
+    use crate::identifier::Ident;
+    use std::collections::HashMap;
+
+    /// The base structure of a Nickel record.
+    ///
+    /// Used to group together fields common to both the [Record](Term::Record) and
+    /// [RecRecord](Term::RecRecord) terms.
+    #[derive(Clone, Debug, Default, PartialEq)]
+    pub struct RecordData {
+        /// Fields whose names are known statically.
+        pub fields: HashMap<Ident, RichTerm>,
+        /// Attributes which may be applied to a record.
+        pub attrs: RecordAttrs,
+    }
+
+    impl RecordData {
+        pub fn new(fields: HashMap<Ident, RichTerm>, attrs: RecordAttrs) -> Self {
+            RecordData { fields, attrs }
+        }
+
+        /// A record with no fields and the default set of attributes.
+        pub fn empty() -> Self {
+            Default::default()
+        }
+
+        /// A record with the provided fields & the default set of attributes.
+        pub fn with_fields(fields: HashMap<Ident, RichTerm>) -> Self {
+            let attrs = Default::default();
+            RecordData { fields, attrs }
+        }
+
+        /// Returns the record resulting from applying the provided function
+        /// to each field.
+        ///
+        /// Note that `f` is taken as `mut` in order to allow it to mutate
+        /// external state while iterating.
+        pub fn map_fields<F>(self, mut f: F) -> Self
+        where
+            F: FnMut(Ident, RichTerm) -> RichTerm,
+        {
+            let fields = self
+                .fields
+                .into_iter()
+                .map(|(id, t)| (id, f(id, t)))
+                .collect();
+            RecordData { fields, ..self }
+        }
+    }
+}
+
 #[derive(Debug, Default, Eq, PartialEq, Copy, Clone)]
 pub struct RecordAttrs {
     pub open: bool,
@@ -700,11 +753,11 @@ impl Term {
                     func(def)
                 }
             }
-            Record(ref mut static_map, _) => {
-                static_map.iter_mut().for_each(|(_, t)| func(t));
+            Record(ref mut r) => {
+                r.fields.iter_mut().for_each(|(_, t)| func(t));
             }
-            RecRecord(ref mut static_map, ref mut dyn_fields, ..) => {
-                static_map.iter_mut().for_each(|(_, t)| func(t));
+            RecRecord(ref mut r, ref mut dyn_fields, ..) => {
+                r.fields.iter_mut().for_each(|(_, t)| func(t));
                 dyn_fields.iter_mut().for_each(|(t1, t2)| {
                     func(t1);
                     func(t2);
@@ -857,8 +910,9 @@ impl Term {
     /// Return a deep string representation of a term, used for printing in the REPL
     pub fn deep_repr(&self) -> String {
         match self {
-            Term::Record(fields, _) | Term::RecRecord(fields, ..) => {
-                let fields_str: Vec<String> = fields
+            Term::Record(r) | Term::RecRecord(r, ..) => {
+                let fields_str: Vec<String> = r
+                    .fields
                     .iter()
                     .map(|(ident, term)| format!("{} = {}", ident, term.as_ref().deep_repr()))
                     .collect();
@@ -1547,23 +1601,20 @@ impl RichTerm {
                     pos,
                 )
             },
-            Term::Record(map, attrs) => {
-                // The annotation on `map_res` uses Result's corresponding trait to convert from
+            Term::Record(record) => {
+                // The annotation on `fields_res` uses Result's corresponding trait to convert from
                 // Iterator<Result> to a Result<Iterator>
-                let map_res: Result<HashMap<Ident, RichTerm>, E> = map
+                let fields_res: Result<HashMap<Ident, RichTerm>, E> = record.fields
                     .into_iter()
                     // For the conversion to work, note that we need a Result<(Ident,RichTerm), E>
                     .map(|(id, t)| t.traverse(f, state, order).map(|t_ok| (id.clone(), t_ok)))
                     .collect();
-                RichTerm::new(
-                    Term::Record(map_res?, attrs),
-                    pos,
-                )
+                RichTerm::new(Term::Record(RecordData::new(fields_res?, record.attrs)), pos)
             },
-            Term::RecRecord(map, dyn_fields, attrs, deps) => {
+            Term::RecRecord(record, dyn_fields, deps) => {
                 // The annotation on `map_res` uses Result's corresponding trait to convert from
                 // Iterator<Result> to a Result<Iterator>
-                let map_res: Result<HashMap<Ident, RichTerm>, E> = map
+                let static_fields_res: Result<HashMap<Ident, RichTerm>, E> = record.fields
                     .into_iter()
                     // For the conversion to work, note that we need a Result<(Ident,RichTerm), E>
                     .map(|(id, t)| Ok((id, t.traverse(f, state, order)?)))
@@ -1578,7 +1629,7 @@ impl RichTerm {
                     })
                     .collect();
                 RichTerm::new(
-                    Term::RecRecord(map_res?, dyn_fields_res?, attrs, deps),
+                    Term::RecRecord(RecordData::new(static_fields_res?, record.attrs), dyn_fields_res?, deps),
                     pos,
                 )
             },
@@ -1813,11 +1864,11 @@ pub mod make {
     macro_rules! mk_record {
         ( $( ($id:expr, $body:expr) ),* ) => {
             {
-                let mut map = std::collections::HashMap::new();
+                let mut fields = std::collections::HashMap::new();
                 $(
-                    map.insert($id.into(), $body.into());
+                    fields.insert($id.into(), $body.into());
                 )*
-                $crate::term::RichTerm::from($crate::term::Term::Record(map, Default::default()))
+                $crate::term::RichTerm::from($crate::term::Term::Record($crate::term::record::RecordData::new(fields, Default::default())))
             }
         };
     }
diff --git a/src/transform/free_vars.rs b/src/transform/free_vars.rs
index b409fd33..49aa3fca 100644
--- a/src/transform/free_vars.rs
+++ b/src/transform/free_vars.rs
@@ -100,20 +100,20 @@ fn collect_free_vars(rt: &mut RichTerm, free_vars: &mut HashSet<Ident>) {
             }
         }
         Term::Sealed(_, t, _) => collect_free_vars(t, free_vars),
-        Term::Record(map, _) => {
-            for t in map.values_mut() {
+        Term::Record(record) => {
+            for t in record.fields.values_mut() {
                 collect_free_vars(t, free_vars);
             }
         }
-        Term::RecRecord(map, dyn_fields, _, deps) => {
-            let rec_fields: HashSet<Ident> = map.keys().cloned().collect();
+        Term::RecRecord(record, dyn_fields, deps) => {
+            let rec_fields: HashSet<Ident> = record.fields.keys().cloned().collect();
             let mut fresh = HashSet::new();
             let mut new_deps = RecordDeps {
-                stat_fields: HashMap::with_capacity(map.len()),
+                stat_fields: HashMap::with_capacity(record.fields.len()),
                 dyn_fields: Vec::with_capacity(dyn_fields.len()),
             };
 
-            for (id, t) in map.iter_mut() {
+            for (id, t) in record.fields.iter_mut() {
                 fresh.clear();
 
                 collect_free_vars(t, &mut fresh);
diff --git a/src/transform/share_normal_form.rs b/src/transform/share_normal_form.rs
index 99d12d2f..df94b92e 100644
--- a/src/transform/share_normal_form.rs
+++ b/src/transform/share_normal_form.rs
@@ -48,26 +48,23 @@ pub fn transform_one(rt: RichTerm) -> RichTerm {
     let pos = rt.pos;
     match_sharedterm! {rt.term,
         with {
-            Term::Record(map, attrs) => {
-                let mut bindings = Vec::with_capacity(map.len());
+            Term::Record(record) => {
+                let mut bindings = Vec::with_capacity(record.fields.len());
 
-                let map = map
-                    .into_iter()
-                    .map(|(id, t)| {
-                        if should_share(&t.term) {
-                            let fresh_var = Ident::fresh();
-                            let pos_t = t.pos;
-                            bindings.push((fresh_var.clone(), t, BindingType::Normal));
-                            (id, RichTerm::new(Term::Var(fresh_var), pos_t))
-                        } else {
-                            (id, t)
-                        }
-                    })
-                    .collect();
+                let record = record.map_fields(|id, t| {
+                    if should_share(&t.term) {
+                        let fresh_var = Ident::fresh();
+                        let pos_t = t.pos;
+                        bindings.push((fresh_var.clone(), t, BindingType::Normal));
+                        RichTerm::new(Term::Var(fresh_var), pos_t)
+                    } else {
+                        t
+                    }
+                });
 
-                with_bindings(Term::Record(map, attrs), bindings, pos)
+                with_bindings(Term::Record(record), bindings, pos)
             },
-            Term::RecRecord(map, dyn_fields, attrs, deps) => {
+            Term::RecRecord(record, dyn_fields, deps) => {
                 // When a recursive record is evaluated, all fields need to be turned to closures
                 // anyway (see the corresponding case in `eval::eval()`), which is what the share
                 // normal form transformation does. This is why the test is more lax here than for
@@ -82,7 +79,7 @@ pub fn transform_one(rt: RichTerm) -> RichTerm {
                 // fields of recursive records contain either a constant or a *generated* variable,
                 // but never a user-supplied variable directly (the former starts with a special
                 // marker). See comments inside [`crate::RichTerm::closurize`] for more details.
-                let mut bindings = Vec::with_capacity(map.len());
+                let mut bindings = Vec::with_capacity(record.fields.len());
 
                 fn mk_binding_type(field_deps: Option<HashSet<Ident>>) -> BindingType {
                     // If the fields has an empty set of dependencies, we can eschew the
@@ -99,25 +96,22 @@ pub fn transform_one(rt: RichTerm) -> RichTerm {
                     }
                 }
 
-                let map = map
-                    .into_iter()
-                    .map(|(id, t)| {
-                        // CHANGE THIS CONDITION CAREFULLY. Doing so can break the post-condition
-                        // explained above.
-                        if !t.as_ref().is_constant() {
-                            let fresh_var = Ident::fresh();
-                            let pos_t = t.pos;
-                            let field_deps = deps.as_ref().and_then(|deps| deps.stat_fields.get(&id)).cloned();
-                            let is_non_rec = (&field_deps).as_ref().map(|deps| deps.is_empty()).unwrap_or(false);
-                            let btype = mk_binding_type(field_deps);
-                            bindings.push((fresh_var.clone(), t, btype));
+                let record = record.map_fields(|id, t| {
+                    // CHANGE THIS CONDITION CAREFULLY. Doing so can break the post-condition
+                    // explained above.
+                    if !t.as_ref().is_constant() {
+                        let fresh_var = Ident::fresh();
+                        let pos_t = t.pos;
+                        let field_deps = deps.as_ref().and_then(|deps| deps.stat_fields.get(&id)).cloned();
+                        let is_non_rec = (&field_deps).as_ref().map(|deps| deps.is_empty()).unwrap_or(false);
+                        let btype = mk_binding_type(field_deps);
+                        bindings.push((fresh_var.clone(), t, btype));
 
-                            (id, RichTerm::new(Term::Var(fresh_var), pos_t))
-                        } else {
-                            (id, t)
-                        }
-                    })
-                    .collect();
+                        RichTerm::new(Term::Var(fresh_var), pos_t)
+                    } else {
+                        t
+                    }
+                });
 
                 let dyn_fields = dyn_fields
                     .into_iter()
@@ -136,7 +130,7 @@ pub fn transform_one(rt: RichTerm) -> RichTerm {
                     })
                     .collect();
 
-                with_bindings(Term::RecRecord(map, dyn_fields, attrs, deps), bindings, pos)
+                with_bindings(Term::RecRecord(record, dyn_fields, deps), bindings, pos)
             },
             Term::Array(ts, attrs) => {
                 let mut bindings = Vec::with_capacity(ts.len());
diff --git a/src/typecheck/eq.rs b/src/typecheck/eq.rs
index 4a786606..f39804bd 100644
--- a/src/typecheck/eq.rs
+++ b/src/typecheck/eq.rs
@@ -268,17 +268,31 @@ fn contract_eq_bounded<E: TermEnvironment>(
                         .unwrap_or(false)
                 })
         }
-        (Record(m1, attrs1), Record(m2, attrs2)) => {
-            map_eq(contract_eq_bounded, state, m1, env1, m2, env2) && attrs1 == attrs2
+        (Record(r1), Record(r2)) => {
+            map_eq(
+                contract_eq_bounded,
+                state,
+                &r1.fields,
+                env1,
+                &r2.fields,
+                env2,
+            ) && r1.attrs == r2.attrs
         }
-        (RecRecord(m1, dyn_fields1, attrs1, _), RecRecord(m2, dyn_fields2, attrs2, _)) =>
+        (RecRecord(r1, dyn_fields1, _), RecRecord(r2, dyn_fields2, _)) =>
         // We only compare records whose field structure is statically known (i.e. without dynamic
         // fields).
         {
             dyn_fields1.is_empty()
                 && dyn_fields2.is_empty()
-                && map_eq(contract_eq_bounded, state, m1, env1, m2, env2)
-                && attrs1 == attrs2
+                && map_eq(
+                    contract_eq_bounded,
+                    state,
+                    &r1.fields,
+                    env1,
+                    &r2.fields,
+                    env2,
+                )
+                && r1.attrs == r2.attrs
         }
         (Array(ts1, attrs1), Array(ts2, attrs2)) => {
             ts1.len() == ts2.len()
diff --git a/src/typecheck/mod.rs b/src/typecheck/mod.rs
index fddba72e..4a551e72 100644
--- a/src/typecheck/mod.rs
+++ b/src/typecheck/mod.rs
@@ -112,8 +112,11 @@ pub fn mk_initial_ctxt(initial_env: &Vec<RichTerm>) -> Result<Context, EnvBuildE
     let bindings = initial_env
         .iter()
         .map(|rt| {
-            if let Term::RecRecord(rec, ..) = rt.as_ref() {
-                Ok(rec.iter().map(|(id, rt)| (id.clone(), rt.clone())))
+            if let Term::RecRecord(record, ..) = rt.as_ref() {
+                Ok(record
+                    .fields
+                    .iter()
+                    .map(|(id, rt)| (id.clone(), rt.clone())))
             } else {
                 Err(EnvBuildError::NotARecord(rt.clone()))
             }
@@ -146,8 +149,8 @@ pub fn env_add_term(
     let RichTerm { term, pos } = rt;
 
     match term.as_ref() {
-        Term::Record(bindings, _) | Term::RecRecord(bindings, ..) => {
-            for (id, t) in bindings {
+        Term::Record(record) | Term::RecRecord(record, ..) => {
+            for (id, t) in &record.fields {
                 let tyw: TypeWrapper = TypeWrapper::from_apparent_type(
                     apparent_type(t.as_ref(), Some(env), Some(resolver)),
                     term_env,
@@ -384,8 +387,8 @@ fn walk<L: Linearizer>(
 
             walk(state, ctxt, lin, linearizer, exp)
         }
-        Term::RecRecord(stat_map, dynamic, ..) => {
-            for (id, field) in stat_map.iter() {
+        Term::RecRecord(record, dynamic, ..) => {
+            for (id, field) in record.fields.iter() {
                 let binding_type = binding_type(
                     state,
                     field.as_ref(),
@@ -399,7 +402,7 @@ fn walk<L: Linearizer>(
             // We don't bind the fields in the term environment used to check for contract
             // equality. See the `Let` case above for more details on why such recursive bindings
             // are currently ignored.
-            stat_map
+            record.fields
                 .iter()
                 .map(|(_, t)| t)
                 .chain(dynamic.iter().map(|(_, t)| t))
@@ -407,8 +410,8 @@ fn walk<L: Linearizer>(
                     walk(state, ctxt.clone(), lin, linearizer.scope(), t)
                 })
         }
-        Term::Record(stat_map, _) => {
-            stat_map
+        Term::Record(record) => {
+            record.fields
                 .iter()
                 .try_for_each(|(_, t)| -> Result<(), TypecheckError> {
                     walk(state, ctxt.clone(), lin, linearizer.scope(), t)
@@ -723,17 +726,18 @@ fn type_check_<L: Linearizer>(
         }
         // If some fields are defined dynamically, the only potential type that works is `{_ : a}`
         // for some `a`
-        Term::RecRecord(stat_map, dynamic, ..) if !dynamic.is_empty() => {
+        Term::RecRecord(record, dynamic, ..) if !dynamic.is_empty() => {
             let ty_dyn = state.table.fresh_unif_var();
 
-            for id in stat_map.keys() {
+            for id in record.fields.keys() {
                 ctxt.type_env.insert(id.clone(), ty_dyn.clone());
                 linearizer.retype_ident(lin, id, ty_dyn.clone())
             }
 
             // We don't bind the fields in the term environment used to check for contract. See
             // `Let` case in `walk`.
-            stat_map
+            record
+                .fields
                 .iter()
                 .try_for_each(|(_, t)| -> Result<(), TypecheckError> {
                     type_check_(
@@ -749,12 +753,12 @@ fn type_check_<L: Linearizer>(
             unify(state, &ctxt, ty, mk_typewrapper::dyn_record(ty_dyn))
                 .map_err(|err| err.into_typecheck_err(state, rt.pos))
         }
-        Term::Record(stat_map, _) | Term::RecRecord(stat_map, ..) => {
+        Term::Record(record) | Term::RecRecord(record, ..) => {
             // For recursive records, we look at the apparent type of each field and bind it in
             // ctxt before actually typechecking the content of fields.
             // Fields defined by interpolation are ignored.
             if let Term::RecRecord(..) = t.as_ref() {
-                for (id, rt) in stat_map {
+                for (id, rt) in &record.fields {
                     let tyw = binding_type(state, rt.as_ref(), &ctxt, true);
                     ctxt.type_env.insert(id.clone(), tyw.clone());
                     linearizer.retype_ident(lin, id, tyw);
@@ -769,7 +773,8 @@ fn type_check_<L: Linearizer>(
 
             if let TypeWrapper::Concrete(AbsType::Dict(rec_ty)) = root_ty {
                 // Checking for a dictionary
-                stat_map
+                record
+                    .fields
                     .iter()
                     .try_for_each(|(_, t)| -> Result<(), TypecheckError> {
                         type_check_(
@@ -782,7 +787,7 @@ fn type_check_<L: Linearizer>(
                         )
                     })
             } else {
-                let row = stat_map.iter().try_fold(
+                let row = record.fields.iter().try_fold(
                     mk_tyw_row!(),
                     |acc, (id, field)| -> Result<TypeWrapper, TypecheckError> {
                         // In the case of a recursive record, new types (either type variables or
@@ -1079,16 +1084,19 @@ pub fn infer_record_type(t: &Term, term_env: &SimpleTermEnvironment) -> TypeWrap
             contracts,
             ..
         }) if contracts.is_empty() => infer_record_type(rt.as_ref(), term_env),
-        Term::Record(rec, ..) | Term::RecRecord(rec, ..) => AbsType::Record(Box::new(
-            TypeWrapper::Concrete(rec.iter().fold(AbsType::RowEmpty(), |r, (id, rt)| {
-                AbsType::RowExtend(
-                    id.clone(),
-                    Some(Box::new(infer_record_type(rt.term.as_ref(), term_env))),
-                    Box::new(r.into()),
-                )
-            })),
-        ))
-        .into(),
+        Term::Record(record) | Term::RecRecord(record, ..) => {
+            AbsType::Record(Box::new(TypeWrapper::Concrete(record.fields.iter().fold(
+                AbsType::RowEmpty(),
+                |r, (id, rt)| {
+                    AbsType::RowExtend(
+                        id.clone(),
+                        Some(Box::new(infer_record_type(rt.term.as_ref(), term_env))),
+                        Box::new(r.into()),
+                    )
+                },
+            ))))
+            .into()
+        }
         t => TypeWrapper::from_apparent_type(
             apparent_type(t, None, None),
             &SimpleTermEnvironment::new(),
diff --git a/src/types.rs b/src/types.rs
index 70eb79e2..639a4de4 100644
--- a/src/types.rs
+++ b/src/types.rs
@@ -54,7 +54,7 @@ use crate::{
     identifier::Ident,
     mk_app, mk_fun,
     term::make as mk_term,
-    term::{RichTerm, Term, TraverseOrder},
+    term::{record::RecordData, RichTerm, Term, TraverseOrder},
 };
 
 use std::{collections::HashMap, fmt};
@@ -375,7 +375,7 @@ impl Types {
                     ),
                 };
 
-                let rec = RichTerm::from(Term::Record(fcs, Default::default()));
+                let rec = RichTerm::from(Term::Record(RecordData::with_fields(fcs)));
 
                 mk_app!(contract::record(), rec, tail)
             }


Test patch:
diff --git a/src/parser/tests.rs b/src/parser/tests.rs
index e6f7448e..12e6b65e 100644
--- a/src/parser/tests.rs
+++ b/src/parser/tests.rs
@@ -4,7 +4,7 @@ use crate::identifier::Ident;
 use crate::parser::error::ParseError as InternalParseError;
 use crate::term::make as mk_term;
 use crate::term::Term::*;
-use crate::term::{BinaryOp, RichTerm, StrChunk, UnaryOp};
+use crate::term::{record, BinaryOp, RichTerm, StrChunk, UnaryOp};
 use crate::{mk_app, mk_switch};
 use assert_matches::assert_matches;
 use codespan::Files;
@@ -212,15 +212,17 @@ fn record_terms() {
     assert_eq!(
         parse_without_pos("{ a = 1, b = 2, c = 3}"),
         RecRecord(
-            vec![
-                (Ident::from("a"), Num(1.).into()),
-                (Ident::from("b"), Num(2.).into()),
-                (Ident::from("c"), Num(3.).into()),
-            ]
-            .into_iter()
-            .collect(),
+            record::RecordData::new(
+                vec![
+                    (Ident::from("a"), Num(1.).into()),
+                    (Ident::from("b"), Num(2.).into()),
+                    (Ident::from("c"), Num(3.).into()),
+                ]
+                .into_iter()
+                .collect(),
+                Default::default(),
+            ),
             Vec::new(),
-            Default::default(),
             None,
         )
         .into()
@@ -229,17 +231,19 @@ fn record_terms() {
     assert_eq!(
         parse_without_pos("{ a = 1, \"%{123}\" = (if 4 then 5 else 6), d = 42}"),
         RecRecord(
-            vec![
-                (Ident::from("a"), Num(1.).into()),
-                (Ident::from("d"), Num(42.).into()),
-            ]
-            .into_iter()
-            .collect(),
+            record::RecordData::new(
+                vec![
+                    (Ident::from("a"), Num(1.).into()),
+                    (Ident::from("d"), Num(42.).into()),
+                ]
+                .into_iter()
+                .collect(),
+                Default::default(),
+            ),
             vec![(
                 StrChunks(vec![StrChunk::expr(RichTerm::from(Num(123.)))]).into(),
                 mk_app!(mk_term::op1(UnaryOp::Ite(), Num(4.)), Num(5.), Num(6.))
             )],
-            Default::default(),
             None,
         )
         .into()
@@ -248,14 +252,16 @@ fn record_terms() {
     assert_eq!(
         parse_without_pos("{ a = 1, \"\\\"%}%\" = 2}"),
         RecRecord(
-            vec![
-                (Ident::from("a"), Num(1.).into()),
-                (Ident::from("\"%}%"), Num(2.).into()),
-            ]
-            .into_iter()
-            .collect(),
+            record::RecordData::new(
+                vec![
+                    (Ident::from("a"), Num(1.).into()),
+                    (Ident::from("\"%}%"), Num(2.).into()),
+                ]
+                .into_iter()
+                .collect(),
+                Default::default(),
+            ),
             Vec::new(),
-            Default::default(),
             None,
         )
         .into()
diff --git a/tests/free_vars.rs b/tests/free_vars.rs
index 30662520..0d13db1d 100644
--- a/tests/free_vars.rs
+++ b/tests/free_vars.rs
@@ -35,7 +35,7 @@ fn check_dyn_vars(expr: &str, expected: Vec<Vec<&str>>) -> bool {
     free_vars::transform(&mut rt);
 
     match rt.term.into_owned() {
-        Term::RecRecord(_, dyns, _, deps) => {
+        Term::RecRecord(_, dyns, deps) => {
             let deps = deps.unwrap();
             dyns.len() == deps.dyn_fields.len() && dyn_free_vars_incl(&deps.dyn_fields, expected)
         }
@@ -48,13 +48,14 @@ fn check_stat_vars(expr: &str, expected: HashMap<&str, Vec<&str>>) -> bool {
     free_vars::transform(&mut rt);
 
     match rt.term.into_owned() {
-        Term::RecRecord(map, _, _, deps) => {
+        Term::RecRecord(record, _, deps) => {
             let deps = deps.unwrap();
             println!(
                 "-- comparing {:#?} **AND* {:#?}",
                 deps.stat_fields, expected
             );
-            map.len() == deps.stat_fields.len() && stat_free_vars_incl(&deps.stat_fields, expected)
+            record.fields.len() == deps.stat_fields.len()
+                && stat_free_vars_incl(&deps.stat_fields, expected)
         }
         _ => panic!("{} hasn't been parsed as a record", expr),
     }