Fix record.update by making record.insert act consistently

core/src/eval/mod.rs

@@ -85,7 +85,8 @@ use crate::{
         array::ArrayAttrs,
         make as mk_term,
         record::{Field, RecordData},
-        BinaryOp, BindingType, LetAttrs, RichTerm, RuntimeContract, StrChunk, Term, UnaryOp,
+        BinaryOp, BindingType, LetAttrs, RecordOpKind, RichTerm, RuntimeContract, StrChunk, Term,
+        UnaryOp,
     },
     transform::Closurizable,
 };
@@ -608,6 +609,7 @@ impl<R: ImportResolver, C: Cache> VirtualMachine<R, C> {
                                         metadata: metadata.clone(),
                                         pending_contracts: pending_contracts.clone(),
                                         ext_kind,
+                                        op_kind: RecordOpKind::ConsiderAllFields,
                                     },
                                     name_as_term.clone(),
                                     acc,

core/src/eval/operation.rs

@@ -27,8 +27,7 @@ use crate::{
         make as mk_term,
         record::{self, Field, FieldMetadata, RecordData},
         string::NickelString,
-        BinaryOp, CompiledRegex, IndexMap, MergePriority, NAryOp, Number, RecordExtKind, RichTerm,
-        RuntimeContract, SharedTerm, StrChunk, Term, UnaryOp,
+        *,
     },
     transform::Closurizable,
 };
