-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
Cyclic object assertions behave in a different way than other jsonnet implementations #125
Cyclic object assertions behave in a different way than other jsonnet implementations #125
Conversation
I have jsonnet code ressembling the `assert_recursion_allowed.jsonnet` file This file evaluates fine in C jsonnet and go jsonnet but fails here because of `infinite recursion detected` The fix I propose here is to now fail when recursion is detected but rather keep going until stack overflow is reached (as is done in other jsonnet interpreters) I also modified the test for issue CertainLach#23. This issue is still fixed IMO because it doesn't panic. Instead, it prints out the full stack, which is what other jsonnet interpreters do, and it is a meaningful error message as well Note: I am a complete Rust noob. My fix may be terrible. Let me know if it is!
There is no problem with infinite recursion detection. It works fine in all cases. The real issue here is with object assertion implementation, which is implemented closely to spec, yet other jsonnet implementations don't follow this spec: google/jsonnet#451 (comment)
According to the spec, the code you provided should be executed as {
enabled: self.obj.enabled && !$.obj.disabled,
obj:: {
local asserts = self.enabled && $.enabled && self.disabled == false,
enabled: assert asserts; true,
disabled: assert asserts; false,
},
} Thus, in fact, causing the infinite recursion. But, I think the better solution here will not be copying the semantics of other jsonnet implementations and altering the spec, but adding the proposed Cc: @sbarzowski |
This issue is probably the biggest (and the only serious) problem with Jsonnet official semantics. The de facto semantics do not match the official the official ones and do not have desirable properties (e.g. it breaks referential transparency). In this case, breaking the strict backwards compatibility, within reason, is justified. If we can agree on precise new semantics, I'll be happy to help to bring it to go-jsonnet and (maybe) cpp-jsonnet.
I'm pretty sure noop will lead to confusion. I think it should actually skip assertion checks if we add it. No-op perhaps a good compromise if it turns out difficult. @sparkprime FYI |
I believe the implementation skips evaluating the assertions the "2nd time around" therefore not getting stuck in infinite loops. There is no mention of that in the spec. But i'm not sure how it leads to breaking referential transparency? |
@sparkprime given that local s = {
assert (s {}).v,
v: true,
};
(s {}).v should work the same way as local s = {
assert sExtended.v,
v: true,
},
sExtended = s {};
sExtended.v But it doesn't, it only works because of the caching in the second snippet. In jrsonnet this causes a failure, because during field computation, you are not allowed to evaluate the same field (which is a valid assumption, if following the spec and counting on a referential transparency), the following code is causing infinite recursion error: {f: self.f} Because the value of field
So the same errors arise when we need to execute assertion dependent on the current field. Thus, first snippet fails in jsonnet/go-jsonnet with the stack overflow error, and in jrsonnet with the infinite recursion error, and the second snippet works in jsonnet/go-jsonnet, and fails in jrsonnet with the same infinite recursion error. |
I see. Using the reference implementation, you can simply say that this working program: local
s = {
assert s.v,
v: true,
};
s.v should behave the same as this one which does not: local
s = {
assert (s+{}).v,
v: true,
};
s.v and also this one which does not: local
s() = {
assert s().v,
v: true,
};
s().v I think it can be boiled down to creating a new object within the loop and therefore having a new cache that would otherwise break the loop. In the 2nd case I think it's because the cache is at the root (because there can be late binding in the assert expression) and the root is a new object. I need to read the other bug from 2018 still, I just noticed there is a lot of discussion there. |
I have jsonnet code ressembling the
assert_recursion_allowed.jsonnet
fileThis file evaluates fine in C jsonnet and go jsonnet but fails here because of
infinite recursion detected
The fix I propose here is to not fail when recursion is detected but rather keep going until stack overflow is reached (as is done in other jsonnet interpreters)
I also modified the test for issue #23. This issue is still fixed IMO because it doesn't panic. Instead, it prints out the full stack, which is what other jsonnet interpreters do, and it is a meaningful error message as well
Note: I am a complete Rust noob. My fix may be terrible. Let me know if it is!