[fud2] Refactor plugin script loading (#2083)

* [fud2] Make plugins optional

Very minor fix: #2078 added plugins, controlled by a `plugins` key in
`fud2.toml`. However, it inadvertently made this option required, i.e.,
fud2 would crash if this option was not present. Now it's optional! So
we just silently proceed with no plugins if none are configured.

* Fix a doc typo

* Start refactoring plugin loading

* Start refactoring builder functions

* Refactor the rest of the functions

* Gather up a `register` function

* Use a fresh engine for each plugin??

Maybe this is wasteful, but it simplifies things a lot.

* Run one script at a time

* Refector script contexts

* Refactor runner struct

* Further beef up the runner

* Further simplify interface

* Fix a borrow that lasts too long

* Fix a method name

Clippy was thrown for a loop because of the `to_*` name.

* Move plugin loading to DriverBuilder?

Now the top-level thing is in the builder itself... no need for an extra
trait? Not sure this is a good idea.

fud2/fud-core/src/exec/driver.rs

@@ -1,5 +1,5 @@
 use super::{OpRef, Operation, Request, Setup, SetupRef, State, StateRef};
-use crate::{run, utils};
+use crate::{config, run, script, utils};
 use camino::{Utf8Path, Utf8PathBuf};
 use cranelift_entity::{PrimaryMap, SecondaryMap};
 use std::{collections::HashMap, error::Error, fmt::Display};
@@ -124,7 +124,7 @@ impl Driver {
         }
     }
 
-    /// Concot a plan to carry out the requested build.
+    /// Concoct a plan to carry out the requested build.
     ///
     /// This works by searching for a path through the available operations from the input state
     /// to the output state. If no such path exists in the operation graph, we return None.
@@ -358,6 +358,28 @@ impl DriverBuilder {
         self.rsrc_files = Some(files);
     }
 
