[Rust] Add builder API for creating workflows

`Workflow::new` is replaced by `Workflow::build` that returns a
`Builder` type that supports the operations that mutate a workflow, such
as registering activities.

`Workflow::instance` is replaced by `Workflow::get` to make clear that
it is intended to look up an existing workflow.

`Workflow::cloned` is introduced to wrap the common pattern of
retrieving an existing workflow and cloning it with the same name in
order to modify it.

`Builder::activity_before` / `Builder::activity_after` are introduced to
wrap the common pattern of registering an activity then inserting it
before or after a given activity.

3 files changed, 240 insertions, 193 deletions

diff --git a/rust/examples/workflow.rs b/rust/examples/workflow.rs
index 49560579..db0bf599 100644
--- a/rust/examples/workflow.rs
+++ b/rust/examples/workflow.rs
@@ -52,7 +52,7 @@ pub fn main() {
         binaryninja::headless::Session::new().expect("Failed to initialize session");
 
     println!("Registering workflow...");
-    let old_meta_workflow = Workflow::instance("core.function.metaAnalysis");
+    let old_meta_workflow = Workflow::get("core.function.metaAnalysis");
     let meta_workflow = old_meta_workflow.clone_to("core.function.metaAnalysis");
     let activity = Activity::new_with_action(RUST_ACTIVITY_CONFIG, example_activity);
     meta_workflow.register_activity(&activity).unwrap();
diff --git a/rust/src/workflow.rs b/rust/src/workflow.rs
index d937987a..2c03a482 100644
--- a/rust/src/workflow.rs
+++ b/rust/src/workflow.rs
@@ -148,7 +148,6 @@ unsafe impl RefCountable for AnalysisContext {
     }
 }
 
