diff --git a/core/src/eval/mod.rs b/core/src/eval/mod.rs index 20ef42b3ed..40a83e7f6b 100644 --- a/core/src/eval/mod.rs +++ b/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 VirtualMachine { metadata: metadata.clone(), pending_contracts: pending_contracts.clone(), ext_kind, + op_kind: RecordOpKind::ConsiderAllFields, }, name_as_term.clone(), acc, diff --git a/core/src/eval/operation.rs b/core/src/eval/operation.rs index 47736e2647..ff84494dc7 100644 --- a/core/src/eval/operation.rs +++ b/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 VirtualMachine { metadata, pending_contracts, ext_kind, + op_kind, } => { if let Term::Str(id) = &*t1 { match_sharedterm! {t2, with { @@ -1664,13 +1664,13 @@ impl VirtualMachine { 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 VirtualMachine { 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 VirtualMachine { }) } } - } } 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, ))) diff --git a/core/src/parser/grammar.lalrpop b/core/src/parser/grammar.lalrpop index 2a226ff0cb..eb50894caf 100644 --- a/core/src/parser/grammar.lalrpop +++ b/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), diff --git a/core/src/parser/lexer.rs b/core/src/parser/lexer.rs index 9a3b9d1c6a..1dee5ec2e3 100644 --- a/core/src/parser/lexer.rs +++ b/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%")] diff --git a/core/src/term/mod.rs b/core/src/term/mod.rs index 958882e9e0..22c6ebaad7 100644 --- a/core/src/term/mod.rs +++ b/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, 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"), diff --git a/core/src/transform/desugar_destructuring.rs b/core/src/transform/desugar_destructuring.rs index e461d7bb4b..52698dd3e9 100644 --- a/core/src/transform/desugar_destructuring.rs +++ b/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(), diff --git a/core/src/typecheck/operation.rs b/core/src/typecheck/operation.rs index e4b586e097..299d00979b 100644 --- a/core/src/typecheck/operation.rs +++ b/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(), diff --git a/core/tests/integration/pass/records/record_insert.ncl b/core/tests/integration/pass/records/record_insert.ncl new file mode 100644 index 0000000000..0a451cc46b --- /dev/null +++ b/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" }