+    /// Load any plugin scripts specified in the configuration file.
+    pub fn load_plugins(self) -> Self {
+        // Get a list of plugins (paths to Rhai scripts) from the config file, if any.
+        // TODO: Let's try to avoid loading/parsing the configuration file here and
+        // somehow reusing it from wherever we do that elsewhere.
+        let config = config::load_config(&self.name);
+        let plugin_files =
+            match config.extract_inner::<Vec<std::path::PathBuf>>("plugins") {
+                Ok(v) => v,
+                Err(_) => {
+                    // No plugins to load.
+                    return self;
+                }
+            };
+
+        let mut bld = self;
+        for path in plugin_files {
+            bld = script::ScriptRunner::run_file(bld, path.as_path());
+        }
+        bld
+    }
+
     pub fn build(self) -> Driver {
         Driver {
             name: self.name,

fud2/fud-core/src/lib.rs

@@ -6,4 +6,3 @@ pub mod script;
 pub mod utils;
 
 pub use exec::{Driver, DriverBuilder};
-pub use script::LoadPlugins;

fud2/fud-core/src/script/mod.rs

@@ -3,4 +3,4 @@ mod exec_scripts;
 mod plugin;
 mod report;
 
-pub use plugin::LoadPlugins;
+pub use plugin::ScriptRunner;

fud2/fud-core/src/script/plugin.rs

@@ -1,156 +1,191 @@
 use crate::{
-    config,
     exec::{OpRef, SetupRef, StateRef},
     DriverBuilder,
 };
-use std::{cell::RefCell, path::PathBuf, rc::Rc};
+use std::{
+    cell::RefCell,
+    path::{Path, PathBuf},
+    rc::Rc,
+};
 
 use super::{
     error::RhaiSystemError,
     exec_scripts::{to_rhai_err, to_str_slice, RhaiResult, RhaiSetupCtx},
     report::RhaiReport,
 };
 
-fn to_setup_refs(
-    ctx: &rhai::NativeCallContext,
-    setups: rhai::Array,
+struct ScriptContext {
+    builder: DriverBuilder,
     path: PathBuf,
     ast: rhai::AST,
-    this: Rc<RefCell<DriverBuilder>>,
-) -> RhaiResult<Vec<SetupRef>> {
-    setups
-        .into_iter()
-        .map(|s| match s.clone().try_cast::<rhai::FnPtr>() {
-            Some(fnptr) => Ok(this.borrow_mut().add_setup(
-                &format!("{} (plugin)", fnptr.fn_name()),
-                RhaiSetupCtx {
-                    path: path.clone(),
-                    ast: ast.clone(),
-                    name: fnptr.fn_name().to_string(),
-                },
-            )),
-            // if we can't cast as a FnPtr, try casting as a SetupRef directly
-            None => s.clone().try_cast::<SetupRef>().ok_or_else(move || {
-                RhaiSystemError::setup_ref(s)
-                    .with_pos(ctx.position())
-                    .into()
-            }),
-        })
-        .collect::<RhaiResult<Vec<_>>>()
 }
 
-pub trait LoadPlugins {
-    fn load_plugins(self) -> Self;
+impl ScriptContext {
+    /// Take a Rhai array value that is supposed to contain setups and produce
+    /// an array of actual references to setups. The array might contain string names
+    /// for the setups, or it might be function references that define those setups.
+    fn setups_array(
+        &mut self,
+        ctx: &rhai::NativeCallContext,
+        setups: rhai::Array,
+    ) -> RhaiResult<Vec<SetupRef>> {
+        setups
+            .into_iter()
+            .map(|s| match s.clone().try_cast::<rhai::FnPtr>() {
+                Some(fnptr) => {
+                    // TODO: Do we really need to clone stuff here, or can we continue to thread through
+                    // the `Rc`?
+                    let rctx = RhaiSetupCtx {
+                        path: self.path.clone(),
+                        ast: self.ast.clone(),
+                        name: fnptr.fn_name().to_string(),
+                    };
+                    Ok(self.builder.add_setup(
+                        &format!("{} (plugin)", fnptr.fn_name()),
+                        rctx,
+                    ))
+                }
+                // if we can't cast as a FnPtr, try casting as a SetupRef directly
+                None => {
+                    s.clone().try_cast::<SetupRef>().ok_or_else(move || {
+                        RhaiSystemError::setup_ref(s)
+                            .with_pos(ctx.position())
+                            .into()
+                    })
+                }
+            })
+            .collect::<RhaiResult<Vec<_>>>()
+    }
 }
 
-impl LoadPlugins for DriverBuilder {
-    fn load_plugins(self) -> Self {
-        // get list of plugins
-        let config = config::load_config(&self.name);
-        let plugin_files = match config.extract_inner::<Vec<PathBuf>>("plugins")
-        {
-            Ok(v) => v,
-            Err(_) => {
-                // No plugins to load.
-                return self;
-            }
-        };
+pub struct ScriptRunner {
+    ctx: Rc<RefCell<ScriptContext>>,
+    engine: rhai::Engine,
+}
 
-        // wrap driver in a ref cell, so that we can call it from a
-        // Rhai context
-        let this = Rc::new(RefCell::new(self));
+impl ScriptRunner {
+    fn from_file(builder: DriverBuilder, path: &Path) -> Self {
+        // Compile the script's source code.
+        let engine = rhai::Engine::new();
+        let ast = engine.compile_file(path.into()).unwrap();
 
-        // scope rhai engine code so that all references to `this`
-        // are dropped before the end of the function
-        {
-            let mut engine = rhai::Engine::new();
+        // TODO: Consider removing the clones here. We can probably just recover the stuff.
+        Self {
+            ctx: Rc::new(RefCell::new(ScriptContext {
+                builder,
+                path: path.into(),
+                ast: ast.clone(),
+            })),
+            engine,
+        }
+    }
+
+    /// Obtain the wrapped `DriverBuilder`. Panic if other references (from the
+    /// script, for example) still exist.
+    fn unwrap_builder(self) -> DriverBuilder {
+        std::mem::drop(self.engine); // Drop references to the context.
+        Rc::into_inner(self.ctx)
+            .expect("script references still live")
+            .into_inner()
+            .builder
+    }
 
-            // register AST independent functions
-            let t = this.clone();
-            engine.register_fn(
-                "state",
-                move |name: &str, extensions: rhai::Array| {
-                    let v = to_str_slice(&extensions);
-                    let v = v.iter().map(|x| &**x).collect::<Vec<_>>();
-                    t.borrow_mut().state(name, &v)
-                },
-            );
+    fn reg_state(&mut self) {
+        let sctx = self.ctx.clone();
+        self.engine.register_fn(
+            "state",
+            move |name: &str, extensions: rhai::Array| {
+                let v = to_str_slice(&extensions);
+                let v = v.iter().map(|x| &**x).collect::<Vec<_>>();
+                sctx.borrow_mut().builder.state(name, &v)
+            },
+        );
+    }
 
-            let t = Rc::clone(&this);
-            engine.register_fn("get_state", move |state_name: &str| {
-                t.borrow().find_state(state_name).map_err(to_rhai_err)
+    fn reg_get_state(&mut self) {
+        let sctx = self.ctx.clone();
+        self.engine
+            .register_fn("get_state", move |state_name: &str| {
+                sctx.borrow()
+                    .builder
+                    .find_state(state_name)
+                    .map_err(to_rhai_err)
             });
+    }
 
-            let t = Rc::clone(&this);
-            engine.register_fn("get_setup", move |setup_name: &str| {
-                t.borrow().find_setup(setup_name).map_err(to_rhai_err)
+    fn reg_get_setup(&mut self) {
+        let sctx = self.ctx.clone();
+        self.engine
+            .register_fn("get_setup", move |setup_name: &str| {
+                sctx.borrow()
+                    .builder
+                    .find_setup(setup_name)
+                    .map_err(to_rhai_err)
             });
+    }
 
-            // go through each plugin file, and execute the script which adds a plugin
-            // we need to define the following two functions in the loop because they
-            // need the ast of the current file
-            for path in plugin_files {
-                // compile the file into an Ast
-                let ast = engine.compile_file(path.clone()).unwrap();
+    fn reg_rule(&mut self) {
+        let sctx = self.ctx.clone();
+        self.engine.register_fn::<_, 4, true, OpRef, true, _>(
+            "rule",
+            move |ctx: rhai::NativeCallContext,
+                  setups: rhai::Array,
+                  input: StateRef,
+                  output: StateRef,
+                  rule_name: &str| {
+                let mut sctx = sctx.borrow_mut();
+                let setups = sctx.setups_array(&ctx, setups)?;
+                Ok(sctx.builder.rule(&setups, input, output, rule_name))
+            },
+        );
+    }
 
-                let t = Rc::clone(&this);
-                let rule_ast = ast.clone_functions_only();
-                let path_copy = path.clone();
-                engine.register_fn::<_, 4, true, OpRef, true, _>(
-                    "rule",
-                    move |ctx: rhai::NativeCallContext,
-                          setups: rhai::Array,
-                          input: StateRef,
-                          output: StateRef,
-                          rule_name: &str| {
-                        let setups = to_setup_refs(
-                            &ctx,
-                            setups,
-                            path_copy.clone(),
-                            rule_ast.clone(),
-                            Rc::clone(&t),
-                        )?;
-                        Ok(t.borrow_mut()
-                            .rule(&setups, input, output, rule_name))
-                    },
-                );
+    fn reg_op(&mut self) {
+        let sctx = self.ctx.clone();
+        self.engine.register_fn::<_, 5, true, OpRef, true, _>(
+            "op",
+            move |ctx: rhai::NativeCallContext,
+                  name: &str,
+                  setups: rhai::Array,
+                  input: StateRef,
+                  output: StateRef,
+                  build: rhai::FnPtr| {
+                let mut sctx = sctx.borrow_mut();
+                let setups = sctx.setups_array(&ctx, setups)?;
+                let rctx = RhaiSetupCtx {
+                    path: sctx.path.clone(),
+                    ast: sctx.ast.clone(),
+                    name: build.fn_name().to_string(),
+                };
+                Ok(sctx.builder.add_op(name, &setups, input, output, rctx))
+            },
+        );
+    }
 
-                let t = Rc::clone(&this);
-                let rule_ast = ast.clone_functions_only();
-                let path_copy = path.clone();
-                engine.register_fn::<_, 5, true, OpRef, true, _>(
-                    "op",
-                    move |ctx: rhai::NativeCallContext,
-                          name: &str,
-                          setups: rhai::Array,
-                          input: StateRef,
-                          output: StateRef,
-                          build: rhai::FnPtr| {
-                        let setups = to_setup_refs(
-                            &ctx,
-                            setups,
-                            path_copy.clone(),
-                            rule_ast.clone(),
-                            Rc::clone(&t),
-                        )?;
-                        Ok(t.borrow_mut().add_op(
-                            name,
-                            &setups,
-                            input,
-                            output,
-                            RhaiSetupCtx {
-                                path: path_copy.clone(),
-                                ast: rule_ast.clone(),
-                                name: build.fn_name().to_string(),
-                            },
-                        ))
-                    },
-                );
+    /// Register all the builder functions in the engine.
+    fn register(&mut self) {
+        self.reg_state();
+        self.reg_get_state();
+        self.reg_get_setup();
+        self.reg_rule();
+        self.reg_op();
+    }
 
-                engine.run_ast(&ast).report(&path);
-            }
-        }
+    /// Run the script.
+    fn run(&self) {
+        // TODO this whole dance feels unnecessary...
+        let (ast, path) = {
+            let sctx = self.ctx.borrow();
+            (sctx.ast.clone(), sctx.path.clone())
+        };
+        self.engine.run_ast(&ast).report(path);
+    }
 
-        Rc::into_inner(this).expect("Back into inner").into_inner()
+    /// Execute a script from a file, adding to the builder.
+    pub fn run_file(builder: DriverBuilder, path: &Path) -> DriverBuilder {
+        let mut runner = ScriptRunner::from_file(builder, path);
+        runner.register();
+        runner.run();
+        runner.unwrap_builder()
     }
 }

fud2/src/main.rs

@@ -1,5 +1,5 @@
 use fud2::build_driver;
-use fud_core::{cli, DriverBuilder, LoadPlugins};
+use fud_core::{cli, DriverBuilder};
 
 fn main() -> anyhow::Result<()> {
     let mut bld = DriverBuilder::new("fud2");