-// TODO: We need to hide the JSON here behind a sensible/typed API.
 #[repr(transparent)]
 pub struct Workflow {
     handle: NonNull<BNWorkflow>,
@@ -164,19 +163,22 @@ impl Workflow {
     }
 
     /// Create a new unregistered [Workflow] with no activities.
+    /// Returns a [WorkflowBuilder] that can be used to configure and register the new [Workflow].
     ///
     /// To get a copy of an existing registered [Workflow] use [Workflow::clone_to].
-    pub fn new(name: &str) -> Ref<Self> {
+    pub fn build(name: &str) -> WorkflowBuilder {
         let name = name.to_cstr();
         let result = unsafe { BNCreateWorkflow(name.as_ptr()) };
-        unsafe { Workflow::ref_from_raw(NonNull::new(result).unwrap()) }
+        WorkflowBuilder {
+            handle: unsafe { Workflow::ref_from_raw(NonNull::new(result).unwrap()) },
+        }
     }
 
     /// Make a new unregistered [Workflow], copying all activities and the execution strategy.
+    /// Returns a [WorkflowBuilder] that can be used to configure and register the new [Workflow].
     ///
     /// * `name` - the name for the new [Workflow]
-    #[must_use]
-    pub fn clone_to(&self, name: &str) -> Ref<Workflow> {
+    pub fn clone_to(&self, name: &str) -> WorkflowBuilder {
         self.clone_to_with_root(name, "")
     }
 
@@ -184,11 +186,10 @@ impl Workflow {
     ///
     /// * `name` - the name for the new [Workflow]
     /// * `root_activity` - perform the clone operation with this activity as the root
-    #[must_use]
-    pub fn clone_to_with_root(&self, name: &str, root_activity: &str) -> Ref<Workflow> {
+    pub fn clone_to_with_root(&self, name: &str, root_activity: &str) -> WorkflowBuilder {
         let raw_name = name.to_cstr();
         let activity = root_activity.to_cstr();
-        unsafe {
+        let workflow = unsafe {
             Self::ref_from_raw(
                 NonNull::new(BNWorkflowClone(
                     self.handle.as_ptr(),
@@ -197,13 +198,23 @@ impl Workflow {
                 ))
                 .unwrap(),
             )
-        }
+        };
+        WorkflowBuilder { handle: workflow }
     }
 
-    pub fn instance(name: &str) -> Ref<Workflow> {
+    /// Get an existing [Workflow] by name.
+    pub fn get(name: &str) -> Option<Ref<Workflow>> {
+        // TODO: BNWorkflowInstance has get-or-create semantics. There is currently no way to just get.
         let name = name.to_cstr();
         let result = unsafe { BNWorkflowInstance(name.as_ptr()) };
-        unsafe { Workflow::ref_from_raw(NonNull::new(result).unwrap()) }
+        let handle = NonNull::new(result)?;
+        Some(unsafe { Workflow::ref_from_raw(handle) })
+    }
+
+    /// Clone the existing [Workflow] named `name`.
+    /// Returns a [WorkflowBuilder] that can be used to configure and register the new [Workflow].
+    pub fn cloned(name: &str) -> Option<WorkflowBuilder> {
+        Self::get(name).map(|workflow| workflow.clone_to(name))
     }
 
     /// List of all registered [Workflow]'s
@@ -220,58 +231,6 @@ impl Workflow {
         unsafe { BnString::into_string(result) }
     }
 
-    /// Register this [Workflow], making it immutable and available for use.
-    pub fn register(&self) -> Result<(), ()> {
-        self.register_with_config("")
-    }
-
-    /// Register this [Workflow], making it immutable and available for use.
-    ///
-    /// * `configuration` - a JSON representation of the workflow configuration
-    pub fn register_with_config(&self, config: &str) -> Result<(), ()> {
-        let config = config.to_cstr();
-        if unsafe { BNRegisterWorkflow(self.handle.as_ptr(), config.as_ptr()) } {
-            Ok(())
-        } else {
-            Err(())
-        }
-    }
-
-    /// Register an [Activity] with this Workflow.
-    ///
-    /// * `activity` - the [Activity] to register
-    pub fn register_activity(&self, activity: &Activity) -> Result<Ref<Activity>, ()> {
-        self.register_activity_with_subactivities::<Vec<String>>(activity, vec![])
-    }
-
-    /// Register an [Activity] with this Workflow.
-    ///
-    /// * `activity` - the [Activity] to register
-    /// * `subactivities` - the list of Activities to assign
-    pub fn register_activity_with_subactivities<I>(
-        &self,
-        activity: &Activity,
-        subactivities: I,
-    ) -> Result<Ref<Activity>, ()>
-    where
-        I: IntoIterator,
-        I::Item: IntoCStr,
-    {
-        let subactivities_raw: Vec<_> = subactivities.into_iter().map(|x| x.to_cstr()).collect();
-        let mut subactivities_ptr: Vec<*const _> =
-            subactivities_raw.iter().map(|x| x.as_ptr()).collect();
-        let result = unsafe {
-            BNWorkflowRegisterActivity(
-                self.handle.as_ptr(),
-                activity.handle.as_ptr(),
-                subactivities_ptr.as_mut_ptr(),
-                subactivities_ptr.len(),
-            )
-        };
-        let activity_ptr = NonNull::new(result).ok_or(())?;
-        unsafe { Ok(Activity::ref_from_raw(activity_ptr)) }
-    }
-
     /// Determine if an Activity exists in this [Workflow].
     pub fn contains(&self, activity: &str) -> bool {
         let activity = activity.to_cstr();
@@ -343,11 +302,166 @@ impl Workflow {
         unsafe { Array::new(result as *mut *mut c_char, count, ()) }
     }
 
+    /// Generate a FlowGraph object for the current [Workflow] and optionally show it in the UI.
+    ///
+    /// * `activity` - if specified, generate the Flowgraph using `activity` as the root
+    /// * `sequential` - whether to generate a **Composite** or **Sequential** style graph
+    pub fn graph(&self, activity: &str, sequential: Option<bool>) -> Option<Ref<FlowGraph>> {
+        let sequential = sequential.unwrap_or(false);
+        let activity = activity.to_cstr();
+        let graph =
+            unsafe { BNWorkflowGetGraph(self.handle.as_ptr(), activity.as_ptr(), sequential) };
+        if graph.is_null() {
+            return None;
+        }
+        Some(unsafe { FlowGraph::ref_from_raw(graph) })
+    }
+
+    /// Not yet implemented.
+    pub fn show_metrics(&self) {
+        unsafe { BNWorkflowShowReport(self.handle.as_ptr(), c"metrics".as_ptr()) }
+    }
+
+    /// Show the Workflow topology in the UI.
+    pub fn show_topology(&self) {
+        unsafe { BNWorkflowShowReport(self.handle.as_ptr(), c"topology".as_ptr()) }
+    }
+
+    /// Not yet implemented.
+    pub fn show_trace(&self) {
+        unsafe { BNWorkflowShowReport(self.handle.as_ptr(), c"trace".as_ptr()) }
+    }
+}
+
+impl ToOwned for Workflow {
+    type Owned = Ref<Self>;
+
+    fn to_owned(&self) -> Self::Owned {
+        unsafe { RefCountable::inc_ref(self) }
+    }
+}
+
+unsafe impl RefCountable for Workflow {
+    unsafe fn inc_ref(handle: &Self) -> Ref<Self> {
+        Ref::new(Self {
+            handle: NonNull::new(BNNewWorkflowReference(handle.handle.as_ptr()))
+                .expect("valid handle"),
+        })
+    }
+
+    unsafe fn dec_ref(handle: &Self) {
+        BNFreeWorkflow(handle.handle.as_ptr());
+    }
+}
+
+impl CoreArrayProvider for Workflow {
+    type Raw = *mut BNWorkflow;
+    type Context = ();
+    type Wrapped<'a> = Guard<'a, Workflow>;
+}
+
+unsafe impl CoreArrayProviderInner for Workflow {
+    unsafe fn free(raw: *mut Self::Raw, count: usize, _context: &Self::Context) {
+        BNFreeWorkflowList(raw, count)
+    }
+
+    unsafe fn wrap_raw<'a>(raw: &'a Self::Raw, context: &'a Self::Context) -> Self::Wrapped<'a> {
+        Guard::new(
+            Workflow::from_raw(NonNull::new(*raw).expect("valid handle")),
+            context,
+        )
+    }
+}
+
+#[must_use = "Workflow is not registered until `register` is called"]
+pub struct WorkflowBuilder {
+    handle: Ref<Workflow>,
+}
+
+impl WorkflowBuilder {
+    fn raw_handle(&self) -> *mut BNWorkflow {
+        self.handle.handle.as_ptr()
+    }
+
+    /// Register an [Activity] with this Workflow and insert it before the designated position.
+    ///
+    /// * `activity` - the [Activity] to register
+    /// * `sibling` - the activity to insert the new activity before
+    pub fn activity_before(self, activity: &Activity, sibling: &str) -> Result<Self, ()> {
+        self.register_activity(activity)?
+            .insert(sibling, vec![activity.name()])
+    }
+
+    /// Register an [Activity] with this Workflow and insert it in the designated position.
+    ///
+    /// * `activity` - the [Activity] to register
+    /// * `sibling` - the activity to insert the new activity after
+    pub fn activity_after(self, activity: &Activity, sibling: &str) -> Result<Self, ()> {
+        self.register_activity(activity)?
+            .insert_after(sibling, vec![activity.name()])
+    }
+
+    /// Register an [Activity] with this Workflow.
+    ///
+    /// * `activity` - the [Activity] to register
+    pub fn register_activity(self, activity: &Activity) -> Result<Self, ()> {
+        self.register_activity_with_subactivities::<Vec<String>>(activity, vec![])
+    }
+
+    /// Register an [Activity] with this Workflow.
+    ///
+    /// * `activity` - the [Activity] to register
+    /// * `subactivities` - the list of Activities to assign
+    pub fn register_activity_with_subactivities<I>(
+        self,
+        activity: &Activity,
+        subactivities: I,
+    ) -> Result<Self, ()>
+    where
+        I: IntoIterator,
+        I::Item: IntoCStr,
+    {
+        let subactivities_raw: Vec<_> = subactivities.into_iter().map(|x| x.to_cstr()).collect();
+        let mut subactivities_ptr: Vec<*const _> =
+            subactivities_raw.iter().map(|x| x.as_ptr()).collect();
+        let result = unsafe {
+            BNWorkflowRegisterActivity(
+                self.raw_handle(),
+                activity.handle.as_ptr(),
+                subactivities_ptr.as_mut_ptr(),
+                subactivities_ptr.len(),
+            )
+        };
+        let Some(activity_ptr) = NonNull::new(result) else {
+            return Err(());
+        };
+        let _ = unsafe { Activity::ref_from_raw(activity_ptr) };
+        Ok(self)
+    }
+
+    /// Register this [Workflow], making it immutable and available for use.
+    pub fn register(self) -> Result<Ref<Workflow>, ()> {
+        self.register_with_config("")
+    }
+
+    /// Register this [Workflow], making it immutable and available for use.
+    ///
+    /// * `configuration` - a JSON representation of the workflow configuration
+    pub fn register_with_config(self, config: &str) -> Result<Ref<Workflow>, ()> {
+        // TODO: We need to hide the JSON here behind a sensible/typed API.
+        let config = config.to_cstr();
+        if unsafe { BNRegisterWorkflow(self.raw_handle(), config.as_ptr()) } {
+            Ok(self.handle)
+        } else {
+            Err(())
+        }
+    }
+
     /// Assign the list of `activities` as the new set of children for the specified `activity`.
     ///
     /// * `activity` - the Activity node to assign children
     /// * `activities` - the list of Activities to assign
-    pub fn assign_subactivities<I>(&self, activity: &str, activities: I) -> bool
+    pub fn subactivities<I>(self, activity: &str, activities: I) -> Result<Self, ()>
     where
         I: IntoIterator,
         I::Item: IntoCStr,
@@ -355,26 +469,36 @@ impl Workflow {
         let activity = activity.to_cstr();
         let input_list: Vec<_> = activities.into_iter().map(|a| a.to_cstr()).collect();
         let mut input_list_ptr: Vec<*const _> = input_list.iter().map(|x| x.as_ptr()).collect();
-        unsafe {
+        let result = unsafe {
             BNWorkflowAssignSubactivities(
-                self.handle.as_ptr(),
+                self.raw_handle(),
                 activity.as_ptr(),
                 input_list_ptr.as_mut_ptr(),
                 input_list.len(),
             )
+        };
+        if result {
+            Ok(self)
+        } else {
+            Err(())
         }
     }
 
     /// Remove all Activity nodes from this [Workflow].
-    pub fn clear(&self) -> bool {
-        unsafe { BNWorkflowClear(self.handle.as_ptr()) }
+    pub fn clear(self) -> Result<Self, ()> {
+        let result = unsafe { BNWorkflowClear(self.raw_handle()) };
+        if result {
+            Ok(self)
+        } else {
+            Err(())
+        }
     }
 
     /// Insert the list of `activities` before the specified `activity` and at the same level.
     ///
     /// * `activity` - the Activity node for which to insert `activities` before
     /// * `activities` - the list of Activities to insert
-    pub fn insert<I>(&self, activity: &str, activities: I) -> bool
+    pub fn insert<I>(self, activity: &str, activities: I) -> Result<Self, ()>
     where
         I: IntoIterator,
         I::Item: IntoCStr,
@@ -382,13 +506,18 @@ impl Workflow {
         let activity = activity.to_cstr();
         let input_list: Vec<_> = activities.into_iter().map(|a| a.to_cstr()).collect();
         let mut input_list_ptr: Vec<*const _> = input_list.iter().map(|x| x.as_ptr()).collect();
-        unsafe {
+        let result = unsafe {
             BNWorkflowInsert(
-                self.handle.as_ptr(),
+                self.raw_handle(),
                 activity.as_ptr(),
                 input_list_ptr.as_mut_ptr(),
                 input_list.len(),
             )
+        };
+        if result {
+            Ok(self)
+        } else {
+            Err(())
         }
     }
 
@@ -396,7 +525,7 @@ impl Workflow {
     ///
     /// * `activity` - the Activity node for which to insert `activities` after
     /// * `activities` - the list of Activities to insert
-    pub fn insert_after<I>(&self, activity: &str, activities: I) -> bool
+    pub fn insert_after<I>(self, activity: &str, activities: I) -> Result<Self, ()>
     where
         I: IntoIterator,
         I::Item: IntoCStr,
@@ -404,105 +533,46 @@ impl Workflow {
         let activity = activity.to_cstr();
         let input_list: Vec<_> = activities.into_iter().map(|a| a.to_cstr()).collect();
         let mut input_list_ptr: Vec<*const _> = input_list.iter().map(|x| x.as_ptr()).collect();
-        unsafe {
+        let result = unsafe {
             BNWorkflowInsertAfter(
-                self.handle.as_ptr(),
+                self.raw_handle(),
                 activity.as_ptr(),
                 input_list_ptr.as_mut_ptr(),
                 input_list.len(),
             )
+        };
+        if result {
+            Ok(self)
+        } else {
+            Err(())
         }
     }
 
     /// Remove the specified `activity`
-    pub fn remove(&self, activity: &str) -> bool {
+    pub fn remove(self, activity: &str) -> Result<Self, ()> {
         let activity = activity.to_cstr();
-        unsafe { BNWorkflowRemove(self.handle.as_ptr(), activity.as_ptr()) }
+        let result = unsafe { BNWorkflowRemove(self.raw_handle(), activity.as_ptr()) };
+        if result {
+            Ok(self)
+        } else {
+            Err(())
+        }
     }
 
     /// Replace the specified `activity`.
     ///
     /// * `activity` - the Activity to replace
     /// * `new_activity` - the replacement Activity
-    pub fn replace(&self, activity: &str, new_activity: &str) -> bool {
+    pub fn replace(self, activity: &str, new_activity: &str) -> Result<Self, ()> {
         let activity = activity.to_cstr();
         let new_activity = new_activity.to_cstr();
-        unsafe {
-            BNWorkflowReplace(
-                self.handle.as_ptr(),
-                activity.as_ptr(),
-                new_activity.as_ptr(),
-            )
-        }
-    }
-
-    /// Generate a FlowGraph object for the current [Workflow] and optionally show it in the UI.
-    ///
-    /// * `activity` - if specified, generate the Flowgraph using `activity` as the root
-    /// * `sequential` - whether to generate a **Composite** or **Sequential** style graph
-    pub fn graph(&self, activity: &str, sequential: Option<bool>) -> Option<Ref<FlowGraph>> {
-        let sequential = sequential.unwrap_or(false);
-        let activity = activity.to_cstr();
-        let graph =
-            unsafe { BNWorkflowGetGraph(self.handle.as_ptr(), activity.as_ptr(), sequential) };
-        if graph.is_null() {
-            return None;
+        let result = unsafe {
+            BNWorkflowReplace(self.raw_handle(), activity.as_ptr(), new_activity.as_ptr())
+        };
+        if result {
+            Ok(self)
+        } else {
+            Err(())
         }
-        Some(unsafe { FlowGraph::ref_from_raw(graph) })
-    }
-
-    /// Not yet implemented.
-    pub fn show_metrics(&self) {
-        unsafe { BNWorkflowShowReport(self.handle.as_ptr(), c"metrics".as_ptr()) }
-    }
-
-    /// Show the Workflow topology in the UI.
-    pub fn show_topology(&self) {
-        unsafe { BNWorkflowShowReport(self.handle.as_ptr(), c"topology".as_ptr()) }
-    }
-
-    /// Not yet implemented.
-    pub fn show_trace(&self) {
-        unsafe { BNWorkflowShowReport(self.handle.as_ptr(), c"trace".as_ptr()) }
-    }
-}
-
-impl ToOwned for Workflow {
-    type Owned = Ref<Self>;
-
-    fn to_owned(&self) -> Self::Owned {
-        unsafe { RefCountable::inc_ref(self) }
-    }
-}
-
-unsafe impl RefCountable for Workflow {
-    unsafe fn inc_ref(handle: &Self) -> Ref<Self> {
-        Ref::new(Self {
-            handle: NonNull::new(BNNewWorkflowReference(handle.handle.as_ptr()))
-                .expect("valid handle"),
-        })
-    }
-
-    unsafe fn dec_ref(handle: &Self) {
-        BNFreeWorkflow(handle.handle.as_ptr());
-    }
-}
-
-impl CoreArrayProvider for Workflow {
-    type Raw = *mut BNWorkflow;
-    type Context = ();
-    type Wrapped<'a> = Guard<'a, Workflow>;
-}
-
-unsafe impl CoreArrayProviderInner for Workflow {
-    unsafe fn free(raw: *mut Self::Raw, count: usize, _context: &Self::Context) {
-        BNFreeWorkflowList(raw, count)
-    }
-
-    unsafe fn wrap_raw<'a>(raw: &'a Self::Raw, context: &'a Self::Context) -> Self::Wrapped<'a> {
-        Guard::new(
-            Workflow::from_raw(NonNull::new(*raw).expect("valid handle")),
-            context,
-        )
     }
 }
diff --git a/rust/tests/workflow.rs b/rust/tests/workflow.rs
index bf8818c8..8fbe0240 100644
--- a/rust/tests/workflow.rs
+++ b/rust/tests/workflow.rs
@@ -8,8 +8,11 @@ use binaryninja::workflow::Workflow;
 #[test]
 fn test_workflow_clone() {
     let _session = Session::new().expect("Failed to initialize session");
-    let original_workflow = Workflow::new("core.function.baseAnalysis");
-    let mut cloned_workflow = original_workflow.clone_to("clone_workflow");
+    let original_workflow = Workflow::get("core.function.metaAnalysis").unwrap();
+    let mut cloned_workflow = original_workflow
+        .clone_to("clone_workflow")
+        .register()
+        .unwrap();
 
     assert_eq!(cloned_workflow.name().as_str(), "clone_workflow");
     assert_eq!(
@@ -17,36 +20,30 @@ fn test_workflow_clone() {
         original_workflow.configuration()
     );
 
-    cloned_workflow = original_workflow.clone_to("");
+    // `clone_to` with an empty name should re-use the original workflow's name.
+    cloned_workflow = original_workflow.clone_to("").register().unwrap();
     assert_eq!(
         cloned_workflow.name().as_str(),
-        "core.function.baseAnalysis"
+        original_workflow.name().as_str()
     );
 }
 
 #[test]
 fn test_workflow_registration() {
     let _session = Session::new().expect("Failed to initialize session");
-    // Validate NULL workflows cannot be registered
-    let workflow = Workflow::new("null");
-    assert_eq!(workflow.name().as_str(), "null");
-    assert!(!workflow.registered());
-    workflow
-        .register()
-        .expect_err("Re-registration of null is allowed");
 
     // Validate new workflows can be registered
-    let test_workflow = Workflow::instance("core.function.baseAnalysis").clone_to("test_workflow");
+    let test_workflow = Workflow::get("core.function.baseAnalysis")
+        .unwrap()
+        .clone_to("test_workflow");
 
-    assert_eq!(test_workflow.name().as_str(), "test_workflow");
-    assert!(!test_workflow.registered());
-    test_workflow
+    let test_workflow = test_workflow
         .register()
         .expect("Failed to register workflow");
     assert!(test_workflow.registered());
     assert_eq!(
         test_workflow.size(),
-        Workflow::instance("core.function.baseAnalysis").size()
+        Workflow::get("core.function.baseAnalysis").unwrap().size()
     );
     Workflow::list()
         .iter()
@@ -57,32 +54,12 @@ fn test_workflow_registration() {
         .iter()
         .find(|&w| w == "test_workflow")
         .expect("Workflow not found in settings");
-
-    // Validate that registered workflows are immutable
-    let immutable_workflow = Workflow::instance("test_workflow");
-    assert!(!immutable_workflow.clear());
-    assert!(immutable_workflow.contains("core.function.advancedAnalysis"));
-    assert!(!immutable_workflow.remove("core.function.advancedAnalysis"));
-    assert!(!Workflow::instance("core.function.baseAnalysis").clear());
+    assert!(Workflow::get("test_workflow").is_some());
 
     // Validate re-registration of baseAnalysis is not allowed
-    let base_workflow_clone = Workflow::instance("core.function.baseAnalysis").clone_to("");
-
-    assert_eq!(
-        base_workflow_clone.name().as_str(),
-        "core.function.baseAnalysis"
-    );
-    assert!(!base_workflow_clone.registered());
+    let base_workflow_clone = Workflow::cloned("core.function.baseAnalysis").unwrap();
     base_workflow_clone
         .register()
+        .map(|_| ())
         .expect_err("Re-registration of baseAnalysis is allowed");
-    assert_eq!(
-        base_workflow_clone.size(),
-        Workflow::instance("core.function.baseAnalysis").size()
-    );
-    assert_eq!(
-        base_workflow_clone.configuration(),
-        Workflow::instance("core.function.baseAnalysis").configuration()
-    );
-    assert!(!base_workflow_clone.registered());
 }