Confusing merge behaviour in std.record.{update,remove}

[solson]

Describe the bug

The std.record.update and std.record.remove functions don't recalculate changes in fields that depend on the modified field, but those recalculations do take effect after a merge, even a trivial one (& {}).

To Reproduce

nickel> std.record.update "a" 2 { a = 1, b = a }
{ a = 2, b = 1, }

nickel> std.record.update "a" 2 { a = 1, b = a } & {}
{ a = 2, b = 2, }

nickel> std.record.remove "a" { a = 1, b = a }
{ b = 1, }

nickel> std.record.remove "a" { a = 1, b = a } & {}
error: unbound identifier `a`
  ┌─ <repl-input-261>:1:36
  │
1 │ std.record.remove "a" { a = 1, b = a } & {}
  │                                    ^ this identifier is unbound

Expected behavior

This might be a language design question, but the least surprising to me would be:

nickel> std.record.update "a" 2 { a = 1, b = a }
{ a = 2, b = 2, }

nickel> std.record.remove "a" { a = 1, b = a }
// Error if you try to use `.b`, so the REPL would error trying to print the result.

(And in general attempt to enforce that x & {} is always equivalent to x if x is a record, unless there is a subtle reason that shouldn't be the case.)

Environment

• OS name + version: nixos-unstable
• Version of the code: Nickel 1.5.0

Additional context

I ran into this while testing out some more complicated merging/overriding scenarios, and this behaviour was a lot more surprising in context before I tracked down what made "recalculation" of a field happen or not happen.

[yannham]

Thanks for reporting. This is definitely a bug. I think the original design guideline was that only merge could impact recursive fields, and as soon as you use a record as a "normal" dictionary (std.record.{update,remove,insert}), you don't get the recursive behavior (otherwise, you should just use merge instead - although remove can't be emulated with merge, but this is on purpose).

Following this guideline, I believe the right result should be the following:

nickel> std.record.update "a" 2 { a = 1, b = a }
{ a = 2, b = 1, }

nickel> std.record.update "a" 2 { a = 1, b = a } & {}
{ a = 2, b = 1, }

nickel> std.record.remove "a" { a = 1, b = a }
{ b = 1, }

nickel> std.record.remove "a" { a = 1, b = a } & {}
{ b = 1, }

I see two ways to achieve that:

1. We "freeze" the recursive record before any remove primop. We don't really care about insert: insert alone can't mess with existing dependencies, it's either remove alone or the combination of remove and insert that are problematic. This way, there's no more confusion: prior to a remove, the recursive fixpoint is computed once and then it's just a normal data dictionary like in Python with no more recursive references.
2. The drawback of the previous solution is that it makes it impossible to override or recompute any other value in the original record after a remove, even if it doesn't have anything to do with the removed value: (std.record.remove "a" {a = 1, b = c, c = 2}) & {c | force = 3} would give {b = 2, c = 3}, which might be surprising. We can freeze only the value of the field that is removed instead, and let other fields keep their recursive/overriding capabilities.

I think 1. is a simpler mental model: you don't have to think about what fields were removed or not, as soon as you use one remove operation, all recursive fields are frozen. 2. is probably more flexible, though.