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

Fix: field in super should not be interpreted with standalone-super concept. #154

Merged
merged 1 commit into from
Mar 21, 2024

Conversation

CertainLach
Copy link
Owner

Jsonnet spec defines special behavior for super.field and field in super expressions, while in jrsonnet it is implemented by standalone-super concept, with optimizations in case of super.field.

However, standalone-super is conflicting with field in super behavior, so special handling is needed for this expression.

Fixes: #153

…oncept.

Jsonnet spec defines special behavior for `super.field` and
`field in super` expressions, while in jrsonnet it is implemented by
standalone-super concept, with optimizations in case of `super.field`.

However, standalone-super is conflicting with `field in super` behavior,
so special handling is needed for this expression.

Fixes: #153
@CertainLach CertainLach requested a review from JarvisCraft March 20, 2024 19:29
@CertainLach CertainLach added this to the v0.5.0 milestone Mar 20, 2024
@CertainLach
Copy link
Owner Author

Note that jrsonnet-specific value-path is not affected, and

local upper = {
  'bar': super?.baz ?? 'nope',
};

local obj = {
  foo+: upper,
};

obj

Already works as intended

@davidreuss
Copy link

davidreuss commented Mar 20, 2024

Oh, awesome -- i can confirm that this works, and removes the problem in our project ... 🎉

Now ... i does look like we may be hitting other problems further along in evaluation now, but ... this particular problem is solved 🥇

I'm currently staring at a stack overflow, but ... i'll open another issue for that, when i have a good reproduction case, if possible to figure that out.

Thanks so much!


edit: evaluation succeeds if i run it with --max-stack 1000 ... looks like there's maybe some code we need to take a deeper look at 😂

@CertainLach
Copy link
Owner Author

CertainLach commented Mar 20, 2024

edit: evaluation succeeds if i run it with --max-stack 1000 ... looks like there's maybe some code we need to take a deeper look at 😂

All implementations, sjsonnet, jrsonnet and go-jsonnet/jsonnet-cpp (Those two match in that regard) have different stack frame assignment methods, so this is normal if jrsonnet fails with lower max-stack value.

This is not necessary you have some things to fix at your side, just use larger max-stack value.
I might increase default max-stack value, since jrsonnet implements infinite recursion detection, and it is less useful here now.

Copy link

@davidreuss davidreuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome! 🎉

For reference, this takes a ~120s evaluation down to ~15s ... but it has to be said that we're also removing a bunch of validation which we have natively available in our own build of go-jsonnet in this calculation ....

but anyways -- this gives us some food for tought 😄

@CertainLach
Copy link
Owner Author

but it has to be said that we're also removing a bunch of validation which we have natively available in our own build of go-jsonnet in this calculation

Jrsonnet has great (If I say so) api for embedding, and Rust makes it very easy to write bug-free code, so you might consider writing your checking utilities in Rust

Examples of jrsonnet embedding:
https://github.com/uniquenetwork/chainql
https://github.com/uniquenetwork/baedeker
https://github.com/CertainLach/jrsonnet/tree/master/crates/jrsonnet-stdlib/src (stdlib has no magic, you can write your own stdlib with no problems)

I also plan to open-source my own jrsonnet wrapper with useful utilities (Jsonschema validation, by-url imports with integrated jsonnet-bundler, etc) based on my kubernetes deployment tool (Which is also closed-source right now, only very old prototype is available right now: https://github.com/CertainLach/hayasaka), so if you have any wanted features - you may suggest them here, and maybe I'll add jrsonnetx binary/jrsonnet-stdlibx library with those features included.

@davidreuss
Copy link

davidreuss commented Mar 20, 2024

json schema validation, and kubernetes manifest validation (we currently embed kubeconform) are the two big things we need right now … so — that would be very interesting indeed 😄

integrated jsonnet-bundler sounds intriguing ad well, our current vendoring solution is also quite problematic in a multitude of ways .. 🤣

@CertainLach CertainLach merged commit c2313a2 into master Mar 21, 2024
6 checks passed
@CertainLach CertainLach deleted the fix/if-field-in-super-with-no-super branch March 21, 2024 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

no super found under certain conditions
3 participants