bruno-delfino1995 / kct

Kubernetes Configuration Tool
MIT License
18 stars 2 forks source link

jrsonnet-macros #19

Closed CertainLach closed 1 year ago

CertainLach commented 2 years ago

I see you use jrsonnet natives, and parse some jrsonnet values to serde_yaml

I recommend you to look at in-development version of jrsonnet (gcmodule branch), which adds a lot of quality of life enchantments to api, to make it easier to extend language with custom functions

For example, it provides ability to define builtins with automatic conversions and prettier syntax (with acceptable overhead, builtins are switched to this api, and benchmarks wasn't affected: https://github.com/CertainLach/jrsonnet/blob/gcmodule/crates/jrsonnet-evaluator/src/builtin/mod.rs), deriving conversions for custom objects (https://github.com/CertainLach/jrsonnet/blob/gcmodule/crates/jrsonnet-evaluator/src/lib.rs#L1290), and overall api overhaul

It isn't released yet, because of usage of forked GC library (rust-gc was replaced with gcmodule, as it has lower performance impact in standard operation)

bruno-delfino1995 commented 1 year ago

I took the 0.5.0pre6 for a spin and found it much more challenging to create dynamic built-ins. The ArgsLike structure gave some headaches because of its ThunkValue. The library seems much more complex because of the GC and optimizations now. I had trouble with the Trace trait before but created empty impls and shrugged off the possible memory leaks because I don't know how to implement it for Box<T>.

I wish I could use the [builtin] macro; it's way simpler than what I was doing. However, as some of my natives rely on dynamic data from the packages (Context in kct_compiler), I had to somehow try and implement BuiltIn by myself. I wonder if I can implement the jsonnet part by just looking at your implementation anymore because there're too many concepts I can't grasp with my current knowledge. Is there any resource I should read to understand your implementation better?

If you're open to it, I would appreciate your contribution. I wrapped all of the code related to jsonnet in the kct_jsonnet crate and abstracted the implementation details with the Property enum and Callback trait, but I'm unsure if I can keep that structure anymore. Also, I'm lacking ideas on how to change the dynamic natives to static ones, I think that would simplify things, but I also want to keep those at the kct_compiler crate, and maybe this might turn out too complex. Do you have any tips on designing my code around your crate?

CertainLach commented 1 year ago

I'll look at your code later; I can't understand what's going on in the kct_jsonnet crate at first glance (At least, I don't see where you plan to use static builtins). I can only tell you should make Trace a supertrait of Callback: trait Callback: Trace, and replace Box<dyn Callback> with TraceBox<dyn Callback>, as TraceBox has a proper implementation for trait objects.

GC is PITA, but ThunkValue is just a manually created closure alternative with GC support. I have this macro in another project for quickly creating Thunk instances with less boilerplate: https://github.com/CertainLach/chainql/blob/master/src/main.rs#L55-L84

For dynamic builtin, there is still a NativeCallback struct present (Ignore deprecation warning, I'll remove it, as both python and C bindings use this): https://github.com/CertainLach/jrsonnet/blob/master/crates/jrsonnet-evaluator/src/function/builtin.rs#L47

bruno-delfino1995 commented 1 year ago

Thank you so much for the tips! It was only because of them that I somehow managed to upgrade!

I considered using static builtins to reduce possible overheads from the dynamic builtins. At first, I thought about using the Context argument, but I realized they're more about the code than "runtime" information. In the end, I kept the dynamic natives and went with the BuiltIn trait instead of the NativeCallback trait after I saw your helper for dealing with ArgsLike. I also tried turning the Callback trait into an Fn(...) to remove Trace from the other crates, but I had no success because I couldn't call the property due to some typing issues I didn't understand.

Nevertheless, I'm somewhat pleased with the results, but I didn't go in the direction of the macros you shared because the functions are more dynamic than before. So please feel free to submit any PR or suggest different approaches that could result in a whole rewrite. They'll be much appreciated!