std.mergePatch can't merge over error values

[benley]

I was hoping that this would work:

std.mergePatch(
  { a: error "nope" },
  { a: "ok" }
)

but unfortunately that results in a runtime error, because mergePatch has to check the type of each field, and std.type(error "err") is also a runtime error.

I think it would be possible to make the above example work by checking the type of the replacement value before considering the original value, and ignoring the original value unless the replacement is an object. But it still wouldn't be possible to do this:

std.mergePatch(
  { a: error "nope" },
  { a: { b: "ok" } }
)

So now I'm wondering: Would it break the world to make std.type(error "nope") return "error" instead of an error? Or as an alternative, what about adding a boolean stdlib function like std.isError(x)?

[benley]

also note, in the specific example cases above you could replace std.mergePatch the simple + operator and carry on, but my actual use case was more like

std.mergePatch(
  { a: { b: { c: { d: error "nope" } } } },
  { a: { bb: [stuff], b: { c: { d: { stuff: [morestuff] } } } } }  # Imported from plain JSON
)

... where mergePatch is needed.

[sbarzowski]

I think it would be possible to make the above example work by checking the type of the replacement value before considering the original value, and ignoring the original value unless the replacement is an object.

Makes sense. Actually it makes so much sense we're already almost doing this. It would be enough to remove tailstrict from recursive calls to mergePatch (I've actually tried this and it worked on your example).

Would it break the world to make std.type(error "nope") return "error" instead of an error?

It seems to me that it still wouldn't help:

• It would be necessary to distinguish between "error-which-would-be-object" and "error-which-would-evaulate-to-something else". Well, actually it's even worse, we would need to know what's inside these would-be-objects. This basically requires a schema.
• If instead of a simple runtime error, it was a non-terminating computation, we wouldn't get any result at all.

Perhaps a better solution would be to have a fancy version of mergePatch which gets a schema of target object as an argument. I'm not sure.

[sparkprime]

To paraphrase the question: "I can't implement the laziness of mixin composition with std.mergePatch because it deeply evaluates both sides even if the RHS wins". Two more examples:

std.mergePatch(
  { a: [1,2,3][10] },
  { a: "ok" }
)

std.mergePatch(
  { a: non_terminating_function() },
  { a: "ok" }
)

std.isError(x) is basically try / catch functionality, which would poke that whole discussion with a stick again. It would be preferable if there was a different solution here.

std.type(e) where e is an error returning "error"... Seemingly you could implement std.isError(e) with that, so it would be opening the try / catch can of worms again.

Why is mergePatch needed here? It is possible to do this:

  { a: { b: { c: { d: error "nope" } } } }
  +
  { a: { bb: ['stuff'], b: { c: { d: { stuff: ['morestuff'] } } } } }  # Imported from plain JSON

and get

{
   "a": {
      "b": {
         "c": {
            "d": {
               "stuff": [
                  "morestuff"
               ]
            }
         }
      },
      "bb": [
         "stuff"
      ]
   }
}

edit: Never mind, you cannot put a +: into raw JSON, I understand.

[sparkprime]

The problem I think is that with + you can choose between { x: error "hi" } + { x: { } } and { x: error "hi" } + { x+: { } } where the latter will throw an error analogous to the problem being discussed. However, the std.mergePatch() essentially decides to use +: if the type of both sides is an object, and use : otherwise. But unfortunately it needs to know if the type of the LHS is an object for that, and doing so when the LHS is not a computable function is impossible. Even a hypothetical std.isObject(e) call that returns false if e is an error is not possible to implement if e does not terminate.

The fundamental problem here may be that taking std.mergePatch() (which is designed for operation on raw data) and using it in a context where the LHS is not a computable object.

For your case, maybe one of these options will help:

1. Use a different error marker, so that the LHS is always computable. Maybe a function? Maybe we should have a special value that can be passed around but is not manifestable, specifically for this case where we always want something to be overidden.
2. Use + instead of std.mergePatch, since it forces you to statically know the structure of your data and is therefore less automatic.

[sparkprime]

I don't understand the argument about tailstrict. Here is the code:

    mergePatch(target, patch)::
        if std.type(patch) == "object" then
            local target_object =
                if std.type(target) == "object" then target else {};

            local target_fields =
                if std.type(target_object) == "object" then std.objectFields(target_object) else [];

            local null_fields = [k for k in std.objectFields(patch) if patch[k] == null];
            local both_fields = std.setUnion(target_fields, std.objectFields(patch));

            {
                [k]:
                    if !std.objectHas(patch, k) then
                        target_object[k]
                    else if !std.objectHas(target_object, k) then
                        std.mergePatch(null, patch[k]) tailstrict
                    else
                        std.mergePatch(target_object[k], patch[k]) tailstrict
                for k in std.setDiff(both_fields, null_fields)
            }
        else
            patch,

If std.type(patch) == "object" then std.setDiff(both_fields, null_fields) is evaulated, which forces both_fields which forces target_fields which forces target_object which forces std.type(target).

[sparkprime]

I am more and more thinking that what you want to do is

local external = function() "A marker that causes an error if it reaches the manifestation stage."

std.mergePatch(
  { a: { b: { c: { d: external } } } },
  import "something.json",
)

[sbarzowski]

@sparkprime

I don't understand the argument about tailstrict.

This evaluates just fine as it is, target is never evaluated:

std.mergePatch(error "nope", "ok")

But in the original example it's wrapped in an object:

std.mergePatch(
  { a: error "nope" },
  { a: "ok" }
)

Both target and patch evaluate to objects without errors. But then the recursive call happens:

std.mergePatch(target_object[k], patch[k]) tailstrict

which is effectively:

std.mergePatch(error "nope", "ok") tailstrict

This forces both arguments, even though patch is not an object.

[sparkprime]

I see what you mean, but in his case (RHS is an object) it will be forced internally anyway, even if it is not forced in the tail call.

[sparkprime]

@benley Did the workaround work, can I close this?

[benley]

Yeah - sorry I never followed up on this one after the initial discussion. I'm comfortable with implementing application error values at a higher level than the jsonnet error type.

[sparkprime]

No worries, I have added this to my list of "things that would easier if we could catch exceptions"