Fix record.update by making record.insert act consistently

[yannham]

The issue

Although this has been hidden by polymorphic contracts, record.insert has been subtly broken. Most function - if not all - of the stdlib, by design, ignore empty optional fields. This is in particular the case of record.remove and record.has_field. However, record.insert doesn't follow the same convention, for some reason, and fail to insert a value if there's already a field, even if the latter is an empty optional field.

Thus, when defining e.g. record.update, we end up with the following surprising result:

nickel> 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

nickel> let foo | {value | optional, ..} = {bar = "hello"}
nickel> my_insert "value" "world" foo
error: record_insert: tried to extend a record with the field value, but it already exists
  ┌─ <repl-input-2>:8:9
  │
8 │         %record_insert% field r content
  │         ^^^^^^^^^^^^^^^^^^^^^^^ here

Indeed, has_field doesn't see the empty optional field, but %record_insert% does. This is currently mitigated for std.record.insert because polymorphic contracts generates "spurious" values (contract application that, if ever evaluated, would raise a missing field definition), and thus are properly considered by has_field and removed. However, for an upcoming change (contract elision for static type annotation), the polymorphic contract is elided and the behavior above will be triggered from the normal record.update.

Content

I started by making variants of has_field and record_remove that are able to see through empty optional fields. Only then I a realized that the actual issue wasn't that has_field was ignoring empty optionals, but rather than record_insert was the only primop to not do so. Still, I think those variants will prove useful, as I believe we will have demand for the corresponding functions in the stdlib - that is, functions that are able to see and manipulate empty optionals. I propose to let the code stay, even if it's not strictly required for the issue addressed by this PR.

The heart of the fix is then just to make record_insert to be

Backward compatiblity

This makes record.insert succeeds on more programs, as it would refuse to insert something if there was an empty optional already before this change. Now it has the same behavior on previous cases, but it doesn't fail when there is a non-empty optional field already.

[jneem]

I'm not crazy about the naming, because for example it sounds to me like record_insert_all will insert multiple elements into the record. But I don't have a better suggestion right now. If we want to change it later, does renaming primops count as a breaking change, or do we consider them private?

[yannham]

I'm not crazy about the naming, because for example it sounds to me like record_insert_all will insert multiple elements into the record. But I don't have a better suggestion right now. If we want to change it later, does renaming primops count as a breaking change, or do we consider them private?

I entirely agree, and I proceeded nonetheless for this exact reason: primops aren't part of the stability guarantee, so we can rename them as we see fit.