From 17686e0fdc1ee2a9c57a06421f6847aadb33258f Mon Sep 17 00:00:00 2001 From: Yann Hamdaoui Date: Fri, 6 Dec 2024 17:01:12 +0100 Subject: [PATCH] Add a freeze primop and stdlib wrapper (#2113) Following a confusing behavior around the interaction of recursive fields and dictionary operations (insert, remove, update), it was proposed in https://github.com/tweag/nickel/issues/1877 to freeze recursive records before applying dictionary operations. This commit adds a primitive operation to do so, which applies pending contracts, flushes them, and remove any dependency data, effectively freezing recursive fields to their current value. A flag is also added to record attributes, in order to remember that some record has already been frozen. Indeed, freezing is linear in the number of fields and pending contracts: freezing at every dictionary operation thus could be costly. Instead, as for closurization, we do it once and then remember that it's been done in flag. It helps dictionary operations preserve frozenness. Only merging or lazy contract applications might require freezing again. --- core/src/bytecode/ast/compat.rs | 2 + core/src/bytecode/ast/primop.rs | 11 +++ core/src/error/mod.rs | 6 +- core/src/eval/operation.rs | 87 ++++++++++++++++++- core/src/parser/grammar.lalrpop | 2 + core/src/parser/lexer.rs | 2 + core/src/term/mod.rs | 6 ++ core/src/term/record.rs | 23 ++++- core/src/typecheck/operation.rs | 6 +- core/stdlib/std.ncl | 38 ++++++++ .../integration/inputs/records/freezing.ncl | 14 +++ .../inputs/records/merge_unfreezes.ncl | 12 +++ 12 files changed, 203 insertions(+), 6 deletions(-) create mode 100644 core/tests/integration/inputs/records/freezing.ncl create mode 100644 core/tests/integration/inputs/records/merge_unfreezes.ncl diff --git a/core/src/bytecode/ast/compat.rs b/core/src/bytecode/ast/compat.rs index b7e73ddb69..8d0b05c3fc 100644 --- a/core/src/bytecode/ast/compat.rs +++ b/core/src/bytecode/ast/compat.rs @@ -539,6 +539,7 @@ impl From<&term::UnaryOp> for PrimOp { ignore_not_exported: *ignore_not_exported, }, term::UnaryOp::RecordEmptyWithTail => PrimOp::RecordEmptyWithTail, + term::UnaryOp::RecordFreeze => PrimOp::RecordFreeze, term::UnaryOp::Trace => PrimOp::Trace, term::UnaryOp::LabelPushDiag => PrimOp::LabelPushDiag, #[cfg(feature = "nix-experimental")] @@ -1080,6 +1081,7 @@ impl FromAst for TermPrimOp { ignore_not_exported: *ignore_not_exported, }), PrimOp::RecordEmptyWithTail => TermPrimOp::Unary(term::UnaryOp::RecordEmptyWithTail), + PrimOp::RecordFreeze => TermPrimOp::Unary(term::UnaryOp::RecordFreeze), PrimOp::Trace => TermPrimOp::Unary(term::UnaryOp::Trace), PrimOp::LabelPushDiag => TermPrimOp::Unary(term::UnaryOp::LabelPushDiag), PrimOp::EnumGetArg => TermPrimOp::Unary(term::UnaryOp::EnumGetArg), diff --git a/core/src/bytecode/ast/primop.rs b/core/src/bytecode/ast/primop.rs index bb45ce9a77..5825f82e7e 100644 --- a/core/src/bytecode/ast/primop.rs +++ b/core/src/bytecode/ast/primop.rs @@ -335,6 +335,15 @@ pub enum PrimOp { /// 1. The model record to take the tail from. RecordEmptyWithTail, + /// Freezes a recursive record to make it a static dictionary. Apply all pending lazy contracts + /// (and flush them), and remove all dependency information, so that the value of the fields is + /// fixed in time and subsequent overrides will only impact the overriden field. + /// + /// # Arguments + /// + /// 1. The record to freeze. + RecordFreeze, + /// Print a message when encountered during evaluation and proceed with the evaluation of the /// argument on the top of the stack. Operationally the same as the identity function /// @@ -964,6 +973,7 @@ impl fmt::Display for PrimOp { StringFindAll => write!(f, "string/find_all"), Force { .. } => write!(f, "force"), RecordEmptyWithTail => write!(f, "record/empty_with_tail"), + RecordFreeze => write!(f, "record/freeze"), Trace => write!(f, "trace"), LabelPushDiag => write!(f, "label/push_diag"), @@ -1091,6 +1101,7 @@ impl PrimOp { | StringFindAll | Force { .. } | RecordEmptyWithTail + | RecordFreeze | Trace | LabelPushDiag | EnumGetArg diff --git a/core/src/error/mod.rs b/core/src/error/mod.rs index 7d7cacf686..6d9fe14e18 100644 --- a/core/src/error/mod.rs +++ b/core/src/error/mod.rs @@ -251,7 +251,8 @@ pub enum IllegalPolymorphicTailAction { FieldAccess { field: String }, Map, Merge, - RecordRemove { field: String }, + FieldRemove { field: String }, + Freeze, } impl IllegalPolymorphicTailAction { @@ -264,9 +265,10 @@ impl IllegalPolymorphicTailAction { } Map => "cannot map over a record sealed by a polymorphic contract".to_owned(), Merge => "cannot merge a record sealed by a polymorphic contract".to_owned(), - RecordRemove { field } => { + FieldRemove { field } => { format!("cannot remove field `{field}` sealed by a polymorphic contract") } + Freeze => "cannot freeze a record sealed by a polymorphic contract".to_owned(), } } } diff --git a/core/src/eval/operation.rs b/core/src/eval/operation.rs index 0eeaa1ba3c..4e1a8f8c64 100644 --- a/core/src/eval/operation.rs +++ b/core/src/eval/operation.rs @@ -645,9 +645,18 @@ impl VirtualMachine { missing_field_err.into_eval_err(pos, pos_op) })?; + // By construction, mapping freezes the record. We set the frozen flag so + // that operations that require the record to be frozen don't have to + // perform the work again. + let attrs = record.attrs.frozen(); + Ok(Closure { body: RichTerm::new( - Term::Record(RecordData { fields, ..record }), + Term::Record(RecordData { + fields, + attrs, + ..record + }), pos_op_inh, ), env: Environment::new(), @@ -1181,6 +1190,73 @@ impl VirtualMachine { } _ => mk_type_error!("Record"), }), + UnaryOp::RecordFreeze => match_sharedterm!(match (t) { + Term::Record(record) => { + let mut record = record; + + if record.attrs.frozen { + // A frozen record shouldn't have a polymorphic tail + debug_assert!(record.sealed_tail.is_none()); + + return Ok(Closure { + body: RichTerm::new(Term::Record(record), pos), + env, + }); + } + + // It's not clear what the semantics of freezing a record with a sealed tail + // would be, as their might be dependencies between the sealed part and the + // unsealed part. Merging is disallowed on records with tail, so we disallow + // freezing as well. + if let Some(record::SealedTail { label, .. }) = record.sealed_tail { + return Err(EvalError::IllegalPolymorphicTailAccess { + action: IllegalPolymorphicTailAction::Freeze, + evaluated_arg: label.get_evaluated_arg(&self.cache), + label, + call_stack: std::mem::take(&mut self.call_stack), + }); + } + + let fields = record + .fields + .into_iter() + .map(|(id, field)| { + let value = field.value.map(|value| { + let pos = value.pos; + RuntimeContract::apply_all( + value, + field.pending_contracts.into_iter(), + pos, + ) + }); + + let field = Field { + value, + pending_contracts: Vec::new(), + ..field + } + .closurize(&mut self.cache, env.clone()); + + (id, field) + }) + .collect(); + + let attrs = record.attrs.frozen(); + + Ok(Closure { + body: RichTerm::new( + Term::Record(RecordData { + fields, + attrs, + sealed_tail: None, + }), + pos_op.into_inherited(), + ), + env, + }) + } + _ => mk_type_error!("Record"), + }), UnaryOp::Trace => { if let Term::Str(s) = &*t { let _ = writeln!(self.trace, "std.trace: {s}"); @@ -2250,6 +2326,7 @@ impl VirtualMachine { )) } _ => Ok(Closure { + // Insertion preserves the frozenness body: Term::Record(RecordData { fields, ..record }).into(), env: env2, }), @@ -2282,7 +2359,7 @@ impl VirtualMachine { match record.sealed_tail.as_ref() { Some(t) if t.has_dyn_field(&id) => { Err(EvalError::IllegalPolymorphicTailAccess { - action: IllegalPolymorphicTailAction::RecordRemove { + action: IllegalPolymorphicTailAction::FieldRemove { field: id.to_string(), }, evaluated_arg: t.label.get_evaluated_arg(&self.cache), @@ -2307,6 +2384,7 @@ impl VirtualMachine { } else { Ok(Closure { body: RichTerm::new( + // Removal preserves the frozenness Term::Record(RecordData { fields, ..record }), pos_op_inh, ), @@ -2745,6 +2823,11 @@ impl VirtualMachine { // documentation let mut record_data = record_data; + // Applying a lazy contract unfreezes a record, as frozen record are + // expected to have all their contracts applied and thus an empty list of + // pending contracts. + record_data.attrs.frozen = false; + let mut contract_at_field = |id: LocIdent| { let pos = contract_term.pos; mk_app!( diff --git a/core/src/parser/grammar.lalrpop b/core/src/parser/grammar.lalrpop index 19b656b329..4c72b3e6b6 100644 --- a/core/src/parser/grammar.lalrpop +++ b/core/src/parser/grammar.lalrpop @@ -1170,6 +1170,7 @@ UOp: PrimOp = { // "op rec_force" => PrimOp::RecForce, // "op rec_default" => PrimOp::RecDefault, "record/empty_with_tail" => PrimOp::RecordEmptyWithTail, + "record/freeze" => PrimOp::RecordFreeze, "trace" => PrimOp::Trace, "label/push_diag" => PrimOp::LabelPushDiag, "eval_nix" =>? { @@ -1617,6 +1618,7 @@ extern { "record/split_pair" => Token::Normal(NormalToken::RecordSplitPair), "record/disjoint_merge" => Token::Normal(NormalToken::RecordDisjointMerge), "record/merge_contract" => Token::Normal(NormalToken::RecordMergeContract), + "record/freeze" => Token::Normal(NormalToken::RecordFreeze), "array/map" => Token::Normal(NormalToken::ArrayMap), "array/generate" => Token::Normal(NormalToken::ArrayGen), "array/at" => Token::Normal(NormalToken::ArrayAt), diff --git a/core/src/parser/lexer.rs b/core/src/parser/lexer.rs index 305252f02c..12eba63410 100644 --- a/core/src/parser/lexer.rs +++ b/core/src/parser/lexer.rs @@ -311,6 +311,8 @@ pub enum NormalToken<'input> { RecordDisjointMerge, #[token("%record/merge_contract%")] RecordMergeContract, + #[token("%record/freeze%")] + RecordFreeze, #[token("merge")] Merge, diff --git a/core/src/term/mod.rs b/core/src/term/mod.rs index 1d115b723a..364b756fdb 100644 --- a/core/src/term/mod.rs +++ b/core/src/term/mod.rs @@ -1538,6 +1538,11 @@ pub enum UnaryOp { /// function that preserves the sealed polymorphic tail of its argument. RecordEmptyWithTail, + /// Freezes a recursive record to make it a static dictionary. Apply all pending lazy contracts + /// (and flush them), and remove all dependency information, so that the value of the fields is + /// fixed in time and subsequent overrides will only impact the overriden field. + RecordFreeze, + /// Print a message when encountered during evaluation and proceed with the evaluation of the /// argument on the top of the stack. Operationally the same as the identity function Trace, @@ -1655,6 +1660,7 @@ impl fmt::Display for UnaryOp { RecDefault => write!(f, "rec_default"), RecForce => write!(f, "rec_force"), RecordEmptyWithTail => write!(f, "record/empty_with_tail"), + RecordFreeze => write!(f, "record/freeze"), Trace => write!(f, "trace"), LabelPushDiag => write!(f, "label/push_diag"), diff --git a/core/src/term/record.rs b/core/src/term/record.rs index beed017b4b..f941a81cb7 100644 --- a/core/src/term/record.rs +++ b/core/src/term/record.rs @@ -23,6 +23,20 @@ pub struct RecordAttrs { /// be closurized by construction. In the meantime, while we need to cope with a unique AST /// across the whole pipeline, we use this flag. pub closurized: bool, + /// If the record has been frozen. + /// + /// A recursive record is frozen when all the lazy contracts are applied to their corresponding + /// fields and flushed from the lazy contracts list. The values of the fields are computed but + /// all dependencies are erased. That is, we turn a recursive, overridable record into a static + /// dictionary. The information about field dependencies is lost and future overriding won't + /// update reverse dependencies. + /// + /// Like `closurized`, we store this information for performance reason: freezing is expensive + /// (linear in the number of fields of the record), and we might need to do it on every + /// dictionary operation such as `insert`, `remove`, etc. (see + /// [#1877](https://github.com/tweag/nickel/issues/1877)). This flags avoid repeated, useless + /// freezing. + pub frozen: bool, } impl RecordAttrs { @@ -30,11 +44,17 @@ impl RecordAttrs { Self::default() } - /// Set the `closurized` flag to true and return the updated attributes. + /// Sets the `closurized` flag to true and return the updated attributes. pub fn closurized(mut self) -> Self { self.closurized = true; self } + + /// Sets the `frozen` flag to true and return the updated attributes. + pub fn frozen(mut self) -> Self { + self.frozen = true; + self + } } impl Combine for RecordAttrs { @@ -42,6 +62,7 @@ impl Combine for RecordAttrs { RecordAttrs { open: left.open || right.open, closurized: left.closurized && right.closurized, + frozen: left.frozen && right.frozen, } } } diff --git a/core/src/typecheck/operation.rs b/core/src/typecheck/operation.rs index dfad995573..d8cd5b3354 100644 --- a/core/src/typecheck/operation.rs +++ b/core/src/typecheck/operation.rs @@ -231,7 +231,11 @@ pub fn get_uop_type( (ty.clone(), ty) } UnaryOp::RecordEmptyWithTail => (mk_uniftype::dynamic(), mk_uniftype::dynamic()), - + // forall a. { _ : a} -> { _ : a } + UnaryOp::RecordFreeze => { + let dict = mk_uniftype::dict(state.table.fresh_type_uvar(var_level)); + (dict.clone(), dict) + } // forall a. Str -> a -> a UnaryOp::Trace => { let ty = state.table.fresh_type_uvar(var_level); diff --git a/core/stdlib/std.ncl b/core/stdlib/std.ncl index 8b9eb3d016..df35ae1571 100644 --- a/core/stdlib/std.ncl +++ b/core/stdlib/std.ncl @@ -3317,6 +3317,44 @@ record |> fields |> std.array.length, + + freeze + : forall a. { _ : a } -> { _ : a } + | doc m%" + Freezes a recursive record: + + - The values of recursive fields are frozen at the current values, and + the dependencies are forgotten. **This means that subsequent overrides + through merging will only affect the overriden fields, but will no + longer update reverse dependencies**. + - All lazy pending contracts accumulated through merging and annotations + will be applied and flushed. **This means that the contracts of this + record won't propagate through future merges anymore after freezing**. + + Note that the documentation and the priority of the fields aren't + affected. + + `freeze` is used internally in the stdlib when applying dictionary + operations, such as `std.record.insert` or `std.record.remove`. Indeed, + interleaving dictionary operations and recursive records can lean to + surprising and unintuitive results. The Nickel stdlib tries to avoid + using records in both recursive and dictionary modes at the same time. + + Freezing is idempotent: freezing an already frozen record has no effect. + + # Examples + + ```nickel multiline + let frozen = std.record.freeze { x = "old", y = x } in + frozen & { x | force = "new" } + # => { x = "new", y = "old" } + + let frozen = std.record.freeze { x | Number = 1 } in + frozen & { x | force = "bypass number contract" } + # => { x = "bypass number contract" } + ``` + "% + = fun record => %record/freeze% record, }, string = { diff --git a/core/tests/integration/inputs/records/freezing.ncl b/core/tests/integration/inputs/records/freezing.ncl new file mode 100644 index 0000000000..7af461cda6 --- /dev/null +++ b/core/tests/integration/inputs/records/freezing.ncl @@ -0,0 +1,14 @@ +# test.type = 'pass' +[ + (%record/freeze% { x = 1, y = x }) & { x | force = 2 } == { x = 2, y = 1 }, + (%record/freeze% { x | default = 1, y = x }) & { x = 2 } == { x = 2, y = 1 }, + (%record/freeze% { x | default = 1, y = x }) + & (%record/freeze% { x = 2, z = x }) + & { x | force = 3 } == { x = 3, y = 1, z = 2 }, + + # freezing, as record mapping, flushes pending contracts and make them not + # propagate anymore + (%record/freeze% {x | String = "a"}) & {x | force = 1} == {x = 1}, + + ] +|> std.test.assert_all diff --git a/core/tests/integration/inputs/records/merge_unfreezes.ncl b/core/tests/integration/inputs/records/merge_unfreezes.ncl new file mode 100644 index 0000000000..752bd2acdd --- /dev/null +++ b/core/tests/integration/inputs/records/merge_unfreezes.ncl @@ -0,0 +1,12 @@ +# test.type = 'error' +# +# [test.metadata] +# error = 'EvalError::BlameError' + +# Test that even if freezing makes the initial `String` contract to not +# propagate, it doesn't prevent other contracts coming from unfrozen records to +# propagate. +%force% ((%record/freeze% { x | String = "a" }) +& { x | priority 1 | Number = 2 } +& { x | priority 2 = false }) +