Added the imports builtin #115

arduano · 2024-03-30T01:06:27Z

Added builtins.import. In order to achieve this, I needed to refactor some other parts of the code.

Changes include:

Removed the RIX_NIXRT_JS_MODULE environment variable. Instead, the file is directly included via include_str!.
The runtime file is also directly run (without an import statement), and its output namespace is set to the global variable n.
Modified the exports of the runtime file to avoid duplicates. Instead of exporting each item individually AND exporting a default object, it just exports the individual items.
Each transpiled module is just a single function with the nix expression, set to the default export. That value is extracted from the namespace in rust.
The importNixModule intrinsic is injected into the runtime, and it basically just opens a nix file, transpiles it, returns the resulting expression function.

I used a lot of .unwrap, in the future we need to do proper error handling (like a full robust error handling solution with text styling within the errors). I just assumed that it's best to avoid overthinking that now.

flake.nix

urbas · 2024-04-06T08:04:41Z

nixjs-rt/src/builtins/builtins.ts

@@ -22,6 +24,25 @@ export function getBuiltins() {
 );
 },

+ import: (path) => {
+ const pathStrict = path.toStrict();


Not related to your PR, but just wondering if we can find another mechanism for lazy evaluation. Having to call this toStrict() method is a bit cumbersome. Any ideas are welcome.

Isn't this only invoked when an import is already being evaluated? Like an import invocation is only evaluated when the value of it is required, and when that value is required then the path also needs to be evaluated

Oh wait I see what you mean

I'll think about it, will let you know if I have ideas. Quickly looking over this right now, it feels like toStrict is the best option so far as we do need to explicitly differentiate lazy vs required, and I'm not sure what other options there are.

urbas · 2024-04-06T08:13:55Z

src/eval/execution.rs


+ let import_module_attr = v8::String::new(scope, "importNixModule").unwrap();
+ let import_module_fn = v8::Function::new(scope, import_nix_module).unwrap();
+ global


Could you extract this bit of logic (the one that adds an object into the global scope) into a function, something like this:

fn add_to_js_scope(scope: ?, name: &str, js_object: ?) -> Result<(), String> { ... }

Also, could you return a Result with a descriptive error string (instead of using unwrap())?

My next PR was going to be a full error handling refactor, is it ok if we skip the unwraps for this one? I was planning to make a proper error type that can be passed between js and rust, possibly with a full nix stack trace too

urbas · 2024-04-06T08:20:30Z

src/eval/execution.rs

- let source_v8 = to_v8_source(scope, &source_str, "<eval string>");
- let module = v8::script_compiler::compile_module(scope, source_v8).unwrap();
+ let module_source_v8 = to_v8_source(scope, &source_str, "<eval string>");
+ let module = v8::script_compiler::compile_module(scope, module_source_v8).unwrap();


For another day: It would be great to use some kind of caching here (to avoid re-emitting JS or even recompiling JS—with v8's code-caching ability).

urbas · 2024-04-06T08:26:20Z

src/eval/emit_js.rs

- out_src += "export const __nixrt = n;\n";
- out_src += "export const __nixValue = (ctx) => ";
+ let mut out_src = String::new();
+ out_src += "export default (ctx) => ";


Very nice simplification!

urbas · 2024-04-06T08:26:41Z

src/eval/execution.rs


+ let resolve_module_callback = |_, _, _, _| panic!("Module resolution not supported.");


Does this mean you no longer use the resolve_module_callback function at the bottom of the file? Can it be removed?

Just fixed that, yeah, kept the function because there was a second place it was used (moved the panic there)

urbas · 2024-04-06T08:33:00Z

src/eval/execution.rs

- nix_value_from_module(scope, &namespace_obj)
+
+ let nix_value_attr = v8::String::new(scope, "default").unwrap();
+ let Some(nix_value) = namespace_obj.get(scope, nix_value_attr.into()) else {


Can you try using the try_get_js_object_key here instead?

urbas · 2024-04-06T08:38:16Z

src/eval/execution.rs

+ nix_value_from_module(scope, root_nix_fn, nixjs_rt_obj)
+}
+
+fn eval_nix_fn_from_string<'s>(


Nitpick: maybe a slightly different name for the function:

Suggested change

fn eval_nix_fn_from_string<'s>(

fn nix_expr_to_js_function<'s>(

urbas · 2024-04-06T08:40:34Z

src/eval/execution.rs

+ };
+
+ ret.set(nix_fn.into());
+}


Very nice and concise! Great work! 🎉

urbas · 2024-04-06T08:42:50Z

src/eval/execution.rs

+
+ let obj = module.get_module_namespace().to_object(scope).unwrap();
+
+ Ok(obj)


Inline obj?

Suggested change

Ok(obj)

Ok(module.get_module_namespace().to_object(scope).unwrap())

Personally I often lean towards breaking up lines into variables to be more descriptive, e.g. if I add error handling in the future, I'd need to replace the .unwrap with something else and the line will grow further. Especially when it comes to layers of wrapping, with things like Some and Ok

urbas · 2024-04-06T08:46:35Z

src/eval/execution.rs

+
+ let root_nix_fn = eval_nix_fn_from_string(scope, nix_expr)?;
+
+ nix_value_from_module(scope, root_nix_fn, nixjs_rt_obj)


Maybe avoid passing around nixjs_rt_obj (since it's already in root_nix_fn's global scope)?

Also, can you maybe avoid passing around the scope and get the scope from root_nix_fn somehow?

imo explicitly passing around nixjs_rt_obj is better than manually extracting it from the global scope each time, we need to access it in rust anyway because the js_value_to_nix function needs it. E.g. in the future if we move it out of the scope, or pass the runtime object another way, explicitly passing the rt object as an argument here will keep working.

As for scope, not sure, don't think there's a way.

urbas · 2024-04-06T08:57:13Z

src/eval/execution.rs

- .try_into()
- .expect("`n.toStrict` is not a function.");
+ let to_strict_fn: v8::Local<v8::Function> =
+ try_get_js_object_key(scope, &nixrt, "recursiveStrict")?


I wonder if recursiveStrict is the right thing to call here. Actually, we should probably not call toStrict either.

We should only force strict evaluation if the user really wants to see the entire value. For example, if the user requested only a specific attrpath like foo.bar.moo, then only those values should be strictly evaluated.

I would suggest we move the strict evaluation out of this function, much higher up. What do you think? Should we move this "to strict" bit of logic to the evaluate function?

The nix_value_from_module function is only called for the final value that's about to be sent over to Rust anyway, for the root module (only in the evaluate function), not per-import. Imports are still lazy (I should make a test for that sometime).

I don't think this should be the responsibility of this code specifically. In the future, if we allow the evaluator to only evaluate a specific attrpath, then that should be transpiled into js imo rather than being manually done in rust.

urbas · 2024-04-06T08:58:22Z

src/eval/tests.rs

+ #[test]
+ fn test_eval_builtin_() {
+ assert_eq!(
+ eval_ok("(builtins.import ./flake.nix).description"),


Haha! Nice test and it's great to see it actually working! 🎉

arduano added 2 commits March 30, 2024 11:53

Added the imports builtin

a0996e6

Fixed clippy warnings

382b84a

arduano requested a review from urbas March 30, 2024 01:10

arduano added 2 commits March 30, 2024 12:13

Use default export

71e272e

fix

d4bf4f3