Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

std.mergePatch doesn't respect laziness #216

Closed
andrewmchen opened this issue Jul 11, 2016 · 8 comments
Closed

std.mergePatch doesn't respect laziness #216

andrewmchen opened this issue Jul 11, 2016 · 8 comments

Comments

@andrewmchen
Copy link
Contributor

andrewmchen commented Jul 11, 2016

For example

//a.jsonnet
{
  a: "hello",
  b: {field_to_override: $.a}
}
//b.jsonnet
local a = import "a.jsonnet";

a + {
  a: "override",
  b: std.mergePatch(a["b"], {new: "field"})
}

RESULTS IN:

{
   "a": "override",
   "b": {
      "field_to_override": "hello",
      "new": "field"
   }
}

Whereas I expected field_to_override to be override.

@andrewmchen andrewmchen reopened this Jul 11, 2016
@andrewmchen andrewmchen changed the title Imports not lazily evaluated. std.mergePatch doesn't respect laziness Jul 11, 2016
@sparkprime
Copy link
Contributor

sparkprime commented Jul 12, 2016

When you use a in a["b"] (in fact you could just use a.b) you are referring directly to the object defined in a.jsonnet which has had no override operator (+) applied to it. The fact you are inside an object that has been mixed in with a does not change the meaning of a.

The mergePatch just adds "new": "field" to it, which doesn't affect the value in "field_to_override", so that's a red herring.

It sounds like what you wanted was this:

local a = {
  a: "hello",
  b: {field_to_override: $.a}
};

a + {
  a: "override",
  b: std.mergePatch(super.b, {new: "field"})
}

Or even just

local a = {
  a: "hello",
  b: {field_to_override: $.a}
};

a + {
  a: "override",
  b+: {new: "field"}
}

BTW This is not about laziness but about late binding. The difference is:

Lazy evaluation chooses to evaluate the expression bound to a variable at the first time the variable is used, as opposed to when the binding was made (which is almost always much earlier). The practical difference is that with laziness the expression will not be evaluated unless it is actually needed, which becomes important when you have error statements / asserts etc that could fire in that expression (see also non-termination although that does not occur in practice in Jsonnet).

Late binding chooses the expression to be evaluated depending on the concrete class of the target object. I.e., you don't know what the expression even is going to be until execution time when the concrete class is known. Jsonnet doesn't have classes but the concept is the same for prototype inheritance.

@sparkprime
Copy link
Contributor

Closing for inactivity, please re-open if you like

@andrewmchen
Copy link
Contributor Author

Thanks for the detailed explanation!

@ekimekim
Copy link

ekimekim commented Oct 10, 2018

Sorry to bring up such an old issue, but I'm hitting this too and I think I have a clearer example:

std.mergePatch({a: 1, b: self.a}, {a: 2}).b

The output is 1. I would expect the output to be 2, since self should refer to the post-merge object when I try to access b.

This produces the same result (the output has b = 1):

std.mergePatch({a:1, b: $.a}, {a: 2})

where I'm now explicitly referring to the post-merge object by $.

But very interestingly, this produces 2:

local x = std.mergePatch({a:1, b: x.a}, {a: 2}); x.b

and I'm at a complete loss as to why.

At the very least I think that the fact the third example is inconsistent with the first two is a bug. I'd also argue that mergePatch failing to respect late binding this way ("making everything concrete", if that's the right terminology), is surprising and would be better if it acted more like a normal a + b merge.

Edit: This also works:

{x: std.mergePatch({a:1, b: $.x.a}, {a:2})}

output:

{
   "x": {
      "a": 2,
      "b": 2
   }
}

Edit 2: After reading #414, I think I understand better some of the difficulties in avoiding making values concrete when doing a mergePatch. However I'm still not sure why my self example doesn't work when the local one does.

@sparkprime
Copy link
Contributor

This example:

local x = std.mergePatch({a:1, b: x.a}, {a: 2}); x.b

is equivalent to (substitute the x once)

local x = std.mergePatch({a:1, b: (std.mergePatch({a:1, b: x.a}, {a: 2})).a}, {a: 2}); x.b

which if you resolve the .a in the middle:

local x = std.mergePatch({a:1, b: 2}, {a: 2}); x.b

The example with $ is similar:

{x: std.mergePatch({a:1, b: $.x.a}, {a:2})}
{x: std.mergePatch({a:1, b: {x: std.mergePatch({a:1, b: $.x.a}, {a:2})}.x.a}, {a:2})}
{x: std.mergePatch({a:1, b: (std.mergePatch({a:1, b: $.x.a}, {a:2})}).a}, {a:2})}
{x: std.mergePatch({a:1, b: 2}, {a:2})}

These self-referential structures allow you to do something which is a bit like late binding, but it's coing from the recursive let, not from the mergePatch.

@sparkprime
Copy link
Contributor

Otherwise, mergePatch is a JSON thing, so your Jsonnet objects are getting resolved to JSON before executing it, which is why self doesn't work anymore.

@ekimekim
Copy link

Ah, that makes perfect sense, thanks.

mergePatch is a JSON thing, so your Jsonnet objects are getting resolved to JSON before executing it

Since it looks like I'm not the first one to have the wrong expectations here, maybe it's worth explicitly calling this out in the mergePatch documentation?

@sparkprime
Copy link
Contributor

Agreed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants