Add support for plugins to dynamically linked modules (#821)

crates/cli/src/codegen/builder.rs

@@ -84,11 +84,6 @@ impl CodeGenBuilder {
 
     /// Build a [`CodeGenerator`].
     pub fn build(self, ty: CodeGenType, js_runtime_config: JsConfig) -> Result<Generator> {
-        if let CodeGenType::Dynamic = ty {
-            if js_runtime_config.has_configs() {
-                bail!("Cannot set JS runtime options when building a dynamic module")
-            }
-        }
         let mut generator = Generator::new(ty, js_runtime_config, self.plugin);
         generator.source_compression = self.source_compression;
         generator.wit_opts = self.wit_opts;

crates/cli/src/codegen/mod.rs

@@ -203,9 +203,21 @@ impl Generator {
                     canonical_abi_realloc_type,
                 );
 
-                let eval_bytecode_type = module.types.add(&[ValType::I32, ValType::I32], &[]);
-                let (eval_bytecode_fn_id, _) =
-                    module.add_import_func(&import_namespace, "eval_bytecode", eval_bytecode_type);
+                // User plugins can use `invoke` with a null function name.
+                // User plugins also won't have an `eval_bytecode` function to
+                // import. We want to remove `eval_bytecode` from the default
+                // plugin so we don't want to emit more uses of it.
+                let eval_bytecode_fn_id = if self.plugin.is_v2_plugin() {
+                    let eval_bytecode_type = module.types.add(&[ValType::I32, ValType::I32], &[]);
+                    let (eval_bytecode_fn_id, _) = module.add_import_func(
+                        &import_namespace,
+                        "eval_bytecode",
+                        eval_bytecode_type,
+                    );
+                    Some(eval_bytecode_fn_id)
+                } else {
+                    None
+                };
 
                 let invoke_type = module.types.add(
                     &[ValType::I32, ValType::I32, ValType::I32, ValType::I32],
@@ -226,7 +238,7 @@ impl Generator {
 
                 Ok(Identifiers::new(
                     canonical_abi_realloc_fn_id,
-                    Some(eval_bytecode_fn_id),
+                    eval_bytecode_fn_id,
                     invoke_fn_id,
                     memory_id,
                 ))
@@ -269,18 +281,12 @@ impl Generator {
                 .call(eval_bytecode);
         } else {
             // Assert we're not emitting a call with a null function to
-            // invoke for `javy_quickjs_provider_v*`.
-            // `javy_quickjs_provider_v2` will never support calling `invoke`
-            // with a null function. Older `javy_quickjs_provider_v3`'s do not
-            // support being called with a null function. User plugins and
-            // newer `javy_quickjs_provider_v3`s do support being called with a
-            // null function.
-            // Using `assert!` instead of `debug_assert!` because integration
-            // tests are executed with Javy built with the release profile so
-            // `debug_assert!`s are stripped out.
+            // invoke for the v2 plugin. `javy_quickjs_provider_v2` will never
+            // support calling `invoke` with a null function. The default
+            // plugin and user plugins do accept null functions.
             assert!(
-                self.plugin.is_user_plugin(),
-                "Using invoke with null function only supported for user plugins"
+                !self.plugin.is_v2_plugin(),
+                "Using invoke with null function not supported for v2 plugin"
             );
             instructions
                 .local_get(bytecode_ptr_local) // ptr to bytecode
@@ -500,7 +506,6 @@ mod test {
             JsConfig::default(),
             Plugin::Default,
         );
-        assert!(!gen.js_runtime_config.has_configs());
         assert!(gen.source_compression);
         assert!(matches!(gen.plugin, Plugin::Default));
         assert_eq!(gen.wit_opts, WitOptions::default());

crates/cli/src/commands.rs

@@ -234,11 +234,18 @@ impl TryFrom<Vec<GroupOption<CodegenOption>>> for CodegenOptionGroup {
 
         options.wit = WitOptions::from_tuple((wit.cloned(), wit_world.cloned()))?;
 
-        // This is temporary. So I can make the change for dynamic modules
-        // separately because it will be a breaking change.
-        if options.dynamic && options.plugin.is_some() {
-            bail!("Cannot use plugins for building dynamic modules");
+        // We never want to assume the import namespace to use for a
+        // dynamically linked module. If we do assume the import namespace, any
+        // change to that assumed import namespace can result in new
+        // dynamically linked modules not working on existing execution
+        // environments because there will be unmet import errors when trying
+        // to instantiate those modules. Since we can't assume the import
+        // namespace, we must require a plugin so we can derive the import
+        // namespace from the plugin.
+        if options.dynamic && options.plugin.is_none() {
+            bail!("Must specify plugin when using dynamic linking");
         }
+
         Ok(options)
     }
 }
@@ -353,19 +360,20 @@ impl JsConfig {
 
 #[cfg(test)]
 mod tests {
+    use std::path::PathBuf;
+
     use crate::{
         commands::{JsGroupOption, JsGroupValue},
         js_config::JsConfig,
         plugins::Plugin,
     };
 
     use super::{CodegenOption, CodegenOptionGroup, GroupOption};
-    use anyhow::Result;
+    use anyhow::{Error, Result};
 
     #[test]
     fn js_config_from_config_values() -> Result<()> {
         let group = JsConfig::from_group_values(&Plugin::Default, vec![])?;
-        assert!(!group.has_configs());
         assert_eq!(group.get("redirect-stdout-to-stderr"), None);
         assert_eq!(group.get("javy-json"), None);
         assert_eq!(group.get("javy-stream-io"), None);
@@ -501,10 +509,14 @@ mod tests {
         let group: CodegenOptionGroup = vec![].try_into()?;
         assert_eq!(group, CodegenOptionGroup::default());
 
-        let raw = vec![GroupOption(vec![CodegenOption::Dynamic(true)])];
+        let raw = vec![GroupOption(vec![
+            CodegenOption::Dynamic(true),
+            CodegenOption::Plugin(PathBuf::from("file.wasm")),
+        ])];
         let group: CodegenOptionGroup = raw.try_into()?;
         let expected = CodegenOptionGroup {
             dynamic: true,
+            plugin: Some(PathBuf::from("file.wasm")),
             ..Default::default()
         };
 
@@ -519,6 +531,13 @@ mod tests {
 
         assert_eq!(group, expected);
 
+        let raw = vec![GroupOption(vec![CodegenOption::Dynamic(true)])];
+        let result: Result<CodegenOptionGroup, Error> = raw.try_into();
+        assert_eq!(
+            result.err().unwrap().to_string(),
+            "Must specify plugin when using dynamic linking"
+        );
+
         Ok(())
     }
 }

crates/cli/src/js_config.rs

@@ -11,11 +11,6 @@ impl JsConfig {
         JsConfig(configs)
     }
 
-    /// Returns true if any configs are set.
-    pub(crate) fn has_configs(&self) -> bool {
-        !self.0.is_empty()
-    }
-
     /// Encode as JSON.
     pub(crate) fn to_json(&self) -> Result<Vec<u8>> {
         Ok(serde_json::to_vec(&self.0)?)

crates/cli/src/plugins.rs

@@ -57,6 +57,11 @@ impl Plugin {
         matches!(&self, Plugin::User { .. })
     }
 
+    /// Returns true if the plugin is the legacy v2 plugin.
+    pub fn is_v2_plugin(&self) -> bool {
+        matches!(&self, Plugin::V2)
+    }
+
     /// Returns the plugin Wasm module as a byte slice.
     pub fn as_bytes(&self) -> &[u8] {
         match self {

crates/cli/tests/dynamic_linking_test.rs

@@ -56,11 +56,26 @@ fn test_errors_in_exported_functions_are_correctly_reported(builder: &mut Builde
     Ok(())
 }
 
-#[javy_cli_test(dyn = true, root = "tests/dynamic-linking-scripts")]
+#[javy_cli_test(
+    dyn = true,
+    root = "tests/dynamic-linking-scripts",
+    commands(not(Compile))
+)]
 // If you need to change this test, then you've likely made a breaking change.
 pub fn check_for_new_imports(builder: &mut Builder) -> Result<()> {
     let runner = builder.input("console.js").build()?;
-    runner.ensure_expected_imports()
+    runner.ensure_expected_imports(false)
+}
+
+#[javy_cli_test(
+    dyn = true,
+    root = "tests/dynamic-linking-scripts",
+    commands(not(Build))
+)]
+// If you need to change this test, then you've likely made a breaking change.
+pub fn check_for_new_imports_for_compile(builder: &mut Builder) -> Result<()> {
+    let runner = builder.input("console.js").build()?;
+    runner.ensure_expected_imports(true)
 }
 
 #[javy_cli_test(dyn = true, root = "tests/dynamic-linking-scripts")]
@@ -93,9 +108,9 @@ fn test_using_runtime_flag_with_dynamic_triggers_error(builder: &mut Builder) ->
         .input("console.js")
         .redirect_stdout_to_stderr(false)
         .build();
-    assert!(build_result.is_err_and(|e| e
-        .to_string()
-        .contains("Error: Cannot set JS runtime options when building a dynamic module")));
+    assert!(build_result.is_err_and(|e| e.to_string().contains(
+        "error: Property redirect-stdout-to-stderr is not supported for runtime configuration"
+    )));
     Ok(())
 }
 
@@ -119,12 +134,16 @@ fn javy_json_identity(builder: &mut Builder) -> Result<()> {
 }
 
 #[javy_cli_test(dyn = true, commands(not(Compile)))]
-fn test_using_plugin_with_dynamic_build_fails(builder: &mut Builder) -> Result<()> {
-    let result = builder.plugin(Plugin::User).input("plugin.js").build();
-    let err = result.err().unwrap();
-    assert!(err
-        .to_string()
-        .contains("Cannot use plugins for building dynamic modules"));
+fn test_using_plugin_with_dynamic_works(builder: &mut Builder) -> Result<()> {
+    let plugin = Plugin::User;
+    let mut runner = builder
+        .plugin(Plugin::User)
+        .preload(plugin.namespace().into(), plugin.path())
+        .input("plugin.js")
+        .build()?;
+
+    let result = runner.exec(&[]);
+    assert!(result.is_ok());
 
     Ok(())
 }

crates/runner/src/lib.rs

@@ -23,6 +23,8 @@ pub enum Plugin {
     V2,
     Default,
     User,
+    /// Pass the default plugin on the CLI as a user plugin.
+    DefaultAsUser,
 }
 
 impl Plugin {
@@ -31,7 +33,7 @@ impl Plugin {
             Self::V2 => "javy_quickjs_provider_v2",
             // Could try and derive this but not going to for now since tests
             // will break if it changes.
-            Self::Default => "javy_quickjs_provider_v3",
+            Self::Default | Self::DefaultAsUser => "javy_quickjs_provider_v3",
             Self::User { .. } => "test_plugin",
         }
     }
@@ -47,7 +49,7 @@ impl Plugin {
                 .join("src")
                 .join("javy_quickjs_provider_v2.wasm"),
             Self::User => root.join("test_plugin.wasm"),
-            Self::Default => root
+            Self::Default | Self::DefaultAsUser => root
                 .join("..")
                 .join("..")
                 .join("target")
@@ -428,16 +430,17 @@ impl Runner {
         })
     }
 
-    pub fn ensure_expected_imports(&self) -> Result<()> {
+    pub fn ensure_expected_imports(&self, expect_eval_bytecode: bool) -> Result<()> {
         let module = Module::from_binary(self.linker.engine(), &self.wasm)?;
         let instance_name = self.plugin.namespace();
 
         let imports = module
             .imports()
             .filter(|i| i.module() == instance_name)
             .collect::<Vec<_>>();
-        if imports.len() != 4 {
-            bail!("Dynamically linked modules should have exactly 4 imports for {instance_name}");
+        let expected_import_count = if expect_eval_bytecode { 4 } else { 3 };
+        if imports.len() != expected_import_count {
+            bail!("Dynamically linked modules should have exactly {expected_import_count} imports for {instance_name}");
         }
 
         let realloc = imports
@@ -458,17 +461,19 @@ impl Runner {
             .find(|i| i.name() == "memory" && i.ty().memory().is_some())
             .ok_or_else(|| anyhow!("Should have memory import named memory"))?;
 
-        let eval_bytecode = imports
-            .iter()
-            .find(|i| i.name() == "eval_bytecode")
-            .ok_or_else(|| anyhow!("Should have eval_bytecode import"))?;
-        let ty = eval_bytecode.ty();
-        let f = ty.unwrap_func();
-        if !f.params().all(|p| p.is_i32()) || f.params().len() != 2 {
-            bail!("eval_bytecode should accept 2 i32s as parameters");
-        }
-        if f.results().len() != 0 {
-            bail!("eval_bytecode should return no results");
+        if expect_eval_bytecode {
+            let eval_bytecode = imports
+                .iter()
+                .find(|i| i.name() == "eval_bytecode")
+                .ok_or_else(|| anyhow!("Should have eval_bytecode import"))?;
+            let ty = eval_bytecode.ty();
+            let f = ty.unwrap_func();
+            if !f.params().all(|p| p.is_i32()) || f.params().len() != 2 {
+                bail!("eval_bytecode should accept 2 i32s as parameters");
+            }
+            if f.results().len() != 0 {
+                bail!("eval_bytecode should return no results");
+            }
         }
 
         let invoke = imports
@@ -604,7 +609,7 @@ impl Runner {
             args.push(format!("text-encoding={}", if enabled { "y" } else { "n" }));
         }
 
-        if let Plugin::User = plugin {
+        if matches!(plugin, Plugin::User | Plugin::DefaultAsUser) {
             args.push("-C".to_string());
             args.push(format!("plugin={}", plugin.path().to_str().unwrap()));
         }

crates/test-macros/src/lib.rs

@@ -299,7 +299,7 @@ fn expand_cli_tests(test_config: &CliTestConfig, func: syn::ItemFn) -> Result<To
                 }
             } else {
                 quote! {
-                    let plugin = javy_runner::Plugin::Default;
+                    let plugin = javy_runner::Plugin::DefaultAsUser;
                     builder.preload(plugin.namespace().into(), plugin.path());
                     builder.plugin(plugin);
                 }

docs/docs-using-dynamic-linking.md

@@ -33,8 +33,8 @@ Run:
 
 ```
 $ echo 'console.log("hello world!");' > my_code.js
-$ javy build -C dynamic -o my_code.wasm my_code.js
 $ javy emit-plugin -o plugin.wasm
+$ javy build -C dynamic -C plugin=plugin.wasm -o my_code.wasm my_code.js
 $ wasmtime run --preload javy_quickjs_provider_v3=plugin.wasm my_code.wasm
 hello world!
 ```

docs/docs-using-nodejs.md

@@ -11,15 +11,15 @@ This example shows how to use a dynamically linked Javy compiled Wasm module. We
 
 ### Steps
 
-1. The first step is to compile the `embedded.js` with Javy using dynamic linking:
+1. Emit the Javy plugin
 ```shell
-javy build -C dynamic -o embedded.wasm embedded.js
+javy emit-plugin -o plugin.wasm
 ```
-2. Next emit the Javy plugin
+2. Compile the `embedded.js` with Javy using dynamic linking:
 ```shell
-javy emit-plugin -o plugin.wasm
+javy build -C dynamic -C plugin=plugin.wasm -o embedded.wasm embedded.js
 ```
-3. Then we can run `host.mjs`
+3. Run `host.mjs`
 ```shell
 node --no-warnings=ExperimentalWarning host.mjs
 ```