@@ -1634,6 +1633,7 @@ impl<R: ImportResolver, C: Cache> VirtualMachine<R, C> {
                 metadata,
                 pending_contracts,
                 ext_kind,
+                op_kind,
             } => {
                 if let Term::Str(id) = &*t1 {
                     match_sharedterm! {t2, with {
@@ -1664,13 +1664,13 @@ impl<R: ImportResolver, C: Cache> VirtualMachine<R, C> {
                                     LocIdent::from(id),
                                     Field { value, metadata, pending_contracts }
                                 ) {
-                                    //TODO: what to do on insertion where an empty optional field
-                                    //exists? Temporary: we fail with existing field exception
-                                    Some(t) => Err(EvalError::Other(format!(
-                                        "record_insert: \
-                                        tried to extend a record with the field {id}, \
-                                        but it already exists"
-                                    ), pos_op)),
+                                    Some(t) if matches!(op_kind, RecordOpKind::ConsiderAllFields)
+                                        || !t.is_empty_optional() =>
+                                        Err(EvalError::Other(format!(
+                                            "record_insert: \
+                                            tried to extend a record with the field {id}, \
+                                            but it already exists"
+                                        ), pos_op)),
                                     _ => Ok(Closure {
                                         body: Term::Record(RecordData { fields, ..record }).into(),
                                         env: env2,
@@ -1685,40 +1685,41 @@ impl<R: ImportResolver, C: Cache> VirtualMachine<R, C> {
                     Err(mk_type_error!("record_insert", "String", 1, t1, pos1))
                 }
             }
-            BinaryOp::DynRemove() => match_sharedterm! {t1, with {
+            BinaryOp::DynRemove(op_kind) => match_sharedterm! {t1, with {
                     Term::Str(id) => match_sharedterm! {t2, with {
                             Term::Record(record) => {
                                 let mut fields = record.fields;
                                 let fetched = fields.remove(&LocIdent::from(&id));
-                                match fetched {
-                                    None
-                                    | Some(Field {
+                                if fetched.is_none()
+                                    || matches!((op_kind, fetched), (RecordOpKind::IgnoreEmptyOpt, Some(Field {
                                         value: None,
                                         metadata: FieldMetadata { opt: true, ..},
                                         ..
-                                      }) => match record.sealed_tail.as_ref() {
-                                        Some(t) if t.has_dyn_field(&id) =>
-                                            Err(EvalError::IllegalPolymorphicTailAccess {
-                                                action: IllegalPolymorphicTailAction::RecordRemove {
-                                                    field: id.to_string()
-                                                },
-                                                evaluated_arg: t.label
-                                                    .get_evaluated_arg(&self.cache),
-                                                label: t.label.clone(),
-                                                call_stack: std::mem::take(&mut self.call_stack)
-                                            }
-                                        ),
-                                        _ => Err(EvalError::FieldMissing(
-                                            id.into_inner(),
-                                            String::from("record_remove"),
-                                            RichTerm::new(
-                                                Term::Record(RecordData { fields, ..record }),
-                                                pos2,
+                                      }))) {
+                                        match record.sealed_tail.as_ref() {
+                                            Some(t) if t.has_dyn_field(&id) =>
+                                                Err(EvalError::IllegalPolymorphicTailAccess {
+                                                    action: IllegalPolymorphicTailAction::RecordRemove {
+                                                        field: id.to_string()
+                                                    },
+                                                    evaluated_arg: t.label
+                                                        .get_evaluated_arg(&self.cache),
+                                                    label: t.label.clone(),
+                                                    call_stack: std::mem::take(&mut self.call_stack)
+                                                }
                                             ),
-                                            pos_op,
-                                        )),
+                                            _ => Err(EvalError::FieldMissing(
+                                                id.into_inner(),
+                                                String::from("record_remove"),
+                                                RichTerm::new(
+                                                    Term::Record(RecordData { fields, ..record }),
+                                                    pos2,
+                                                ),
+                                                pos_op,
+                                            )),
+                                        }
                                     }
-                                    _ => {
+                                    else {
                                         Ok(Closure {
                                             body: RichTerm::new(
                                                 Term::Record(RecordData { fields, ..record }),
@@ -1728,22 +1729,21 @@ impl<R: ImportResolver, C: Cache> VirtualMachine<R, C> {
                                         })
                                     }
                                 }
-                            }
                         } else {
                             Err(mk_type_error!("record_remove", "Record", 2, t2, pos2))
                         }
-                    },
+                    }
                 } else {
                     Err(mk_type_error!("record_remove", "String", 1, t1, pos1))
                 }
             },
-            BinaryOp::HasField() => match_sharedterm! {t1, with {
+            BinaryOp::HasField(op_kind) => match_sharedterm! {t1, with {
                     Term::Str(id) => {
                         if let Term::Record(record) = &*t2 {
                             Ok(Closure::atomic_closure(RichTerm::new(
                                 Term::Bool(matches!(
                                     record.fields.get(&LocIdent::from(id.into_inner())),
-                                    Some(field) if !field.is_empty_optional()
+                                    Some(field) if matches!(op_kind, RecordOpKind::ConsiderAllFields) || !field.is_empty_optional()
                                 )),
                                 pos_op_inh,
                             )))

core/src/parser/grammar.lalrpop

@@ -841,7 +841,8 @@ BOpPre: BinaryOp = {
     "unseal" => BinaryOp::Unseal(),
     "seal" => BinaryOp::Seal(),
     "go_field" => BinaryOp::GoField(),
-    "has_field" => BinaryOp::HasField(),
+    "has_field" => BinaryOp::HasField(RecordOpKind::IgnoreEmptyOpt),
+    "has_field_all" => BinaryOp::HasField(RecordOpKind::ConsiderAllFields),
     "elem_at" => BinaryOp::ArrayElemAt(),
     "hash" => BinaryOp::Hash(),
     "serialize" => BinaryOp::Serialize(),
@@ -853,8 +854,16 @@ BOpPre: BinaryOp = {
         ext_kind: RecordExtKind::WithValue,
         metadata: Default::default(),
         pending_contracts: Default::default(),
+        op_kind: RecordOpKind::IgnoreEmptyOpt,
     },
-    "record_remove" => BinaryOp::DynRemove(),
+    "record_insert_all" => BinaryOp::DynExtend {
+        ext_kind: RecordExtKind::WithValue,
+        metadata: Default::default(),
+        pending_contracts: Default::default(),
+        op_kind: RecordOpKind::ConsiderAllFields,
+    },
+    "record_remove" => BinaryOp::DynRemove(RecordOpKind::IgnoreEmptyOpt),
+    "record_remove_all" => BinaryOp::DynRemove(RecordOpKind::ConsiderAllFields),
     "label_with_message" => BinaryOp::LabelWithMessage(),
     "label_with_notes" => BinaryOp::LabelWithNotes(),
     "label_append_note" => BinaryOp::LabelAppendNote(),
@@ -1036,7 +1045,9 @@ extern {
         "record_map" => Token::Normal(NormalToken::RecordMap),
         "record_empty_with_tail" => Token::Normal(NormalToken::RecordEmptyWithTail),
         "record_insert" => Token::Normal(NormalToken::RecordInsert),
+        "record_insert_all" => Token::Normal(NormalToken::RecordInsertAll),
         "record_remove" => Token::Normal(NormalToken::RecordRemove),
+        "record_remove_all" => Token::Normal(NormalToken::RecordRemoveAll),
         "record_seal_tail" => Token::Normal(NormalToken::RecordSealTail),
         "record_unseal_tail" => Token::Normal(NormalToken::RecordUnsealTail),
         "seq" => Token::Normal(NormalToken::Seq),
@@ -1052,6 +1063,7 @@ extern {
         "lookup_type_variable" => Token::Normal(NormalToken::LookupTypeVar),
 
         "has_field" => Token::Normal(NormalToken::HasField),
+        "has_field_all" => Token::Normal(NormalToken::HasFieldAll),
         "map" => Token::Normal(NormalToken::Map),
         "generate" => Token::Normal(NormalToken::ArrayGen),
         "elem_at" => Token::Normal(NormalToken::ElemAt),

core/src/parser/lexer.rs

@@ -221,8 +221,12 @@ pub enum NormalToken<'input> {
     RecordMap,
     #[token("%record_insert%")]
     RecordInsert,
+    #[token("%record_insert_all%")]
+    RecordInsertAll,
     #[token("%record_remove%")]
     RecordRemove,
+    #[token("%record_remove_all%")]
+    RecordRemoveAll,
     #[token("%record_empty_with_tail%")]
     RecordEmptyWithTail,
     #[token("%record_seal_tail%")]
@@ -248,6 +252,8 @@ pub enum NormalToken<'input> {
 
     #[token("%has_field%")]
     HasField,
+    #[token("%has_field_all%")]
+    HasFieldAll,
     #[token("%map%")]
     Map,
     #[token("%elem_at%")]

core/src/term/mod.rs

@@ -1340,6 +1340,19 @@ pub enum RecordExtKind {
     WithoutValue,
 }
 
+/// A flavor for record operations. By design, we want empty optional values to be transparent for
+/// record operations, because they would otherwise make many operations fail spuriously (e.g.
+/// trying to map over such an empty value). So they are most of the time silently ignored.
+///
+/// However, it's sometimes useful and even necessary to take them into account. This behavior is
+/// controlled by [RecordOpKind].
+#[derive(Clone, Debug, PartialEq, Eq, Copy, Default)]
+pub enum RecordOpKind {
+    #[default]
+    IgnoreEmptyOpt,
+    ConsiderAllFields,
+}
+
 /// Primitive binary operators
 #[derive(Clone, Debug, PartialEq)]
 pub enum BinaryOp {
@@ -1412,16 +1425,17 @@ pub enum BinaryOp {
         metadata: FieldMetadata,
         pending_contracts: Vec<RuntimeContract>,
         ext_kind: RecordExtKind,
+        op_kind: RecordOpKind,
     },
 
     /// Remove a field from a record. The field name is given as an arbitrary Nickel expression.
-    DynRemove(),
+    DynRemove(RecordOpKind),
 
     /// Access the field of record. The field name is given as an arbitrary Nickel expression.
     DynAccess(),
 
     /// Test if a record has a specific field.
-    HasField(),
+    HasField(RecordOpKind),
 
     /// Concatenate two arrays.
     ArrayConcat(),
@@ -1504,10 +1518,19 @@ impl fmt::Display for BinaryOp {
             ApplyContract() => write!(f, "apply_contract"),
             Unseal() => write!(f, "unseal"),
             GoField() => write!(f, "go_field"),
-            DynExtend { .. } => write!(f, "record_insert"),
-            DynRemove() => write!(f, "record_remove"),
+            DynExtend {
+                op_kind: RecordOpKind::IgnoreEmptyOpt,
+                ..
+            } => write!(f, "record_insert"),
+            DynExtend {
+                op_kind: RecordOpKind::ConsiderAllFields,
+                ..
+            } => write!(f, "record_insert_all"),
+            DynRemove(RecordOpKind::IgnoreEmptyOpt) => write!(f, "record_remove"),
+            DynRemove(RecordOpKind::ConsiderAllFields) => write!(f, "record_remove_all"),
             DynAccess() => write!(f, "dyn_access"),
-            HasField() => write!(f, "has_field"),
+            HasField(RecordOpKind::IgnoreEmptyOpt) => write!(f, "has_field"),
+            HasField(RecordOpKind::ConsiderAllFields) => write!(f, "has_field_all"),
             ArrayConcat() => write!(f, "array_concat"),
             ArrayElemAt() => write!(f, "elem_at"),
             Merge(_) => write!(f, "merge"),

core/src/transform/desugar_destructuring.rs

@@ -33,9 +33,12 @@
 use crate::destructuring::{FieldPattern, Match, RecordPattern};
 use crate::identifier::LocIdent;
 use crate::match_sharedterm;
-use crate::term::make::{op1, op2};
-use crate::term::{BinaryOp::DynRemove, RichTerm, Term, TypeAnnotation, UnaryOp::StaticAccess};
-use crate::term::{BindingType, LetAttrs};
+use crate::term::{
+    make::{op1, op2},
+    BinaryOp::DynRemove,
+    BindingType, LetAttrs, RecordOpKind, RichTerm, Term, TypeAnnotation,
+    UnaryOp::StaticAccess,
+};
 
 /// Entry point of the patterns desugaring.
 /// It desugar a `RichTerm` if possible (the term is a let pattern or a function with patterns in
@@ -156,9 +159,11 @@ fn bind_open_field(x: LocIdent, pat: &RecordPattern, body: RichTerm) -> RichTerm
     Term::Let(
         var,
         matches.iter().fold(Term::Var(x).into(), |x, m| match m {
-            Match::Simple(i, _) | Match::Assign(i, _, _) => {
-                op2(DynRemove(), Term::Str((*i).into()), x)
-            }
+            Match::Simple(i, _) | Match::Assign(i, _, _) => op2(
+                DynRemove(RecordOpKind::default()),
+                Term::Str((*i).into()),
+                x,
+            ),
         }),
         body,
         Default::default(),

core/src/typecheck/operation.rs

@@ -292,7 +292,7 @@ pub fn get_bop_type(
             )
         }
         // forall a. Str -> { _ : a } -> { _ : a}
-        BinaryOp::DynRemove() => {
+        BinaryOp::DynRemove(_) => {
             let res = state.table.fresh_type_uvar(var_level);
             (
                 mk_uniftype::str(),
@@ -301,7 +301,7 @@ pub fn get_bop_type(
             )
         }
         // forall a. Str -> {_: a} -> Bool
-        BinaryOp::HasField() => {
+        BinaryOp::HasField(_) => {
             let ty_elt = state.table.fresh_type_uvar(var_level);
             (
                 mk_uniftype::str(),

core/tests/integration/pass/records/record_insert.ncl

@@ -0,0 +1,15 @@
+# test.type = 'pass'
+
+# regression test for record insertion and record removal not being consistent
+# with respect to optional fields
+let my_insert = fun field content r =>
+  let r =
+    if %has_field% field r then
+      %record_remove% field r
+    else
+      r
+  in
+  %record_insert% field r content
+in
+let foo | { value | optional, .. } = { bar = "hello" } in
+my_insert "value" "world" foo == { bar = "hello", value = "world" }